Quoting John Stebbins (2015-10-30 16:08:42) > On Fri, 2015-10-30 at 08:59 +0100, Anton Khirnov wrote: > > 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. > > > > What would you do for sources that are *potentially* VFR (i.e. you don't > know without checking every frame)? Every codec that has repeat field > flags is potentially VFR, even when the codec flags all say CFR. When > video uses repeat field flags, it often has sequences where it also > *does not* use repeat field flags. So you end up with a mix of, for > example, 29.97fps and 23.976fps. This is very common with TV transfered > to DVD where the original source material can be a mix of film, video, > or CGI. There is no way to know in advance whether this is going to > happen. So if you want to only allow 100% accurate frame_rate, you > would have to set it to unknown for any source codec that supports > repeat field flags (mpeg12, h.264, vc1, etc). > > I ask this, because this is really what I am talking about when I say > HandBrake generates VFR output by default.
If you don't know for sure, you don't set it. If you want to be sure, you have to do two-pass decoding. -- Anton Khirnov _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
