[FFmpeg-devel] [PATCH] H.264/AVCHD interlaced fixes

[Ivan Schreter]

Michael Niedermayer wrote:
> On Sun, Feb 08, 2009 at 03:20:14PM +0100, Ivan Schreter wrote:
>   
>> Hi,
>>
>> Michael Niedermayer wrote:
>>     
>>> On Sun, Feb 08, 2009 at 10:07:23AM +0100, Ivan Schreter wrote:
>>>   
>>>       
>>>>> [...]  
>>>>>       
>>>>>           
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>>  int ff_h264_decode_sei(H264Context *h){
>>>>>>      MpegEncContext * const s = &h->s;
>>>>>>  @@ -6873,6 +6873,10 @@
>>>>>>              if(decode_unregistered_user_data(h, size) < 0)
>>>>>>                  return -1;
>>>>>>              break;
>>>>>> +        case SEI_TYPE_RECOVERY_POINT:
>>>>>> +            if(decode_recovery_point(h) < 0)
>>>>>> +                return -1;
>>>>>> +            break;
>>>>>>          default:
>>>>>>              skip_bits(&s->gb, 8*size);
>>>>>>          }
>>>>>> @@ -7340,6 +7344,8 @@
>>>>>>      int context_count = 0;
>>>>>>       h->max_contexts = avctx->thread_count;
>>>>>> +    h->sei_recovery_frame_cnt = -1;
>>>>>> +
>>>>>>  #if 0
>>>>>>      int i;
>>>>>>      for(i=0; i<50; i++){
>>>>>> @@ -7676,6 +7682,14 @@
>>>>>>          } else {
>>>>>>              cur->repeat_pict = 0;
>>>>>>
>>>>>> +            /*
>>>>>> +             * TODO: Currently, we only communicate the fact we have a 
>>>>>> key
>>>>>> +             * frame, but there is no possibility to communicate 
>>>>>> recovery
>>>>>> +             * frame count. Most probably, recovery frame count is 
>>>>>> anyway 0.
>>>>>> +             * Add this in the future.
>>>>>> +             */
>>>>>> +            cur->key_frame = (h->sei_recovery_frame_cnt >= 0);
>>>>>>     
>>>>>>         
>>>>>>             
>>>>> this could set key_frame == 0 for IDR, which is wrong
>>>>>   
>>>>>       
>>>>>           
>>>> Correct, my mistake. This should fix it:
>>>>
>>>> @@ -7422,6 +7445,7 @@
>>>>                 return -1;
>>>>             }
>>>>             idr(h); //FIXME ensure we don't loose some frames if there is 
>>>> reordering
>>>> +            h->sei_recovery_frame_cnt = 0;
>>>>         case NAL_SLICE:
>>>>             init_get_bits(&hx->s.gb, ptr, bit_length);
>>>>             hx->intra_gb_ptr=
>>>>
>>>> OK now?
>>>>     
>>>>         
>>> now its a hack
>>> i mean idr != sei_recovery_frame_cnt
>>>
>>>   
>>>       
>> True, but IDR == key frame, so implicitly it has recovery frame count == 
>> 0. So this will correctly detect it. How else would you do it?
>>     
>
> keyframe |= ...
>   
Do you mean I should add additional "keyframe" flag? I can do that...

>> h->sei_recovery_frame_cnt >= 0 iff it's a key frame. It's a keyframe, 
>> iff h->sei_recovery_frame_cnt >= 0. The only thing which might 
>> theoretically break it is a SEI recovery point associated with IDR frame 
>> having recovery_frame_cnt > 0. But this is not logical, since IDR frame 
>> is the begin of coding sequence, so it must be decodable without 
>> depending on previous frames. Therefore, I think it's OK so and not a hack.
>>     
>
> sei_recovery_frame_cnt implicates the existence of a SEI, IDR does not
>   
Yes, but the end result is exactly the same - a key frame.

>>>>> also it does not set the key_frame flag if there is no decode but just a
>>>>> AVParser
>>>>>   
>>>>>       
>>>>>           
>>>> I don't quite understand what you mean. Parser is addressed by patch #6, 
>>>> which handles both field combining and key frame flag. It was very hard to 
>>>> split it in two patches, therefore I kept it in one.
>>>>     
>>>>         
>>> i didnt review #6 as it was big & scary IIRC
>>>   
>>>       
>> Umm... It is a little bigger, but not _that_ big. And it's pretty 
>> straightforward - after finding a complete packet, this packet is parsed 
>> via parse_nal_units(), which is nothing else but the code I got from 
>> you, corrected and extended by a few things (namely reading field type 
>> and frame number). In parser itself, it then looks if it's an unpaired 
>> field picture, and if so, it will not return the packet yet, but note 
>> down the position in buffer and relevant picture type info, wait for 
>> next packet and pair it with it. As error handling, if an unpaired field 
>> comes, it will be discarded, since it can't be combined anyway into a 
>> full frame.
>>
>> I have commented the code IMHO quite extensively, so it should be easy 
>> to understand.
>>     
>
> the parsing and combining are seperate things requireing seperate
> patches.
>   
Oh, c'mon! I have split the big patch as good as possible. But I don't 
have infinitely much time, my small daughter was born a few days ago => 
end of hacking. I'd like to have my patches in for the release, though.

Regards,

Ivan