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