On Mon, May 15, 2023 at 10:44:56PM +0200, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2023-05-15 20:59:42)
> > On Tue, May 09, 2023 at 10:44:50AM +0200, Anton Khirnov wrote:
> > > Quoting Michael Niedermayer (2023-05-08 16:15:42)
> > > > On Sun, May 07, 2023 at 03:32:46PM +0200, Anton Khirnov wrote:
> > > > > H.264 and mpeg12 parsers need to be adjusted at the same time to stop
> > > > > using the value of AVCodecContext.ticks_per_frame, because it is not set
> > > > > correctly unless the codec has been opened. Previously this would result
> > > > > in both the parser and lavf seeing the same incorrect value, which would
> > > > > cancel out.
> > > > > Updating lavf and not the parsers would result in correct value in lavf,
> > > > > but the wrong one in parsers, which would break some tests.
> > > > > ---
> > > > >  libavcodec/h264_parser.c      |  4 ++--
> > > > >  libavcodec/mpegvideo_parser.c |  2 +-
> > > > >  libavformat/avformat.c        |  9 ++++++---
> > > > >  libavformat/demux.c           | 29 +++++++++++++++++++----------
> > > > >  libavformat/internal.h        |  3 +++
> > > > >  5 files changed, 31 insertions(+), 16 deletions(-)
> > > > 
> > > > Doesnt this sort of change need a major ABI bump ?
> > > > it sounds like lavc and lavf interdepend here both ways
> > > 
> > > No, we do not guarantee bug compatibility.
> > > 
> > > Libavformat seeing ticks_per_frame=1 for codecs that set it to 2 upon
> > > being opened is a bug. Same for the parser.
> > > 
> > > It just so happens that libavformat AND its internal parser instance see
> > > the same incorrect value and this cancels out in cases that are tested
> > > by FATE (it would break if we had more thorough tests for repeating
> > > single fields).
> > 
> > This patch seems to change tbr
> > ./ffmpeg -i fate-suite//h264/lossless.h264
> > Stream #0:0: Video: h264 (High 4:4:4 Predictive), yuv420p(progressive), 640x480, 25 fps, 60 tbr, 1200k tbn
> > vs.
> > Stream #0:0: Video: h264 (High 4:4:4 Predictive), yuv420p(progressive), 640x480, 25 fps, 120 tbr, 1200k tbn
> > 
> > with 
> > ./ffmpeg -i fate-suite//h264/lossless.h264  -f framecrc -
> > 
> > The output uses 1/60 thats odd because if every frame can be represented in
> > 1/60 then tbr is 1/60 or more course
> > OTOH if tbr is finer than 1/60 then not every frame can be represented in 1/60
> > 
> > maybe iam missing something but the new value seems worse and also
> > not consistent with what ffmpeg actually uses
> 
> ticks_per_frame was added by you in 3797c74ba53, and according to your
> code it's supposed to be 2 for H.264. It just so happens that for this
> specific sample libavformat invokes the parser without opening the
> decoder, so it sees the default value of 1. If it did open the decoder,
> it would see 2. This patch at least makes it consistent, even if it
> might not always be the optimal choice.

Iam not sure how it is consistent if the value used is different than the
value displayed


> 
> As far as I'm concerned, the entire notion of 'tbr' is fundamentally
> flawed and should be abandoned. There is no magical way for the code to
> know what timebase is truly the right one here, without reading the
> whole file.
> 
> Furthermore, the entire approach of "some sample X is now getting
> slightly worse arbitrary numbers than before" seems highly questionable
> to me. Our timestamps code is a unholy mess of hacks upon hacks upon
> hacks. For pretty much ANY change one can find or construct a sample
> that decodes worse after it. We should stop focusing on individual
> samples and prioritize overall consistency and correctness.

I think the important part is provide the user with what (s)he wants
If more files work better, thats a win.
The world of multimedia is a bit messy in some corners (as you know)
so i am not sure if true beauty, cleanliness and consistency can be
achieved while having well working/fast code
But i certainly support making the code nicer.
about "correctness", iam not even sure what exactly is "correct"
in some cases.
just the recent hls case, the first 4 links i tried used 2 mime types
the rfc would consider wrong. id say adding them is "correct" with
the "SHOULD" recommandition but others surely will disagree
about tbr, i think its a usefull field, It wont be the global optimal
value for some videos but neither would width and height be, if they
change midstream.
But any improvment is good and i support that, in this case here i
saw one file change and i reported it.

Thanks

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I am the wisest man alive, for I know one thing, and that is that I know
nothing. -- Socrates