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

[Michael Niedermayer]

Hi

On Thu, Oct 04, 2007 at 03:07:09PM -0400, Jeff Downs wrote:
> On Thu, 4 Oct 2007, Jeff Downs wrote:
> 
> > I'll continue to split as I have time, given how productive this round 
> > was.
> > 
> 
> OK, here is the next round of split patches.  I took these diffs against a 
> tree with the defreflist and reordering patches (posted short time ago) 
> applied, but they will likely also apply clean to current svn.
> 
> Again, in suggested application order with commit message.
> 
> 
> 1. paff-mbaddress.patch
> Modifies macroblock addressing and current macroblock y-position for 
> field decoding. Part of PAFF implementation.
> 
> 2. paff-poc.patch 
> Set Picture.poc for fields and field pairs. Part of PAFF implementation.
> 
> 3. paff-emuedge.patch
> Edge emulation for fields. Part of PAFF implementation
> 
> 4. paff-mmcodecode.patch
> Augment mmcodecoding process to work properly with fields. Part of PAFF 
> implementation
> 
> 5. paff-mmcorefrename.patch
> Cosmetic renaming variable so that it makes more sense for forthcoming 
> PAFF mmco patches.
> 
> 6. paff-mmco.patch
> Augment MMCO execution to work with both fields and frames. Part of PAFF 
> implementation
> 
> 7. paff-mmco-indent.patch
> Re-indent after PAFF MMCO implementation patch.
> 
> 8. paff-picmgmt.patch
> Manage Picture buffers for fields as well as frames. Pair complementary 
> fields into one MPV Picture.  Part of PAFF implementation.
> 
> 
> 
> After these, there are two more -- one to indent after #8 and one to 
> actually enable PAFF handling by properly defining FIELD_PICTURE.
> Since the indent one is quite large, I'll wait until these can be looked 
> at to post the last two.
> 
> 	-Jeff

> Content-Description: Patch 1: mbaddress
[...]
> @@ -3928,6 +3928,11 @@
>      }
>      s->resync_mb_x = s->mb_x = first_mb_in_slice % s->mb_width;
>      s->resync_mb_y = s->mb_y = (first_mb_in_slice / s->mb_width) << h->mb_aff_frame;
> +    if (FIELD_PICTURE) {
> +        s->resync_mb_y = s->mb_y = s->mb_y * 2;
> +        if (s->picture_structure == PICT_BOTTOM_FIELD)
> +            s->resync_mb_y = s->mb_y = s->mb_y + 1;
> +    }
>      assert(s->mb_y < s->mb_height);

i think this breaks the check above this hunk


[...]
> @@ -6666,7 +6673,7 @@
>                  s->mb_x = 0;
>                  ff_draw_horiz_band(s, 16*s->mb_y, 16);
>                  ++s->mb_y;
> -                if(FRAME_MBAFF) {
> +                if(FRAME_MBAFF || FIELD_PICTURE) {

this really should be FRAME_MBAFF_OR_FIELD or something like that


[...]

> Content-Description: Patch 2: poc
[...]
ok


> Content-Description: Patch 3: emuedge
[...]
ok


> Content-Description: Patch 4: MMCO decode
[...]
ok

> Content-Description: Patch 5: MMCOrefrename
[...]
ok


> Content-Description: Patch 6: MMCO
[...]
>          case MMCO_LONG:
> +            j = 1;
> +            if (FIELD_PICTURE && !s->first_field) {
> +                if (h->long_ref[mmco[i].long_arg] == s->current_picture_ptr) {
> +                    /* Just mark second field as referenced */
> +                    j = 0;
> +                } else if (s->current_picture_ptr->reference) {
> +                    /* First field in pair is in short term list or
> +                     * at a different long term index.
> +                     * This is not allowed; see 7.4.3, notes 2 and 3.
> +                     * Report the problem and keep the pair where it is,
> +                     * and mark this field valid.
> +                     */
> +                    av_log(h->s.avctx, AV_LOG_ERROR,
> +                        "illegal long term reference assignment for second "
> +                        "field in complementary field pair (first field is "
> +                        "short term or has non-matching long index)\n");
> +                    j = 0;
> +                }
> +            }
> +
> +            if (j) {

please use a better name than j

[...]
> +        } else {
> +            av_log(h->s.avctx, AV_LOG_ERROR, "problem in internal reference "
> +                                             "list handling; marking second "
> +                                             "field in pair finds first field "
> +                                             "in reference, but not in any "
> +                                             "ref list\n");

if this cannot happen unless our code is buggy te correct behavior is
assert(0);

[...]

> Content-Description: Patch 7: MMCO indent

ok

[...]


> Content-Description: Patch 8: picmgmt

will review later

also i think it would be better if you would post 1 patch per email
instead of many as later can get quite confusing for everyone if 
some are approved some are not and some arent revied yet ...

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
-------------- 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/20071004/aff6d763/attachment.pgp>