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

[Michael Niedermayer]

On Mon, Oct 08, 2007 at 11:50:48AM -0400, Jeff Downs wrote:
> On Mon, 8 Oct 2007, Michael Niedermayer wrote:
> 
> > On Thu, Oct 04, 2007 at 03:07:09PM -0400, Jeff Downs wrote:
> 
> [...]
> 
> > > 8. paff-picmgmt.patch
> > > Manage Picture buffers for fields as well as frames. Pair complementary 
> > > fields into one MPV Picture.  Part of PAFF implementation.
> > > 
> 
> [...]
> 
> > > @@ -3991,8 +3998,50 @@
> > >      }
> > >  
> > >      if(h0->current_slice == 0){
> > > -        if(frame_start(h) < 0)
> > > +        /* See if we have a decoded first field looking for a pair... */
> > > +        if (s0->first_field) {
> > > +            assert(s0->current_picture_ptr);
> > > +            assert(s0->current_picture_ptr->data[0]);
> > > +            assert(s0->current_picture_ptr->reference != DELAYED_PIC_REF);
> > > +
> > > +            /* figure out if we have a complementary field pair */
> > > +            if (!FIELD_PICTURE || s->picture_structure == last_pic_structure) {
> > > +                /*
> > > +                 * Previous field is unmatched. Don't display it, but let it
> > > +                 * remain for reference if marked as such.
> > > +                 */
> > > +                s0->current_picture_ptr = NULL;
> > > +                s0->first_field = FIELD_PICTURE;
> > 
> > first field is set to 0, 1, FIELD_PICTURE and tested against 0/not 0
> > this is inconsistent
> > also it should be here !=0 already
> 
> Hmm not sure I follow your comment.  This block is dealing with any sort 
> of unmatched field pair. That is,
> 
> Had an initial (unmatched as of yet) field.
> Now decoding either a frame picture (!FIELD_PICTURE) or a field of same 
> parity.
> 
> FIELD_PICTURE is 0/1. If desired for clarity, it can easily be changed to: 
> 
> first_field = FIELD_PICTURE ? 1 : 0
> 
> or
> 
> if (!FIELD_PICTURE) 
> first_field = 0

forget my comment ... i have confused FIELD_PICTURE with a constant
but this brings up another issue, i and probably other devels as well
tend to think of upper case names as constants

maybe it would be cleaner if we would use code like
#define FRAME_MBAFF(h) h->mb_aff_frame
instead of the current
#define FRAME_MBAFF h->mb_aff_frame

that of course is seperate from this patch ...
ill retry reviewing it :)

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

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- 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/20071008/3f337420/attachment.pgp>