[FFmpeg-devel] [PATCH] Implement PAFF in H.264
Mon Sep 24 23:43:05 CEST 2007
On Mon, 24 Sep 2007, Michael Niedermayer wrote:
> 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.
> 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
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
> Content-Description: Cosmetics patch
> > @@ -622,9 +623,9 @@
> > int mpeg_f_code;
> > 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
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.
More information about the ffmpeg-devel