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

[Ronald S. Bultje]

Hi,

On Thu, Oct 29, 2015 at 6:39 AM, Ronald S. Bultje <rsbultje at gmail.com>
wrote:

> Hi,
>
> On Thu, Oct 29, 2015 at 12:45 AM, James Almer <jamrial at 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 <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
>>
>> 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.
>

Fixed.

Ronald