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

[James Almer]

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.