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. 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. _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
