[FFmpeg-soc] [soc]: r586 - dirac/libavcodec/dirac.c

[Marco Gerards]

Michael Niedermayer <michaelni at gmx.at> writes:


Hi,

> On Wed, Aug 01, 2007 at 03:28:08PM +0200, marco wrote:
>> Author: marco
>> Date: Wed Aug  1 15:28:08 2007
>> New Revision: 586
>> 
>> Log:
>> manage reference frames
> [...]
>> -    for (i = 0; i < retire; i++)
>> -        dirac_get_se_golomb(gb); /* XXX */
>> +        retire_num = dirac_get_se_golomb(gb) +  s->picnum;
>
> i think retire_num should be uint32_t

Yes, all int's that store a frame number should be uint32_t
actually...

>
> [...]
>> @@ -2040,8 +2113,14 @@ static int decode_frame(AVCodecContext *
>>      avcodec_set_dimensions(avctx, s->sequence.luma_width,
>>                             s->sequence.luma_height);
>>  
>> -    if (s->picture.data[0] != NULL)
>> -        avctx->release_buffer(avctx, &s->picture);
>> +    if (s->picture.data[0] != NULL) {
>> +        if (s->picture.reference)
>> +            avcodec_get_frame_defaults(&s->picture);
>> +        else
>> +            avctx->release_buffer(avctx, &s->picture);
>> +    }
>
> i think i would move the if (s->picture.reference) handling down
> after *picture = s->picture; instead of doing it here, i think thats
> clearer

ok.

>> +
>> +    s->picture.reference = (parse_code & 0x04) == 0x04;
>>  
>>      if (avctx->get_buffer(avctx, &s->picture) < 0) {
>>          av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
>> @@ -2051,6 +2130,12 @@ static int decode_frame(AVCodecContext *
>>      if (dirac_decode_frame(avctx))
>>          return -1;
>>  
>> +    s->picture.coded_picture_number = s->picnum;
>
> is it not display_picture_number ?

Good one, yes it is.

>> +
>> +    if (s->picture.reference) {
>> +        s->refframes[s->refcnt++] = s->picture;
>> +    }
>
> i think refcnt should be checked somewhere so it cannot become too large

Yes.  I am waiting for the next update of the specification.  It will
mention the levels that can be used and the amount of reference frames
a certain level can have.

--
Marco