[FFmpeg-devel] [PATCH] vp9_parser: don't overwrite cached timestamps with nopts.

[Ronald S. Bultje]

Hi,

On Wed, Oct 28, 2015 at 5:51 PM, wm4 <nfxjfg at googlemail.com> wrote:

> On Wed, 28 Oct 2015 16:05:49 -0400
> "Ronald S. Bultje" <rsbultje at gmail.com> wrote:
>
> > Hi,
> >
> > On Wed, Oct 28, 2015 at 3:44 PM, wm4 <nfxjfg at googlemail.com> wrote:
> >
> > > On Wed, 28 Oct 2015 12:21:05 -0400
> > > "Ronald S. Bultje" <rsbultje at gmail.com> wrote:
> > >
> > > > ---
> > > >  libavcodec/vp9_parser.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/libavcodec/vp9_parser.c b/libavcodec/vp9_parser.c
> > > > index 0437097..6713850 100644
> > > > --- a/libavcodec/vp9_parser.c
> > > > +++ b/libavcodec/vp9_parser.c
> > > > @@ -64,7 +64,7 @@ static int parse_frame(AVCodecParserContext *ctx,
> > > const uint8_t *buf, int size)
> > > >          if (ctx->pts == AV_NOPTS_VALUE)
> > > >              ctx->pts = s->pts;
> > > >          s->pts = AV_NOPTS_VALUE;
> > > > -    } else {
> > > > +    } else if (ctx->pts != AV_NOPTS_VALUE) {
> > > >          s->pts = ctx->pts;
> > > >          ctx->pts = AV_NOPTS_VALUE;
> > > >      }
> > >
> > > I find this a bit questionable. What is it needed for? Wouldn't it
> > > repeat the previous timestamp if new packets have none?
> >
> >
> > I don't think so. Let's first go through the problem that I'm seeing,
> maybe
> > you see alternate solutions. Then I'll explain (very end) why I don't
> think
> > this happens.
> >
> > So, we're assuming here (this is generally true) that all input to the
> > parser has timestamps (coming from ivf/webm), and that some frames are
> > "superframes" which have one invisible (alt-ref) frame (only a reference,
> > not an actual frame that's ever displayed) packed with the following
> > visible frame. So each packet in ivf/webm leads to one visible output
> > frame, but in some cases this requires decoding of multiple (invisible)
> > references. For frame threading purposes, we split before we send it to
> the
> > decoder.
> >
> > So what you get is:
> > - ivf/webm file has packet of size a with timestamp b, calls parser
> > - parser notices that packet is superframe with 2 frames in it
> > - we output the first (invisible) frame with no timestamp, and cache the
> > timestamp of the input packet
> > - utils.c code calls parser again with the same input data (we told it we
> > didn't consume any), but the (input) timestamp is now set to nopts
> > - we output the second (visible) frame with the cached timestamp on the
> > packet
> >
> > This generally makes sense, the webm/ivf indeed assume that the timestamp
> > only is valid for the visible frame. Invisible frames have no timestamp
> > associated with them since they're never displayed.
> >
> > So, the above code has the issue: what if there's 2 invisible frames
> packed
> > in a superframe (followed by the visible frame)? Right now, this happens:
> > - ivf/webm file has packet of size a with timestamp b, calls parser
> > - parser notices that packet is superframe with 3 frames in it
> > - we output the first (invisible) frame with no timestamp, and cache the
> > timestamp of the input packet
> > - utils.c code calls parser again with the same input data (we told it we
> > didn't consume any), but the (input) timestamp is now set to nopts
> > - we output the second (invisible) frame with no timestamp, and cache the
> > timestamp of the input packet (which is now set to nopts)
> > - utils.c code calls parser again with the same input data (we told it we
> > didn't consume any), but the (input) timestamp is now set to nopts
> > - we output the third (visible) frame with the cached timestamp on the
> > packet, which was nopts
> >
> > The last 3 are wrong; we should only cache timestamps if there is any to
> be
> > cached, we should not override the cached timestamp with a new nopts
> value,
> > at least not in this particular case.
> >
> > --
> > very end
> > --
> >
> > Ok, so what about your point: can we output the same timestamp twice? I
> > don't think so, because as soon as we output the cached timestamp, we
> reset
> > the value of the cached timestamp to nopts, so if we were to reuse the
> > cached timestamp, it would be nopts and the output data would have no
> > timestamp.
> >
> > (Hope that wasn't too detailed.)
>
> Thanks for the explanations. I didn't know there could be more than 1
> super frame, and I see how the new logic works and doesn't duplicate
> timestamps.
>
> Patch looks good with me then.


TY, pushed.

Ronald