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

[Jeff Downs]

On Mon, 24 Sep 2007, Michael Niedermayer wrote:

> Hi
> 
> On Thu, Sep 20, 2007 at 03:04:50PM -0400, Jeff Downs wrote:
> > On Wed, 19 Sep 2007, Michael Niedermayer wrote:
> >
> >> just a quick review below, ill wait with a full review until there are
> >> clean and well split patches ...
> >
> > Thanks.  I knew it had to be split and I'm happy to be receiving guidance 
> > as to where.
> >
> > I've attached 3 patches, which are intended to be examined/applied in 
> > order:
> [...]
> >>
> >> [...]
> >>
> >>> +            /*
> >>> +             * 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?
> >
> > The error concealer doesn't seem to have any notion of being able to skip 
> > rows, both when marking errors and doing the concealing calculations. Calls 
> 
> skipping rows should be a matter of setting *stride correctly / storing
> things with the stride which is stored in the context

Agree.  I'll look at going that route.

> > to ff_er_add_slice end result in error_count going through the roof for 
> > perfectly ok frames. The frame_end call then falsely corrects the errors.  
> > It was easiest and arguably more efficient to abort the frame_end call 
> > instead of all the different add_slice calls.
> >
> > I would be willing to work to fix this after we get through this already 
> > behemoth patch, at which time this could be removed.
> 
> ok

Thanks.


> reference was intended to keep track
> of which fields are still needed as reference fields/frames, here 3 means
> both that is 1|2
> any code doing something else with it is wrong and has to be changed
> duplicating it just because the code using it is buggy is absolutely not
> ok

Understand and agree.  Are you then proposing that I table the PAFF 
implementation pending a (separate) patch to fix this? I want to get the 
desired order right here...

> > I'm open to suggestions on changing this if needed;  if the replacement 
> > doesn't have it taking on values of PICT_XXX, then it could be a 
> > substantial rework.
> 
> reference was intended to have the same values as PICT_XXX, i as well see
> that something went wrong with h264.c in that respect though ...
> 
> maybe changing the current 1/"keep it around because I need to display it"
> to 4 to would solve the problem?

Sure. Will look at common code in mpegvideo.c to see if that causes issues 
there.

> Content-Description: Cosmetics patch
> [...]
> > @@ -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)
> 
> iam against this hunk it doesnt do any good, the rest of patch 1 is
> ok though

That's fine -- I put that there only to stress the relationship of the 
three is important and not just arbitrary number choices. I will remove.

Please hold to apply - I have a slightly modified version I was about to 
post when your review came through which reflects the threads of 
discussion with Martin Z. re pic_num/pic_id.

> [...]
> 
> Content-Description: MMCO variable rename patch
> 
> ok

Again, please hold application pending an additional documentation item 
for MMCO->short_pic_num, posted shortly in updated patch.

> Content-Description: Substance of PAFF implementation
> [...]
> 
> ill review this soon

Likewise, please hold off for a short time while I post a new version 
reflecting some of Martin Z.'s contributions.

	-Jeff