[Ffmpeg-devel] [PATCH] CRYO APC demuxer

[Anssi Hannula]

Michael Niedermayer wrote:
> Hi

Hi!

> On Sat, Apr 07, 2007 at 08:01:18PM +0300, Anssi Hannula wrote:
>> Anssi Hannula wrote:
>>> Hi!
>>>
>>> Attached is a patch implementing a demuxer for CRYO APC, a simple audio
>>> format using ADPCM encoding.
>>>
>>> Please review.
>> Attached is a patch updated as per Michael's and Reimar's comments.
>>
>> I still av_free() st->codec and st on failure, though, as otherwise
>> there would be a memleak. Maybe av_free_stream() should be separated
>> >from av_close_input_file() and have the demuxers call it when failure
>> occurs in read_header() after streams are allocated, or alternatively
>> have the av_open_input_stream() call it when read_header() returns with
>> an error. That belongs to a separate patch, though.
> 
> [...]
> 
>> +    st->codec->extradata_size = 2 * sizeof(int);
> 
> sizeof(int) is wrong, theres nothing guranteeing that its 4 but the format
> will not magically change if the compiler/architecture has a larger int

Yes it is, I missed it when I changed the size of extradata :/
I'll fix that in the next version.

> 
>> +    st->codec->extradata = av_malloc(st->codec->extradata_size +
>> +                                     FF_INPUT_BUFFER_PADDING_SIZE);
>> +    if (!st->codec->extradata) {
>> +        av_free(st->codec);
>> +        av_free(st);
> 
> if you insist on this freeing stuff

Well IMHO it is preferred to having a memleak... Of course we could just
ignore the allocation failure and do not set extradata at all, avoiding
the freeing.

> at least set the pointers to NULL so
> no double free can happen

Hmm... The st->codec pointer is removed right after the
av_free(st->codec) when av_free(st) is done, so NULLing it seems
pointless to me. And the st pointer is local to this function, so it is
gone right in the next line when the function returns.

Or is there some additional reason to nullify them?

-- 
Anssi Hannula