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

[Michael Niedermayer]

On Sun, Feb 08, 2009 at 10:07:23AM +0100, Ivan Schreter wrote:
> Hi,
>
> Michael Niedermayer wrote:
>> On Sat, Feb 07, 2009 at 01:45:06PM +0100, Ivan Schreter wrote:
>>   
>>> [...]
>>> #3: Decode SEI recovery point and set key_frame appropriately. Still 
>>> missing a possibility to pass recovery frame count to lavf, so this is 
>>> left open.
>>>     
>>
>> comments below
>>
>>
>> [...]
>>   
>>> #5: If h264 decoder becomes a buffer with two field pictures, make sure 
>>> the second one is decoded correctly as well. Until now, NAL_AUD was 
>>> ignored, so decoding would go wrong.
>>>     
>>
>> comment below
>>   
>>> Please review/apply them.
>>>
>>> BTW, it would be nice to have some test file for regression tests with 
>>> field-coded interlaced video. How to go about it?
>>>     
>>
>> you mean you want to add such file to the official regression tests?
>> this is hard unless you happen to have written a encoder for ffmpeg that 
>> can
>> generate such files.
>> Its easier though to add such a thing to fate ...
>>   
> Can't we use a "foreign" file (if we can get small enough sample) and just 
> test decoding and seeking with it? It's not _that_ important to have it, I 
> just find it nice to have some regression test so field-coded interlaced 
> streams won't break in future.

we could add such a file in principle but dont have a small file that covers
much of what should be covered.


>
> As for fate tests, who is to be contacted directly about it (after changes 
> committed)? I suppose, the sample can be a little bigger there...

mike is the one to contact ...
also keep in mind that fate already contains the reference h264 streams,
it decodes them currently by using -vsync 0 if your changes will allow
them to be decoded corectly without that then it should be pretty easy
to update fate ...


>
>>
>> [...]
>>   
>>> Index: libavcodec/h264.c
>>> ===================================================================
>>> --- libavcodec/h264.c	(revision 17030)
>>> +++ libavcodec/h264.c	(working copy)
>>> @@ -6848,6 +6839,15 @@
>>>      return 0;
>>>  }
>>>  +static int decode_recovery_point(H264Context *h){
>>> +    MpegEncContext * const s = &h->s;
>>> +
>>>     
>>
>>   
>>> +    h->sei_recovery_frame_cnt = get_ue_golomb(&s->gb);      /* 
>>> recovery_frame_cnt */
>>>     
>>
>> the comment is redundant the variable name alraedy makes it clear
>>   
> Removed.
>>   
>>> +
>>> +    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


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


>
> I attached corrected patch #3.
>
>> [...]
>>   
>>> Index: libavcodec/h264.c
>>> ===================================================================
>>> --- libavcodec/h264.c	(revision 17030)
>>> +++ libavcodec/h264.c	(working copy)
>>> @@ -7340,6 +7344,19 @@
>>>      H264Context *hx; ///< thread context
>>>      int context_count = 0;
>>>  +    /*
>>> +     * Interlaced H.264 stream can be encoded as field pictures (i.e., 
>>> two
>>> +     * pictures per frame). If the input buffer contains both, then they 
>>> are
>>> +     * delimited by access unit delimiter. These flags allow us to 
>>> decode only
>>>     
>>
>> I dont see anything in the H.264 spec that says that AU delimiters have to 
>> be
>> used in this case.
>>   
> OK, maybe I put it a bit wrong. The decoder itself will work, but the 
> bookkeeping for a decoded picture done in decode_frame() after 
> decode_nal_units() (stuff like MPV_frame_end() and/or ff_er_frame_end()) 
> doesn't get executed if decode_nal_units() doesn't return after an NAL_AUD.
>
> Alternatively, one could move this into decode_nal_units() and do it as 
> response to NAL_AUD. I just wanted to minimize changes there and I prefer 
> to leave it as-is instead of stuffing unrelated bookkeeping code into 
> decode_nal_units() (which should IMHO decode exactly one access unit). OK?

simple thing
1. which part of h264 says that AUD is mandatory?
2. if none then you cant depend on AUD

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090208/d8e4e179/attachment.pgp>