[vlc-devel] [PATCH] Adding caf file demuxer module.

[Matthias Keiser]

Hello Rafaël, thanks for the input. I'm working on incorporating your change requests.

I have some questions and comments, though:

>> +	libcaf_plugin.la \
> 
> This part doesn't apply anymore, IIUC you should modify Makefile.am instead.

I just wanted to double-check, because the module guide (https://wiki.videolan.org/Hacker_Guide/How_To_Write_a_Module/) sais to modify Modules.am and the Makefile.am you suggest I should edit says "DO NOT EDIT THIS FILE DIRECTLY! See Modules.am instead."  and "automatically generated from Modules.am" right at the top.

>> +    span1->i_frames += span2->i_frames;
>> +    span1->i_samples += span2->i_samples;
>> +    span1->i_bytes += span2->i_bytes;
> 
> Since this is only used to know position in file, shouldn't we store
> only one of them and calculate the others?

Well, since the frame size, the number of samples per frame, and the frame description size itself can be of variable size, this would mean we would have to walk an array of potentially 2^64-1 members every time we wanted to know any of these values. This is the reason I introduced this structure.

> Storing a double in the stream looks terrible though, especially since
> it's the sample rate and we convert it to integer anyway.

I guess the caf spec was not designed with the VLC API in mind. ;)

> This looks right and totally relevant to demuxed.

Since the caf spec is totally codec agnostic, in a "perfect world", the module in charge of parsing the caf file could simply hand off the codec specific information (he "magic cookie"-chunk, which is a total black box for the caf spec) to the codec module. This doesn't work for aac and alac:
An alac file has a magic cookie of length 24, but the alac module requires it's "extra" information to be 36 bytes long because apparently some earlier version of alac had a 36 byte cookie. So what do we do? We prepend 12 bytes of zeroes to our cookie and send it to the lib. If this isn't a hack, I don't know what a hack is. Also this should clearly be handled by the alac module.
In the case of aac, the codec module only wants a subset of the magic cookie chunk. It simply would be nice if it could pick itself the bytes it wants to use. This wouldn't be just useful for caf files, because the magic cookie chunk in caf is identical to the esds-atom in an mp4 file.
But yeah, I can remove those comments.

>> +            if( stream_Peek( p_demux->s, &p_peek, (int)i_size ) < i_size )
> 
> The cast does not look good, what happens if it's negative when casted
> to int?

Well, what I expected to happen was that stream_Peek with a negative value would simple return 0 or a negative number. Basically this would mean we don't handle bigger chunks than of size INT32_MAX. I basically would be ok with that. What would you do? Keep in mind that i_size is a 64 bit value, so on platforms with 32 bit size_t we can't possibly succeed here for numbers above  UINT32_MAX (can't allocate buffer). So in some situations we already fail because we don't handle the high 32 bits of the number, is failing for one more bit really that bad? Would you prefer that I check i_size before this call and bail explicitly if it is too big? I could also read the data in INT32_MAX-sized chunks (which would not save the problem of sizes > UINT32_MAX on 32bit platforms). Frankly, I personally think it is not worth the trouble.

Thanks and best regards

Matthias