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

[Michael Niedermayer]

On Sat, Jan 31, 2009 at 04:30:01PM +0100, Ivan Schreter wrote:
> Hi *,
>
> here the patch to correct some problems when working with interlaced
> H.264/AVCHD files. Two primary problems are addressed:
>  - lack of key frame support in H.264 decoder (this is also relevant
>    for progressive)
>  - proper handling of field pictures for interlaced mode (two fields
>    coded as separate pictures instead of one picture per frame)
>
> Please review it and eventually apply it as-is. Note that I won't have
> much time to fix anything in near future, as my first child is going to
> be born in next couple of days. So it would be good to get through with
> this fix before that happens :-)
>
> How it works?
>
> To support key frames, SEI message recovery point is decoded and recovery
> frame count stored on the context. This is then used to set key frame flag
> in decoding process. In the parser, it is used to communicate (synthetic)
> picture type to libavformat's av_read_frame() routine to correctly set key
> flag and compute missing PTS/DTS timestamps. This should be corrected
> by extending appropriate context structure by adding key frame flag and
> recovery frame count, but I didn't want to do this, since a) it would
> potentially break binary compatiblility and b) one has to fix it in all
> parsers.
>
> To support interlaced mode, it was needed to collate two field pictures
> into one buffer to fulfill av_read_frame() contract - i.e., reading one
> whole frame per call. This is done in h264_parser.c by testing picture
> structure and if a field image is detected, by waiting until the rest
> of the frame (second field) comes in, before returning a buffer.
>

> Additionally,

every additionally belongs in a seperate patch


[...]

> Index: libavcodec/h264.c
> ===================================================================
> --- libavcodec/h264.c	(revision 16851)
> +++ libavcodec/h264.c	(working copy)
> @@ -1359,14 +1359,7 @@
>      }
>  }
>  
> -/**
> - * Decodes a network abstraction layer unit.
> - * @param consumed is the number of bytes used as input
> - * @param length is the length of the array
> - * @param dst_length is the number of decoded bytes FIXME here or a decode rbsp tailing?
> - * @returns decoded bytes, might be src+1 if no escapes
> - */
> -static const uint8_t *decode_nal(H264Context *h, const uint8_t *src, int *dst_length, int *consumed, int length){
> +const uint8_t *ff_h264_decode_nal(H264Context *h, const uint8_t *src, int *dst_length, int *consumed, int length){
>      int i, si, di;
>      uint8_t *dst;
>      int bufidx;

this stuff belongs in a seperate patch


[...]
> @@ -3766,6 +3755,8 @@
>          }
>      }
>  
> +    if(h->sps.separate_colour_plane_flag)
> +        get_bits(&s->gb, 2);    // FIXME: implement support for colour_plane_id
>      h->frame_num= get_bits(&s->gb, h->sps.log2_max_frame_num);
>  
>      h->mb_mbaff = 0;

this too belongs in a sperate patch


> @@ -6836,9 +6827,18 @@
>      return 0;
>  }
>  
> -static int decode_sei(H264Context *h){
> +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 */
> +    skip_bits(&s->gb, 4);       /* 1b exact_match_flag, 1b broken_link_flag, 2b changing_slice_group_idc */
> +
> +    return 0;
> +}
> +
> +int ff_h264_decode_sei(H264Context *h){
> +    MpegEncContext * const s = &h->s;
> +
>      while(get_bits_count(&s->gb) + 16 < s->gb.size_in_bits){
>          int size, type;
>  

so does this


> @@ -6853,14 +6853,18 @@
>          }while(get_bits(&s->gb, 8) == 255);
>  
>          switch(type){
> -        case 1: // Picture timing SEI
> +        case SEI_TYPE_PIC_TIMING: // Picture timing SEI
>              if(decode_picture_timing(h) < 0)
>                  return -1;
>              break;
> -        case 5:
> +        case SEI_TYPE_USER_DATA_UNREGISTERED:
>              if(decode_unregistered_user_data(h, size) < 0)
>                  return -1;
>              break;

replacing litteral numbers by named ones is very welcome but again
seperate patch

[...]
> @@ -49,10 +51,10 @@
>           * and there should be FF_INPUT_BUFFER_PADDING_SIZE bytes at the end
>           */
>  #    if HAVE_FAST_64BIT
> -            while(i<buf_size && !((~*(uint64_t*)(buf+i) & (*(uint64_t*)(buf+i) - 0x0101010101010101ULL)) & 0x8080808080808080ULL))
> +            while(i<buf_size && !((~*(const uint64_t*)(buf+i) & (*(const uint64_t*)(buf+i) - 0x0101010101010101ULL)) & 0x8080808080808080ULL))
>                  i+=8;
>  #    else
> -            while(i<buf_size && !((~*(uint32_t*)(buf+i) & (*(uint32_t*)(buf+i) - 0x01010101U)) & 0x80808080U))
> +            while(i<buf_size && !((~*(const uint32_t*)(buf+i) & (*(const uint32_t*)(buf+i) - 0x01010101U)) & 0x80808080U))
>                  i+=4;
>  #    endif
>  #endif

again seperate patch

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

The bravest are surely those who have the clearest vision
of what is before them, glory and danger alike, and yet
notwithstanding go out to meet it. -- Thucydides
-------------- 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/20090203/eb2d0d9d/attachment.pgp>