[FFmpeg-devel] [PATCH] Implement PAFF in H.264

Michael Niedermayer michaelni
Wed Oct 3 01:19:59 CEST 2007


Hi

On Tue, Oct 02, 2007 at 11:57:15AM -0400, Jeff Downs wrote:
> On Tue, 2 Oct 2007, Andreas ?an wrote:
[...]
> Patch w/out that hunk attached.
> 
> Thanks for taking a look.
> 
> 	-Jeff

> --- ../ffmpeg-cosmetics/libavcodec/h264.c	2007-10-01 14:02:06.000000000 -0400
> +++ libavcodec/h264.c	2007-10-01 15:56:22.000000000 -0400
> @@ -175,11 +175,11 @@
>      if(for_deblock && (h->slice_num == 1 || h->slice_table[mb_xy] == h->slice_table[mb_xy-s->mb_stride]) && !FRAME_MBAFF)
>          return;
>  
>      //wow what a mess, why didn't they simplify the interlacing&intra stuff, i can't imagine that these complex rules are worth it
>  
> -    top_xy     = mb_xy  - s->mb_stride;
> +    top_xy     = mb_xy  - (s->mb_stride << FIELD_PICTURE);

could mb_stride itself be <<=1 ?
or are there many uses which need it like it is even for field pics?


