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

[Rafaël Carré]

Hello Matthias,

Le 21/09/2013 23:28, Matthias Keiser a écrit :
> 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.

The wiki just became outdated, you will realize if you do git pull
--rebase to fetch latest changes in vlc.git

If you can't adapt the changes to latest vlc.git come on IRC so we tell
you how to do it.

>>> +    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.

Sorry, I meant to store either frames / samples or bytes for each
seeking point, and then e.g.:

samples = frames * frame_size_in_samples * sample_size;
bytes = frames * frame_size_in_samples;

>> 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. ;)

True ^_^

>> 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.

Hmm I understand, I did the same for flac codec which can have 2
different sizes for extra data.

>>> +            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.

Ok after looking, the internal peek functions silently convert the int
to unsigned, so this means it can be called with UINT32_MAX.

If i_size > UINT32_MAX this will fail since stream_Peek will return an int.

So I would say bailing out if > UINT32_MAX is the right thing to do. Do
ypu expect valid files to use such high values?

Another question is can we DoS VLC with a carefully crafted file?

Let's say if it's stuck reading 4GB (-1 byte) on a HTTP link and on a
mobile phone with 3G connection.

Anyway if this is the case then we need to fix Stream_Peek (after fixing
the signed / unsigned of course).

> Thanks and best regards
> 
> Matthias