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

[Michael Niedermayer]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070924/e6216449/attachment.pgp>