Hi,
On Thu, Oct 29, 2015 at 12:45 AM, James Almer <jamr...@gmail.com> wrote:
> On 10/28/2015 11:16 PM, Ronald S. Bultje wrote:
> > Hi,
> >
> > On Wed, Oct 28, 2015 at 5:51 PM, wm4 <nfx...@googlemail.com> wrote:
> >
> >> On Wed, 28 Oct 2015 16:05:49 -0400
> >> "Ronald S. Bultje" <rsbul...@gmail.com> wrote:
> >>
> >>> Hi,
> >>>
> >>> On Wed, Oct 28, 2015 at 3:44 PM, wm4 <nfx...@googlemail.com> wrote:
> >>>
> >>>> On Wed, 28 Oct 2015 12:21:05 -0400
> >>>> "Ronald S. Bultje" <rsbul...@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
>
> Did you forget to update the ref files for fate-vp9-10-show-existing-frame
> and
> fate-vp9-16-intra-only? Because they are failing in a lot of fate clients.
Oops, I will work on it.
Ronald
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
