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

[Michael Niedermayer]

On Sat, Feb 07, 2009 at 01:45:06PM +0100, Ivan Schreter wrote:
> Hi Michael,
>
> Michael Niedermayer wrote:
>> On Sat, Jan 31, 2009 at 04:30:01PM +0100, Ivan Schreter wrote:
>>   
>>> Additionally,
>>>     
>>
>> every additionally belongs in a seperate patch
>>
>>   
>
> According to your wish, here the patch separated into several smaller ones. 
> I also addressed the problem of correct pairing of key frames. Together 
> with MPEG-TS patches I posted last night, interlaced AVCHD decoding and 
> seeking works perfectly (at least those from Panasonic and Sony 
> camcorders).
>
> Description of attached patches:
>

> #1: Make some h264.c functions visible to the parser via h264.h (used in 
> other patches).

ok


>
> #2: Replace hard-coded SEI type constants with symbolic names.

ok


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


[...]

> #7: Trivial patch to fix a constness warning.

ok


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


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


> +    skip_bits(&s->gb, 4);       /* 1b exact_match_flag, 1b broken_link_flag, 2b changing_slice_group_idc */

that comment is fine


> +
> +    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
also it does not set the key_frame flag if there is no decode but just a
AVParser



[...]

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


[...]



-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
-------------- 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/377d497d/attachment.pgp>