[FFmpeg-devel] [PATCH] Implement PAFF in H.264
Mon Sep 24 23:21:21 CEST 2007
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
>>> + /*
>>> + * FIXME: Error handling code does not seem to support
>>> + * when slices span multiple rows
>>> + */
>>> + if (!FIELD_PICTURE)
>> 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
> 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.
> There was a comment explaining this near an add_slice call in my previous
> patch. I've moved that here in the current patch.
>>> --- libavcodec/mpegvideo.h (revision 10526)
>>> +++ libavcodec/mpegvideo.h (working copy)
>>> @@ -138,8 +138,9 @@
>>> int field_poc; ///< h264 top/bottom POC
>>> int poc; ///< h264 frame POC
>>> + int valid_structure; ///< h264 one of PICT_XXXX, stating
>>> which fields are referenced
>> if my memory doesnt fail my then we already have this variable, its called
> Yes. From what I've been able to tell by reading the existing code,
> Picture.reference == 1 means keep it around because I need to display it
> still. Picture.reference == 3 means keep it around because it is a
> reference frame.
> This new variable is used to track reference marking of the top and bottom
> fields. Most of the new reference marking code uses this to keep track of
> which field in a pair (or both) is used for reference. I created a new
> variable to avoid modifying the behavior of the reference variable and
> because it was very nice to be able to set it to PICT_XXX values, something
> I couldn't do with the existing variable because PICT_TOP_FIELD == 1 ==
> existing semantic meaning above.
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
> 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?
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
Content-Description: MMCO variable rename patch
Content-Description: Substance of PAFF implementation
ill review this soon
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Size: 189 bytes
Desc: Digital signature
More information about the ffmpeg-devel