Re: [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 
wrote:

> Hi,
>
> On Thu, Oct 29, 2015 at 12:45 AM, James Almer  wrote:
>
>> On 10/28/2015 11:16 PM, Ronald S. Bultje wrote:
>> > Hi,
>> >
>> > On Wed, Oct 28, 2015 at 5:51 PM, wm4  wrote:
>> >
>> >> On Wed, 28 Oct 2015 16:05:49 -0400
>> >> "Ronald S. Bultje"  wrote:
>> >>
>> >>> Hi,
>> >>>
>> >>> On Wed, Oct 28, 2015 at 3:44 PM, wm4  wrote:
>> >>>
>>  On Wed, 28 Oct 2015 12:21:05 -0400
>>  "Ronald S. Bultje"  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.

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

[Ronald S. Bultje]

Hi,

On Thu, Oct 29, 2015 at 12:45 AM, James Almer  wrote:

> On 10/28/2015 11:16 PM, Ronald S. Bultje wrote:
> > Hi,
> >
> > On Wed, Oct 28, 2015 at 5:51 PM, wm4  wrote:
> >
> >> On Wed, 28 Oct 2015 16:05:49 -0400
> >> "Ronald S. Bultje"  wrote:
> >>
> >>> Hi,
> >>>
> >>> On Wed, Oct 28, 2015 at 3:44 PM, wm4  wrote:
> >>>
>  On Wed, 28 Oct 2015 12:21:05 -0400
>  "Ronald S. Bultje"  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 

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

[Ronald S. Bultje]

---
 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;
 }
-- 
2.1.2

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[wm4]

On Wed, 28 Oct 2015 12:21:05 -0400
"Ronald S. Bultje"  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?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Ronald S. Bultje]

Hi,

On Wed, Oct 28, 2015 at 3:44 PM, wm4  wrote:

> On Wed, 28 Oct 2015 12:21:05 -0400
> "Ronald S. Bultje"  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.)

Ronald
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [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  wrote:

> On Wed, 28 Oct 2015 16:05:49 -0400
> "Ronald S. Bultje"  wrote:
>
> > Hi,
> >
> > On Wed, Oct 28, 2015 at 3:44 PM, wm4  wrote:
> >
> > > On Wed, 28 Oct 2015 12:21:05 -0400
> > > "Ronald S. Bultje"  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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [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  wrote:
> 
>> On Wed, 28 Oct 2015 16:05:49 -0400
>> "Ronald S. Bultje"  wrote:
>>
>>> Hi,
>>>
>>> On Wed, Oct 28, 2015 at 3:44 PM, wm4  wrote:
>>>
 On Wed, 28 Oct 2015 12:21:05 -0400
 "Ronald S. Bultje"  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.

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [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"  wrote:

> Hi,
> 
> On Wed, Oct 28, 2015 at 3:44 PM, wm4  wrote:
> 
> > On Wed, 28 Oct 2015 12:21:05 -0400
> > "Ronald S. Bultje"  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.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel