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