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

[wm4]

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.