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
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
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[2]; ///< 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 reference
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 ok
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?
-Jeff
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 [...] Content-Description: MMCO variable rename patch ok [...] 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