[FFmpeg-devel] [PATCH] Indeo 5 decoder

[Maxim]

Kostya schrieb:
> On Sat, Feb 06, 2010 at 11:07:06AM +0100, Reimar D?ffinger wrote:
>   
>> On Sat, Feb 06, 2010 at 11:39:24AM +0200, Kostya wrote:
>>     
>>> On Sat, Feb 06, 2010 at 10:07:10AM +0100, Reimar D?ffinger wrote:
>>>       
>>>> On Thu, Feb 04, 2010 at 09:44:13AM +0200, Kostya wrote:
>>>>         
>>>>> On Wed, Feb 03, 2010 at 09:27:29PM +0100, Reimar D?ffinger wrote:
>>>>>           
>>>>>> On Mon, Feb 01, 2010 at 07:49:55PM +0200, Kostya wrote:
>>>>>>             
>>>>>>> +    uint32_t        frame_num;
>>>>>>>               
>>>>>> Only a 8-bit value is stored in that, also it is unused...
>>>>>>             
>>>>> still, it's in the bitstream. I'd like to keep it for debug purposes. 
>>>>>           
>>>> I wasn't objecting to reading it, I just don't like it much that
>>>> you're storing it in a context variable, that makes it rather hard
>>>> to find out it's not used.
>>>> Why do you think it is worse to do e.g.
>>>>
>>>>         
>>>>> +    ctx->frame_num = get_bits(&ctx->gb, 8);
>>>>>           
>>>> skip_bits(&ctx->gb, 8); // frame_num
>>>> (or keep the get_bits, then it's even less change to re-add it).
>>>>         
>>> Not that I have any strict preferences, decoder author used this and I'd
>>> like to keep it this way.
>>>       
>> Then at least add a comment somewhere for the poor person trying to find out
>> what the purpose of that is.
>> Possibly sort all those unused variables at the end and put a "// these are currently
>> not used" above it or something.
>> Also the "decoder author used this" would be more convincing if I knew if this was
>> intentional or if it's just a left-over from the beginning when it was unclear
>> if it would be needed or not.
>>     
>
> Should be obvious for frame_num, can't say anything for the other fields,
> but it looks to me that Maxim tried to follow original codec structure
> closely.
>   

I'm online again (I was moved to another town last week...)

The variables "frame_num" and "gop_hdr_size" belong to some error
cancealment code I wrote but never included into the final patch...

- "frame_num" was used to detect if there was any frame drop in the
sequence. If yes, the decoder should wait for the next key frame...

"gop_num" was intended for debug purposes only...

Regards
Maxim