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

Jeff Downs heydowns
Fri Oct 5 20:23:48 CEST 2007


On Thu, 4 Oct 2007, Michael Niedermayer wrote:

[...]

> > Content-Description: Patch 6: MMCO
> [...]
> >          case MMCO_LONG:
> > +            j = 1;
> > +            if (FIELD_PICTURE && !s->first_field) {
> > +                if (h->long_ref[mmco[i].long_arg] == s->current_picture_ptr) {
> > +                    /* Just mark second field as referenced */
> > +                    j = 0;
> > +                } else if (s->current_picture_ptr->reference) {
> > +                    /* First field in pair is in short term list or
> > +                     * at a different long term index.
> > +                     * This is not allowed; see 7.4.3, notes 2 and 3.
> > +                     * Report the problem and keep the pair where it is,
> > +                     * and mark this field valid.
> > +                     */
> > +                    av_log(h->s.avctx, AV_LOG_ERROR,
> > +                        "illegal long term reference assignment for second "
> > +                        "field in complementary field pair (first field is "
> > +                        "short term or has non-matching long index)\n");
> > +                    j = 0;
> > +                }
> > +            }
> > +
> > +            if (j) {
> 
> please use a better name than j

Used j because it was already defined and unused in this context. Added 
new var named unref_pic.

 
> [...]
> > +        } else {
> > +            av_log(h->s.avctx, AV_LOG_ERROR, "problem in internal reference "
> > +                                             "list handling; marking second "
> > +                                             "field in pair finds first field "
> > +                                             "in reference, but not in any "
> > +                                             "ref list\n");
> 
> if this cannot happen unless our code is buggy te correct behavior is
> assert(0);

OK. Added the assert, changed the log message to a comment, removed the 
reference clearing. The block will be a no-op when NDEBUG is defined.

Revised patch attached.

If ok, I'll follow up with revised indent patch to match prior to 
application in svn.

	-Jeff
-------------- next part --------------
--- ../ffmpeg-mmcorefrename/libavcodec/h264.c	2007-10-04 14:30:07.000000000 -0400
+++ libavcodec/h264.c	2007-10-05 14:16:20.000000000 -0400
@@ -3465,42 +3465,80 @@
         av_log(h->s.avctx, AV_LOG_DEBUG, "no mmco here\n");
 
     for(i=0; i<mmco_count; i++){
+        int structure, frame_num, unref_pic;
         if(s->avctx->debug&FF_DEBUG_MMCO)
             av_log(h->s.avctx, AV_LOG_DEBUG, "mmco:%d %d %d\n", h->mmco[i].opcode, h->mmco[i].short_pic_num, h->mmco[i].long_arg);
 
         switch(mmco[i].opcode){
         case MMCO_SHORT2UNUSED:
-            pic= remove_short(h, mmco[i].short_pic_num);
-            if(pic)
-                unreference_pic(h, pic, 0);
-            else if(s->avctx->debug&FF_DEBUG_MMCO)
-                av_log(h->s.avctx, AV_LOG_DEBUG, "mmco: remove_short() failure\n");
+            if(s->avctx->debug&FF_DEBUG_MMCO)
+                av_log(h->s.avctx, AV_LOG_DEBUG, "mmco: unref short %d count %d\n", h->mmco[i].short_pic_num, h->short_ref_count);
+            frame_num = pic_num_extract(h, mmco[i].short_pic_num, &structure);
+            pic = find_short(h, frame_num, &j);
+            if (pic) {
+                if (unreference_pic(h, pic, structure ^ PICT_FRAME))
+                    remove_short_at_index(h, j);
+            } else if(s->avctx->debug&FF_DEBUG_MMCO)
+                av_log(h->s.avctx, AV_LOG_DEBUG, "mmco: unref short failure\n");
             break;
         case MMCO_SHORT2LONG:
-            pic= remove_long(h, mmco[i].long_arg);
-            if(pic) unreference_pic(h, pic, 0);
+            if (FIELD_PICTURE && mmco[i].long_arg < h->long_ref_count &&
+                    h->long_ref[mmco[i].long_arg]->frame_num ==
+                                              mmco[i].short_pic_num / 2) {
+                /* do nothing, we've already moved this field pair. */
+            } else {
+                int frame_num = mmco[i].short_pic_num >> FIELD_PICTURE;
+
+                pic= remove_long(h, mmco[i].long_arg);
+                if(pic) unreference_pic(h, pic, 0);
 
-            h->long_ref[ mmco[i].long_arg ]= remove_short(h, mmco[i].short_pic_num);
+                h->long_ref[ mmco[i].long_arg ]= remove_short(h, frame_num);
             if (h->long_ref[ mmco[i].long_arg ]){
                 h->long_ref[ mmco[i].long_arg ]->long_ref=1;
                 h->long_ref_count++;
             }
+            }
             break;
         case MMCO_LONG2UNUSED:
-            pic= remove_long(h, mmco[i].long_arg);
-            if(pic)
-                unreference_pic(h, pic, 0);
-            else if(s->avctx->debug&FF_DEBUG_MMCO)
-                av_log(h->s.avctx, AV_LOG_DEBUG, "mmco: remove_long() failure\n");
+            j = pic_num_extract(h, mmco[i].long_arg, &structure);
+            pic = h->long_ref[j];
+            if (pic) {
+                if (unreference_pic(h, pic, structure ^ PICT_FRAME))
+                    remove_long_at_index(h, j);
+            } else if(s->avctx->debug&FF_DEBUG_MMCO)
+                av_log(h->s.avctx, AV_LOG_DEBUG, "mmco: unref long failure\n");
             break;
         case MMCO_LONG:
+            unref_pic = 1;
+            if (FIELD_PICTURE && !s->first_field) {
+                if (h->long_ref[mmco[i].long_arg] == s->current_picture_ptr) {
+                    /* Just mark second field as referenced */
+                    unref_pic = 0;
+                } else if (s->current_picture_ptr->reference) {
+                    /* First field in pair is in short term list or
+                     * at a different long term index.
+                     * This is not allowed; see 7.4.3, notes 2 and 3.
+                     * Report the problem and keep the pair where it is,
+                     * and mark this field valid.
+                     */
+                    av_log(h->s.avctx, AV_LOG_ERROR,
+                        "illegal long term reference assignment for second "
+                        "field in complementary field pair (first field is "
+                        "short term or has non-matching long index)\n");
+                    unref_pic = 0;
+                }
+            }
+
+            if (unref_pic) {
             pic= remove_long(h, mmco[i].long_arg);
             if(pic) unreference_pic(h, pic, 0);
 
             h->long_ref[ mmco[i].long_arg ]= s->current_picture_ptr;
             h->long_ref[ mmco[i].long_arg ]->long_ref=1;
             h->long_ref_count++;
+            }
 
+            s->current_picture_ptr->reference |= s->picture_structure;
             current_ref_assigned=1;
             break;
         case MMCO_SET_MAX_LONG:
@@ -3525,6 +3563,34 @@
         }
     }
 
+    if (!current_ref_assigned && FIELD_PICTURE &&
+            !s->first_field && s->current_picture_ptr->reference) {
+
+        /* Second field of complementary field pair; the first field of
+         * which is already referenced. If short referenced, it
+         * should be first entry in short_ref. If not, it must exist
+         * in long_ref; trying to put it on the short list here is an
+         * error in the encoded bit stream (ref: 7.4.3, NOTE 2 and 3).
+         */
+        if (h->short_ref_count && h->short_ref[0] == s->current_picture_ptr) {
+            /* Just mark the second field valid */
+            s->current_picture_ptr->reference = PICT_FRAME;
+        } else if (s->current_picture_ptr->long_ref) {
+            av_log(h->s.avctx, AV_LOG_ERROR, "illegal short term reference "
+                                             "assignment for second field "
+                                             "in complementary field pair "
+                                             "(first field is long term)\n");
+        } else {
+            /*
+             * First field in reference, but not in any sensible place on our
+             * reference lists. This shouldn't happen unless reference
+             * handling somewhere else is wrong.
+             */
+            assert(0);
+        }
+        current_ref_assigned = 1;
+    }
+
     if(!current_ref_assigned){
         pic= remove_short(h, s->current_picture_ptr->frame_num);
         if(pic){
@@ -3538,6 +3604,7 @@
         h->short_ref[0]= s->current_picture_ptr;
         h->short_ref[0]->long_ref=0;
         h->short_ref_count++;
+        s->current_picture_ptr->reference |= s->picture_structure;
     }
 
     print_short_term(h);



More information about the ffmpeg-devel mailing list