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

[Jeff Downs]

On Fri, 25 Jul 2008, Michael Niedermayer wrote:

> On Thu, Sep 20, 2007 at 03:04:50PM -0400, Jeff Downs wrote:
> > On Wed, 19 Sep 2007, Michael Niedermayer wrote:
> [...]
> >          case MMCO_LONG:
> > -            pic= remove_long(h, mmco[i].long_arg);
> > -            if(pic) unreference_pic(h, pic);
> > +            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->valid_structure) {
> > +                    /* First field in pair in short term list.
> > +                     * This is not allowed; see 7.4.3, notes 2 and 3.
> > +                     */
> > +                    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)\n");
> > +                    j = 0;
> [...]
> > -    if(!current_is_long){
> > -        pic= remove_short(h, s->current_picture_ptr->frame_num);
> > +    if (!current_ref_assigned && FIELD_PICTURE &&
> > +            !s->first_field && s->current_picture_ptr->valid_structure) {
> > +
> > +        /* Second field of complementary field pair; the first field of
> > +         * which is already referenced. If short referenced, it
> > +         * should be first entry in short_ref. If not, it must exist
> > +         * in long_ref; trying to put it on the short list here is an
> > +         * error in the encoded bit stream (ref: 7.4.3, NOTE 2 and 3).
> > +         * If on neither list, we have a logic problem elsewhere
> > +         */
> 
> the notes 2 & 3 in 7.4.3 do not say anything that would support that view.

Yes. Apologies - the citation as you noticed should be 7.4.3.3.
I've changed this.

> the notes 2 & 3 in 7.4.3.3 could be bent toward this given the assumtation
> that the stream does not contain non paired reference fields.
> What made you think such fields do not occur? No i dont have a stream that
> i know contains such things.
> iam just curious if theres something in the spec that supports this
> view.

The first hunk quoted up there is wrong.  I misread spec to be that 
you cannot have second field in a complementary pair be marked long unless 
the first had already been marked long.  That doesn't hold here.  The only 
restriction is that if the first was made long, then the second must be 
too.
It looks like that comment is the only thing surviving there, and can 
probably be removed if you agree.

Second hunk...  Both ways to assign long term to a field (IDR long term 
ref flag, MMCO current is long, and MMCO short to long) specify that if 
applied to the first, you have to apply the same to the second field in a 
pair.

Unpaired fields aren't considered by that comment/code (first_field == 1 
for unpaired fields); the restriction doesn't apply.

All that said, maybe its better to just remove this check and put the 
field on the short list anyway.  I was unsure of how to handle this case 
since it shouldn't happen.

In light of the comments on the first hunk, I do not know if the code 
properly handles field pairs on mixed lists or not. I am still catching up 
on the recent work that's been done.

	-Jeff