Hi, On Thu, Oct 29, 2015 at 6:39 AM, Ronald S. Bultje <rsbul...@gmail.com> wrote:
> 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. > Fixed. Ronald _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel