Quoting John Stebbins (2015-10-29 22:35:24) > On Thu, 2015-10-29 at 21:31 +0100, Anton Khirnov wrote: > > Quoting John Stebbins (2015-10-29 21:17:35) > > > On Thu, 2015-10-29 at 20:12 +0100, Anton Khirnov wrote: > > > > Quoting John Stebbins (2015-10-29 19:59:44) > > > > > On Thu, 2015-10-29 at 19:47 +0100, Anton Khirnov wrote: > > > > > > Quoting John Stebbins (2015-10-28 17:48:17) > > > > > > > From: Nicolas George <[email protected]> > > > > > > > > > > > > > > (cherry picked from ffmpeg commit > > > > > > > 7b42036b3b23c85f473bf9369e37fa8da22eaf93) > > > > > > > --- > > > > > > > libavfilter/avfilter.c | 2 ++ > > > > > > > libavfilter/avfilter.h | 12 ++++++++++++ > > > > > > > 2 files changed, 14 insertions(+) > > > > > > > > > > > > > > diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c > > > > > > > index 64b2645..cd98d16 100644 > > > > > > > --- a/libavfilter/avfilter.c > > > > > > > +++ b/libavfilter/avfilter.c > > > > > > > @@ -195,6 +195,8 @@ int > > > > > > > avfilter_config_links(AVFilterContext > > > > > > > *filter) > > > > > > > link->src->inputs[0] > > > > > > > ->sample_aspect_ratio : > > > > > > > (AVRational){1,1}; > > > > > > > > > > > > > > if (link->src->nb_inputs) { > > > > > > > + if (!link->frame_rate.num && !link > > > > > > > ->frame_rate.den) > > > > > > > + link->frame_rate = link->src > > > > > > > ->inputs[0] > > > > > > > ->frame_rate; > > > > > > > if (!link->w) > > > > > > > link->w = link->src->inputs[0]->w; > > > > > > > if (!link->h) > > > > > > > diff --git a/libavfilter/avfilter.h b/libavfilter/avfilter.h > > > > > > > index 9dbfeea..b7b97ce 100644 > > > > > > > --- a/libavfilter/avfilter.h > > > > > > > +++ b/libavfilter/avfilter.h > > > > > > > @@ -375,6 +375,18 @@ struct AVFilterLink { > > > > > > > AVLINK_STARTINIT, ///< started, but > > > > > > > incomplete > > > > > > > AVLINK_INIT ///< complete > > > > > > > } init_state; > > > > > > > + > > > > > > > + /** > > > > > > > + * Frame rate of the stream on the link, or 1/0 if > > > > > > > unknown; > > > > > > > + * if left to 0/0, will be automatically be copied from > > > > > > > the > > > > > > > first input > > > > > > > + * of the source filter if it exists. > > > > > > > + * > > > > > > > + * Sources should set it to the best estimation of the > > > > > > > real > > > > > > > frame rate. > > > > > > > + * Filters should update it if necessary depending on > > > > > > > their > > > > > > > function. > > > > > > > + * Sinks can use it to set a default output frame rate. > > > > > > > + * It is similar to the avg_frame_rate field in > > > > > > > AVStream. > > > > > > > + */ > > > > > > > > > > > > Please no! No guesses or estimates, we already have plenty of > > > > > > this > > > > > > crap > > > > > > in lavf and we want to get rid of it, not add more. VFR > > > > > > streams > > > > > > with > > > > > > no > > > > > > such thing as "real frame rate" exist. > > > > > > > > > > > > So IMO this should be: > > > > > > - if AND ONLY if the stream is known to be of some specific > > > > > > constant > > > > > > framerate, this field should be set to that number > > > > > > - in ALL OTHER CASES, this field should be set to 0/0 (or > > > > > > whatever > > > > > > other > > > > > > invalid value) and treated as VFR. > > > > > > > > > > An actual framerate has to be written at the muxing stage by a > > > > > transcoder such as HandBrake. We can't know how the filter > > > > > chain > > > > > will > > > > > affect our nominal framerate without passing the nominal value > > > > > to > > > > > the > > > > > input of the filter chain and seeing what comes out the other > > > > > side. > > > > > So > > > > > although nominal framerates are a PITA, they are a requirement > > > > > we > > > > > can't > > > > > ignore. > > > > > > > > > > Although HandBrake generates output that is technically variable > > > > > framerate by default (i.e. vfr flags are set in h.264 headers), > > > > > it > > > > > is > > > > > often the case that the measured framerate is constant > > > > > throughout > > > > > the > > > > > stream. There is just no way to know this in advance with > > > > > certain > > > > > source types (e.g. DVD). > > > > > > > > > > We could document this as you prefer as a means of discouraging > > > > > this > > > > > kind of usage. But this is a real world case that is going to > > > > > happen. > > > > > > > > If you really need some kind of a framerate, you could e.g. pass > > > > two > > > > dummy frames through the filtergraph and check the pts difference > > > > on > > > > the > > > > output. Would that be a lot more work for you? > > > > > > > > I recall you complaining about the lavf timestamps mess yourself. > > > > And > > > > one of the main reasons it is so horrible is precisely that it > > > > somehow > > > > makes up some guesses or estimates and then yet more guesses on > > > > top of > > > > that until we have no clue what are real data and what is made up. > > > > I > > > > would really like to avoid that in any new code, if reasonably > > > > possible. > > > > > > > > > > In theory, HandBrake could calculate the average framerate over the > > > entire video (or keep track of the framerate of the majority of the > > > video) and update muxer header information before finalizing the > > > file. > > > Headers for many formats are not actually written till muxing is > > > complete anyway. But this would rarely result in a different value > > > than > > > what we estimate up front. We initially sample the source at > > > various > > > locations to collect statistics about several properties of the > > > source > > > prior to transcoding. So our estimates are pretty accurate. > > > Probably > > > more accurate that running the first few frames through the filter > > > chain. > > > > Well, that's an estimate of the _input_ framerate, while the point > > here > > is getting the _output_ one, no? > > > > Given what filters like setpts can do, I'm going to have to concede that > input framerate can be meaningless in the general case. I hadn't really > looked closely at what this filter does. I hadn't considered that some > filter in the middle of the filter chain could generate a completely > unknown framerate. HandBrake doesn't make use of such filters, but I > should probably be planning for the day that it does.
It's not really limited to the setpts filter. Some webcams or streaming sources will also give you truly VFR video. > The only way to > handle the output of such a filter is going to be to measure the > framerate at the output and delay writing the muxer headers till > sufficient frames have been processed to get a reliable framerate. This > is probably not a bad idea. Doing so however completely eliminates my > original need for this data at the output of the filter chain. If I > can't reliably retrieve some nominal framerate value from the output of > the filter chain, it might as will not be there and this patch series is > pointless for me. You could argue that it could be useful in a use case > where it is known that all video will be constant framerate. But such a > use case would never be able to use filters such as setpts, even when > setpts may be configured to generate constant framerate output. So even > for this use case, this new field is of limited value. > > So now I am wondering if this patch series should just be trashed all > together. Because providing the frame_rate in this way is going to > provide a crutch for people to take shortcuts that only work for a > subset of use cases. And they may not realize under which circumstances > it will fail to provide what they need. I still believe it is useful in certain cases. Yes, it won't give you a number in all possible situations. But it will tell you when you can be absolutely sure that your stream is of a given constant framerate. And I think having a reliable number only sometimes is still more useful than always having something that you can't rely on. -- Anton Khirnov _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
