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

Michael Niedermayer michaelni
Fri Feb 13 02:20:27 CET 2009


On Tue, Feb 10, 2009 at 09:36:35PM +0100, Ivan Schreter wrote:
> Hi Michael,
>
> Michael Niedermayer wrote:
>
>>>> the parsing and combining are seperate things requireing seperate
>>>> patches.
>>>>       
> OK, so here the parsing-only patch.
[...]
> #6a is the parsing of additional information from H.264 picture to get key 
> frame flag, recovery count, field picture flag and frame number out of the 
> stream in parser for latter use in lavf. Currently, only key frame flag can 
> be transported via picture type, so recovery count and field picture flag 
> are not yet communicated. These will be needed, though, in implementation 
> of correct timestamps (and/or frame combining).
>
> So please review #6a.
>
> Regards,
>
> Ivan
>
>


[...]

> Index: libavcodec/h264_parser.c
> ===================================================================
> --- libavcodec/h264_parser.c	(revision 17130)
> +++ libavcodec/h264_parser.c	(working copy)
> @@ -27,6 +27,8 @@
>  
>  #include "parser.h"
>  #include "h264_parser.h"
> +#include "h264data.h"
> +#include "golomb.h"
>  
>  #include <assert.h>
>  
> @@ -96,6 +98,167 @@
>      return i-(state&5);
>  }
>  
> +/*!
> + * Parse NAL units of found picture and decode some basic information.
> + *
> + * @param s parser context.
> + * @param avctx codec context.
> + * @param buf buffer with field/frame data.
> + * @param buf_size size of the buffer.
> + * @param ppict_type set to picture type.
> + * @param pfield_pict set to 1 for field picture, 0 for frame picture.
> + * @param pkey_frame set to 1, if key frame found (IDR or SEI recovery point).
> + * @param precovery_frame_cnt set to recovery frame count for key frames
> + *              (if SEI recovery point present), otherwise set to -1.
> + * @param pframe_num set to frame number.
> + */
> +static int parse_nal_units(AVCodecParserContext *s,
> +                           AVCodecContext *avctx,
> +                           const uint8_t *buf, int buf_size,
> +                           int *ppict_type,
> +                           int *pfield_pict,
> +                           int *pkey_frame,
> +                           int *precovery_frame_cnt,
> +                           int *pframe_num)
> +{
> +    H264Context *h = s->priv_data;
> +    const uint8_t *buf_end= buf + buf_size;
> +    int pps_id;
> +    int slice_type;
> +    const uint8_t *ptr;
> +
> +    /* set some sane values */
> +    *ppict_type = FF_I_TYPE;
> +    *precovery_frame_cnt = -1;
> +    *pkey_frame = 0;
> +    *pfield_pict = 0;
> +
> +    h->s.avctx= avctx;
> +    h->sei_recovery_frame_cnt = -1;
> +    for(;;){
> +        int dst_length, consumed, bit_length;
> +        for(; buf + 3 < buf_end; buf++){
> +            // This should always succeed in the first iteration.
> +            if(buf[0] == 0 && buf[1] == 0 && buf[2] == 1)
> +                break;
> +        }
> +        buf += 3;
> +        if(buf >= buf_end)
> +            break;

> +        ptr= ff_h264_decode_nal(h, buf, &dst_length, &consumed, buf_end - buf);

this effectively reads through the whole bitstream
obviously this is not ok and unneeded


> +        if (ptr==NULL || dst_length < 0)
> +            break;
> +        while(ptr[dst_length - 1] == 0 && dst_length > 0)
> +            dst_length--;
> +        bit_length= !dst_length ? 0 : (8*dst_length - ff_h264_decode_rbsp_trailing(h, ptr + dst_length - 1));
> +
> +        init_get_bits(&h->s.gb, ptr, bit_length);
> +        switch(h->nal_unit_type){
> +        case NAL_SPS:
> +            ff_h264_decode_seq_parameter_set(h);
> +            break;
> +        case NAL_PPS:
> +            ff_h264_decode_picture_parameter_set(h, h->s.gb.size_in_bits);
> +            break;
> +        case NAL_SEI:
> +            ff_h264_decode_sei(h);
> +            break;
> +        case NAL_IDR_SLICE:
> +        case NAL_SLICE:
> +            get_ue_golomb(&h->s.gb);  // skip first_mb_in_slice
> +            slice_type = get_ue_golomb_31(&h->s.gb) % 5;
> +            *ppict_type= golomb_to_pict_type[slice_type];
> +            *precovery_frame_cnt = h->sei_recovery_frame_cnt;
> +            if (h->nal_unit_type == NAL_IDR_SLICE) {
> +                /* this is an IDR key frame (mostly at begin of the stream) */
> +                *pkey_frame = 1;
> +            } else if (h->sei_recovery_frame_cnt >= 0) {
> +                /* key frame, since recovery_frame_cnt is set */
> +                *pkey_frame = 1;
> +            }
> +            pps_id= get_ue_golomb(&h->s.gb);
> +            if(pps_id>=MAX_PPS_COUNT){
> +                av_log(h->s.avctx, AV_LOG_ERROR, "pps_id out of range\n");
> +                return -1;
> +            }
> +            if(!h->pps_buffers[pps_id]) {
> +                av_log(h->s.avctx, AV_LOG_ERROR, "non-existing PPS referenced\n");
> +                return -1;
> +            }
> +            h->pps= *h->pps_buffers[pps_id];
> +            if(!h->sps_buffers[h->pps.sps_id]) {
> +                av_log(h->s.avctx, AV_LOG_ERROR, "non-existing SPS referenced\n");
> +                return -1;
> +            }
> +            h->sps = *h->sps_buffers[h->pps.sps_id];
> +            *pframe_num = get_bits(&h->s.gb, h->sps.log2_max_frame_num);
> +            if (h->sps.frame_mbs_only_flag) {
> +                /* single picture for a frame */
> +                *pfield_pict = 0;
> +            } else {
> +                if (get_bits1(&h->s.gb)) {   /* field_pict_flag */
> +                    /*
> +                     * This frame is divided in two separate field pictures.
> +                     * Field parity bit follows in picture header, but we
> +                     * don't need it.
> +                     */
> +                    *pfield_pict = 1;
> +                } else {
> +                    *pfield_pict = 0;
> +                }
> +            }

split the patch please so that
IDR-keyframe
SEI recovery
field pics

are in 3 seperate patches
the original patch i wrote was not such an intermingled mix


[...]
> +/**
> + * Set context parameters for lavf.
> + *
> + * @param s parser context.
> + * @param pict_type picture type of the first slice in frame.
> + * @param field_pict set to 1 for field picture, 0 for frame picture.
> + * @param key_frame set to 1 for key pictures, 0 for non-key pictures.
> + * @param recovery_frame_cnt frame count from SEI recovery point if present
> + *              or -1 otherwise.
> + * @param frame_num frame number.
> + */
> +static inline void h264_set_keyframe(AVCodecParserContext *s,
> +                              int pict_type,
> +                              int field_pict,
> +                              int key_frame,
> +                              int recovery_frame_cnt,
> +                              int frame_num)
> +{
> +    /*
> +     * NOTE: Currently, there is no flag to tell libavformat about
> +     * key frames. Instead, it relies on having pict_type set to I
> +     * for key frames, so we use this to communicate it. This should

if your patch contains a hack like this its rejected.
if theres a bug it has to be fixed, not worked around by such hacks

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

Complexity theory is the science of finding the exact solution to an
approximation. Benchmarking OTOH is finding an approximation of the exact
-------------- 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/20090213/b17f7939/attachment.pgp>



More information about the ffmpeg-devel mailing list