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

[Ivan Schreter]

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.

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

>
> [...]
>   
>> 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?

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

BTW, what about patch #6 (and #4, but that's just a helper)?

Regards,

Ivan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: avchd_3_recovery_point.patch
Type: text/x-patch
Size: 2656 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090208/dfd9f02d/attachment.bin>