[FFmpeg-devel] [PATCH] Implement PAFF in H.264
Michael Niedermayer
michaelni
Wed Sep 19 00:08:47 CEST 2007
Hi
just a quick review below, ill wait with a full review until there are
clean and well split patches ...
On Tue, Sep 18, 2007 at 04:24:54PM -0400, Jeff Downs wrote:
[...]
>>> if(opcode==MMCO_SHORT2UNUSED ||
>>> opcode==MMCO_SHORT2LONG){
>>> - h->mmco[i].short_frame_num= (h->frame_num -
>>> get_ue_golomb(gb) - 1) & ((1<<h->sps.log2_max_frame_num)-1); //FIXME
>>> fields
>>> + h->mmco[i].short_pic_num= h->curr_pic_num -
>>> get_ue_golomb(gb) - 1;
>>> /* if(h->mmco[i].short_frame_num >=
>>> h->short_ref_count || h->short_ref[ h->mmco[i].short_frame_num ] ==
>>> NULL){
>>> av_log(s->avctx, AV_LOG_ERROR, "illegal short
>>> ref in memory management control operation %d\n", mmco);
>>> return -1;
>>> }*/
>>> }
>>> if(opcode==MMCO_SHORT2LONG || opcode==MMCO_LONG2UNUSED
>>> || opcode==MMCO_LONG || opcode==MMCO_SET_MAX_LONG){
>>> - unsigned int long_index= get_ue_golomb(gb);
>>> - if(/*h->mmco[i].long_index >= h->long_ref_count ||
>>> h->long_ref[ h->mmco[i].long_index ] == NULL*/ long_index >= 16){
>>> + unsigned int long_arg= get_ue_golomb(gb);
>>> + if(long_arg >= 32 || (long_arg >= 16 && !(opcode ==
>>> MMCO_LONG2UNUSED && FIELD_PICTURE))){
>>> av_log(h->s.avctx, AV_LOG_ERROR, "illegal long
>>> ref in memory management control operation %d\n", opcode);
>>> return -1;
>>> }
>>> - h->mmco[i].long_index= long_index;
>>> + h->mmco[i].long_arg= long_arg;
>>> }
>>
>> Maybe it would be better to do the renaming of that var before or after
>> the bulk of your patch is applied.
>
> I can do that if necessary; the renaming was done to have the variable make
> sense in both frame and field contexts.
yes, please do, all cosmetics must be in a seperate patch
[...]
> /**
> + * Split one reference list into field parts, interleaving by parity
> + * as per H.264 spec section 8.2.4.2.5. Output fields have their data pointers
> + * set to look at the actual start of data for that field.
> + *
> + * @param dest output list
> + * @param dest_len maximum number of fields to put in dest
> + * @param src the source reference list containing fields and/or field pairs
> + * (aka short_ref/long_ref, or
> + * refFrameListXShortTerm/refFrameListLongTerm in spec-speak)
> + * @param src_len number of Picture's in source (pairs and unmatched fields)
> + * @param parity the parity of the picture being decoded/needing
> + * these ref pics (PICT_{TOP,BOTTOM}_FIELD)
> + * @return number of fields placed in dest
> + */
> +static inline int split_field_half_ref_list(Picture *dest, int dest_len, Picture *src, int src_len, int parity)
> +{
i dont think this should be inline, it doesnt appear speed critical
> + int same_i, opp_i;
> + int i;
> + int same;
> + int out_i;
> +
> + same_i = 0;
> + opp_i = 0;
> + same = 1;
> + out_i = 0;
> +
> + while (out_i < dest_len) {
> + if (same && same_i < src_len) {
> + if ((src[same_i].valid_structure & parity)) {
> + same = 0;
> + dest[out_i] = src[same_i];
> + for (i = 0; i < 4; ++i) {
> + if (parity == PICT_BOTTOM_FIELD)
> + dest[out_i].data[i] += dest[out_i].linesize[i];
> + dest[out_i].linesize[i] *= 2;
> + dest[out_i].pic_id *= 2;
> + }
> + out_i++;
> + }
> + same_i++;
> +
> + } else if (opp_i < src_len) {
> + if ((src[opp_i].valid_structure & (PICT_FRAME - parity))) {
> + same = 1;
> + dest[out_i] = src[opp_i];
> + for (i = 0; i < 4; ++i) {
> + if (parity == PICT_TOP_FIELD)
> + dest[out_i].data[i] += dest[out_i].linesize[i];
> + dest[out_i].linesize[i] *= 2;
> + dest[out_i].pic_id *= 2;
> + dest[out_i].pic_id++;
> + }
> + out_i++;
> + }
> + opp_i++;
> +
near duplicate
[...]
> /**
> + * Removes reference marking from a field or field pair by picture number.
> + * If an unmatched field pair, or both fields in a pair become unreferenced,
> + * the field (pair) is removed from the short term reference list and list
> + * state is updated accordingly.
> + * @param picret set to the unreferenced and removed picture, or NULL if the
> + * picture still has fields in reference or was not found.
> + * @return -1 if picture with pic_num not found. 0 otherwise
> + */
> +static int remove_field_short(H264Context *h, int pic_num, Picture **picret){
> + MpegEncContext * const s = &h->s;
> + int i;
> + Picture *pic = NULL;
> + int frame_num = pic_num >> 1;
> +
> + if(s->avctx->debug&FF_DEBUG_MMCO)
> + av_log(h->s.avctx, AV_LOG_DEBUG, "remove field short %d count %d\n", pic_num, h->short_ref_count);
> +
> + for(i=0; i<h->short_ref_count; i++){
> + pic= h->short_ref[i];
> +
> + if(s->avctx->debug&FF_DEBUG_MMCO)
> + av_log(h->s.avctx, AV_LOG_DEBUG, "%d %d %p\n", i, pic->frame_num, pic);
> +
> + if (pic->frame_num == frame_num)
> + break;
> + }
> +
> + *picret = NULL;
> + if (pic) {
> + int mask;
> +
> + mask = (pic_num & 1) ? ~s->picture_structure : s->picture_structure;
> + pic->valid_structure &= mask;
> +
> + if (pic->valid_structure == 0) {
> + h->short_ref[i]= NULL;
> + if (--h->short_ref_count)
> + memmove(&h->short_ref[i], &h->short_ref[i+1], (h->short_ref_count - i)*sizeof(Picture*));
> + *picret = pic;
> + }
> + return 0;
> + }
> +
> + return -1;
> +}
this is largly a duplicate of remove_short()
[...]
> @@ -7402,13 +7788,26 @@
>
> h->prev_frame_num_offset= h->frame_num_offset;
> h->prev_frame_num= h->frame_num;
> - if(s->current_picture_ptr->reference){
> + if(s->current_picture.reference){
this is wrong, current_picture is a copy of current_picture_ptr
if current_picture is correct while _ptr is not theres a bug
[...]
> + /*
> + * FIXME: Error handling code does not seem to support interlaced
> + * when slices span multiple rows
> + */
> + if (!FIELD_PICTURE)
> ff_er_frame_end(s);
why doesnt this work?
[...]
> @@ -84,8 +86,8 @@
> int poc_cycle_length; ///< num_ref_frames_in_pic_order_cnt_cycle
> int ref_frame_count; ///< num_ref_frames
> int gaps_in_frame_num_allowed_flag;
> - int mb_width; ///< frame_width_in_mbs_minus1 + 1
> - int mb_height; ///< frame_height_in_mbs_minus1 + 1
> + int mb_width; ///< pic_width_in_mbs_minus1 + 1
> + int mb_height; ///< pic_height_in_map_units_minus1 + 1
> int frame_mbs_only_flag;
> int mb_aff; ///<mb_adaptive_frame_field_flag
> int direct_8x8_inference_flag;
> @@ -151,8 +153,8 @@
> */
> typedef struct MMCO{
> MMCOOpcode opcode;
> - int short_frame_num;
> - int long_index;
> + int short_pic_num;
> + int long_arg; ///< index, pic_num, or num long refs depending on opcode
> } MMCO;
>
> /**
[...]
> @@ -323,8 +325,9 @@
> unsigned int list_count;
> Picture *short_ref[32];
> Picture *long_ref[32];
> - Picture default_ref_list[2][32];
> + Picture default_ref_list[2][32]; ///< base reference list for all slices of a coded picture
> Picture ref_list[2][48]; ///< 0..15: frame refs, 16..47: mbaff field refs
> + ///< Reordered version of default_ref_list according to picture reordering in slice header
> Picture *delayed_pic[18]; //FIXME size?
> Picture *delayed_output_pic;
cosmetics
>
> Index: libavcodec/mpegvideo.c
> ===================================================================
> --- libavcodec/mpegvideo.c (revision 10526)
> +++ libavcodec/mpegvideo.c (working copy)
> @@ -949,7 +949,7 @@
>
> assert(s->pict_type == I_TYPE || (s->last_picture_ptr && s->last_picture_ptr->data[0]));
>
> - if(s->picture_structure!=PICT_FRAME){
> + if(s->picture_structure!=PICT_FRAME && s->out_format != FMT_H264){
> int i;
> for(i=0; i<4; i++){
> if(s->picture_structure == PICT_BOTTOM_FIELD){
> Index: libavcodec/mpegvideo.h
> ===================================================================
> --- libavcodec/mpegvideo.h (revision 10526)
> +++ libavcodec/mpegvideo.h (working copy)
> @@ -138,8 +138,9 @@
>
> int field_poc[2]; ///< h264 top/bottom POC
> int poc; ///< h264 frame POC
> + int valid_structure; ///< h264 one of PICT_XXXX, stating which fields are referenced
if my memory doesnt fail my then we already have this variable, its called
reference
> - int frame_num; ///< h264 frame_num
> - int pic_id; ///< h264 pic_num or long_term_pic_idx
> + int frame_num; ///< h264 frame_num (raw frame_num from slice header)
> + int pic_id; ///< h264 pic_num (short or long term)
cosmetic
> int long_ref; ///< 1->long term reference 0->short term reference
> int ref_poc[2][16]; ///< h264 POCs of the frames used as reference
> int ref_count[2]; ///< number of entries in ref_poc
> @@ -622,9 +623,9 @@
> int mpeg_f_code[2][2];
> int picture_structure;
> /* picture type */
> -#define PICT_TOP_FIELD 1
> -#define PICT_BOTTOM_FIELD 2
> -#define PICT_FRAME 3
> +#define PICT_TOP_FIELD 0x1
> +#define PICT_BOTTOM_FIELD 0x2
> +#define PICT_FRAME (PICT_TOP_FIELD | PICT_BOTTOM_FIELD)
cosmetic
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
-------------- 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/20070919/a097cbae/attachment.pgp>
More information about the ffmpeg-devel
mailing list