>      topleft_xy = top_xy - 1;
>      topright_xy= top_xy + 1;
>      left_xy[1] = left_xy[0] = mb_xy-1;
>      left_block[0]= 0;
>      left_block[1]= 1;
> @@ -1701,11 +1701,11 @@
>      int extra_height= h->emu_edge_height;
>      int emu=0;
>      const int full_mx= mx>>2;
>      const int full_my= my>>2;
>      const int pic_width  = 16*s->mb_width;
> -    const int pic_height = 16*s->mb_height >> MB_MBAFF;
> +    const int pic_height = 16*s->mb_height >> (MB_MBAFF || FIELD_PICTURE);
>  
>      if(!pic->data[0]) //FIXME this is unacceptable, some senseable error concealment must be done for missing reference frames
>          return;
>  
>      if(mx&7) extra_width -= 3;
> @@ -1725,11 +1725,11 @@
>          qpix_op[luma_xy](dest_y + delta, src_y + delta, h->mb_linesize);
>      }
>  
>      if(ENABLE_GRAY && s->flags&CODEC_FLAG_GRAY) return;
>  
> -    if(MB_MBAFF){
> +    if(MB_MBAFF || FIELD_PICTURE){
>          // chroma offset when predicting from a field of opposite parity
>          my += 2 * ((s->mb_y & 1) - (h->ref_cache[list][scan8[n]] & 1));
>          emu |= (my>>3) < 0 || (my>>3) + 8 >= (pic_height>>1);
>      }
>      src_cb= pic->data[1] + (mx>>3) + (my>>3)*h->mb_uvlinesize;
> @@ -1760,11 +1760,11 @@
>  
>      dest_y  += 2*x_offset + 2*y_offset*h->  mb_linesize;
>      dest_cb +=   x_offset +   y_offset*h->mb_uvlinesize;
>      dest_cr +=   x_offset +   y_offset*h->mb_uvlinesize;
>      x_offset += 8*s->mb_x;
> -    y_offset += 8*(s->mb_y >> MB_MBAFF);
> +    y_offset += 8*(s->mb_y >> (MB_MBAFF || FIELD_PICTURE));
>  
>      if(list0){
>          Picture *ref= &h->ref_list[0][ h->ref_cache[0][ scan8[n] ] ];
>          mc_dir_part(h, ref, n, square, chroma_height, delta, 0,
>                             dest_y, dest_cb, dest_cr, x_offset, y_offset,
> @@ -1793,11 +1793,11 @@
>  
>      dest_y  += 2*x_offset + 2*y_offset*h->  mb_linesize;
>      dest_cb +=   x_offset +   y_offset*h->mb_uvlinesize;
>      dest_cr +=   x_offset +   y_offset*h->mb_uvlinesize;
>      x_offset += 8*s->mb_x;
> -    y_offset += 8*(s->mb_y >> MB_MBAFF);
> +    y_offset += 8*(s->mb_y >> (MB_MBAFF || FIELD_PICTURE));
>  
>      if(list0 && list1){
>          /* don't optimize for luma-only case, since B-frames usually
>           * use implicit weights => chroma too. */
>          uint8_t *tmp_cb = s->obmc_scratchpad;

these can be commited if we have a FIELD_PICTURE=0 


> @@ -2247,10 +2247,11 @@
>      int i;
>  
>      if(MPV_frame_start(s, s->avctx) < 0)
>          return -1;
>      ff_er_frame_start(s);
> +    s->current_picture_ptr->key_frame= 0;
>  

this line could benefit from a comment explaining how it is/has to be
initalized for field_pics


[...]
> +static int split_field_copy(Picture *dest, Picture *src,
> +                            int parity, int id_add){
> +    int match = (src->reference & parity) != 0;

!!(src->reference & parity);


[...]
> +    int same_i, opp_i;
> +    int i;
> +    int same;
> +    int out_i;
> +
> +    same_i = 0;
> +    opp_i = 0;
> +    same = 1;
> +    out_i = 0;

declaration and initalization can be merged


> +
> +    while (out_i < dest_len) {
> +        if (same && same_i < src_len) {
> +            i = split_field_copy(dest + out_i, src + same_i, parity, 1);
> +            same = !i;
> +            same_i++;
> +
> +        } else if (opp_i < src_len) {
> +            i = split_field_copy(dest + out_i, src + opp_i,
> +                                 PICT_FRAME - parity, 0);
> +            same = i;
> +            opp_i++;
> +
> +        } else {
> +            break;
> +        }
> +        out_i += i;
> +    }

for(out_i = 0; out_i < dest_len; out_i += i)


[...]
> +    if (!FIELD_PICTURE) {
> +        structure_sel = 0;
> +        frame_list[0] = h->default_ref_list[0];
> +        frame_list[1] = h->default_ref_list[1];
> +    } else {
> +        structure_sel = PICT_FRAME;
> +        frame_list[0] = field_entry_list[0];
> +        frame_list[1] = field_entry_list[1];
> +    }

if(FIELD_PICTURE){
}else{
}
is a "not" less and IMHO a tiny bit easier to understand


[...]
> +                        if (FIELD_PICTURE) {
> +                            frame_num = pred >> 1;
> +                            if (pred & 1) {
> +                                pic_structure = s->picture_structure;
> +                                bot = (s->picture_structure == PICT_BOTTOM_FIELD);
> +                            } else {
> +                                pic_structure = PICT_FRAME - s->picture_structure;
> +                                bot = (s->picture_structure == PICT_TOP_FIELD);
> +                            }
> +                        } else {
> +                            frame_num = pred;
> +                            pic_structure = PICT_FRAME;
> +                            bot = 0;
> +                        }
> +
>                          for(i= h->short_ref_count-1; i>=0; i--){
>                              ref = h->short_ref[i];
> -                            assert(ref->reference == 3);
> +                            assert(ref->reference);
>                              assert(!ref->long_ref);
> -                            if(ref->data[0] != NULL && ref->frame_num == pred && ref->long_ref == 0) // ignore non existing pictures by testing data[0] pointer
> +                            if(ref->data[0] != NULL &&
> +                                   ref->frame_num == frame_num &&
> +                                   (ref->reference & pic_structure) &&
> +                                   ref->long_ref == 0) // ignore non existing pictures by testing data[0] pointer
>                                  break;
>                          }
>                          if(i>=0)
> -                            ref->pic_id= ref->frame_num;
> +                            ref->pic_id= pred;
>                      }else{
> +                        int long_idx;
>                          pic_id= get_ue_golomb(&s->gb); //long_term_pic_idx
> -                        if(pic_id>31){
> +
> +                        if (FIELD_PICTURE) {
> +                            long_idx = pic_id >> 1;
> +                            if (pic_id & 1) {
> +                                pic_structure = s->picture_structure;
> +                                bot = (s->picture_structure == PICT_BOTTOM_FIELD);
> +                            } else {
> +                                pic_structure = PICT_FRAME - s->picture_structure;
> +                                bot = (s->picture_structure == PICT_TOP_FIELD);
> +                            }
> +                        } else {
> +                            long_idx = pic_id;
> +                            pic_structure = PICT_FRAME;
> +                            bot = 0;
> +                        }

this looks duplicated


[...]
> -static inline void unreference_pic(H264Context *h, Picture *pic){
> +/**
> + * Mark a picture as no longer needed for reference. The refmask
> + * argument allows unreferencing of individual fields or the whole frame.
> + * If the picture becomes entirely unreferenced, but is being held for
> + * display purposes, it is marked as such.
> + * @param refmask mask of fields to unreference; the mask is bitwise
> + *                anded with the reference marking of pic
> + * @return non-zero if pic becomes entirely unreferenced (except possibly
> + *         for display purposes) zero if one of the fields remains in
> + *         reference
> + */
> +static inline int unreference_pic(H264Context *h, Picture *pic, int refmask){
>      int i;
> -    pic->reference=0;
> +    if (pic->reference &= refmask) {
> +        return 0;
> +    } else {
>      if(pic == h->delayed_output_pic)
>          pic->reference=DELAYED_PIC_REF;
>      else{
>          for(i = 0; h->delayed_pic[i]; i++)
>              if(pic == h->delayed_pic[i]){
>                  pic->reference=DELAYED_PIC_REF;
>                  break;
>              }
>      }
> +        return 1;
> +    }
>  }

this looks ok, if it where in a seperate patch with the related
unreference_pic() call changes that could be commited


[...]
>  /**
> - *
> - * @return the removed picture or NULL if an error occurs
> + * Find a Picture in the short term reference list by frame number.
> + * @param frame_num frame number to search for
> + * @param idx the index into h->short_ref where returned picture is found
> + *            undefined if no picture found.
> + * @return pointer to the found picture, or NULL if no pic with the provided
> + *                 frame number is found
>   */
> -static Picture * remove_short(H264Context *h, int frame_num){
> +static Picture * find_short(H264Context *h, int frame_num, int *idx){
>      MpegEncContext * const s = &h->s;
>      int i;
>  
> -    if(s->avctx->debug&FF_DEBUG_MMCO)
> -        av_log(h->s.avctx, AV_LOG_DEBUG, "remove short %d count %d\n", frame_num, h->short_ref_count);
> -
>      for(i=0; i<h->short_ref_count; i++){
>          Picture *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){
> -            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*));
> +        if(pic->frame_num == frame_num) {
> +            *idx = i;
>              return pic;
>          }
>      }
>      return NULL;
>  }

this split out could also be in its own patch


[...]
> -                    unsigned int long_index= get_ue_golomb(gb);
> -                    if(/*h->mmco[i].long_arg >= h->long_ref_count || h->long_ref[ h->mmco[i].long_arg ] == 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_arg= long_index;
> +                    h->mmco[i].long_arg= long_arg;

this renaming should be in a seperate patch


[...]
>      }
> +
>      if(h != h0)

cosmetic

[...]
> -    //FIXME do something with unavailable reference frames
[...]
> +        //FIXME do something with unavailable reference frames

cosmetic

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

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- 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/20071003/fda67a90/attachment.pgp>



More information about the ffmpeg-devel mailing list