[FFmpeg-devel] [PATCH 1/1] [RFC] ffprobe: report DAR even if SAR is undefined

[Michael Niedermayer]

On Tue, Apr 17, 2018 at 11:16:36AM +0200, Hendrik Leppkes wrote:
> On Tue, Apr 17, 2018 at 11:06 AM, Michael Niedermayer
> <michael at niedermayer.cc> wrote:
> > On Tue, Apr 17, 2018 at 08:32:57AM +0300, Timo Teras wrote:
> >> On Tue, 17 Apr 2018 01:02:43 +0200
> >> Michael Niedermayer <michael at niedermayer.cc> wrote:
> >>
> >> > On Mon, Apr 16, 2018 at 07:56:34PM +0200, Marton Balint wrote:
> >> > >
> >> > > On Mon, 16 Apr 2018, Timo Teras wrote:
> >> > >
> >> > > >On Sun, 15 Apr 2018 16:42:01 +0200 (CEST)
> >> > > >Marton Balint <cus at passwd.hu> wrote:
> >> > > >
> >> > > >>On Sun, 15 Apr 2018, Timo Teräs wrote:
> >> > > >>
> >> > > >>> Calculate DAR with assumed SAR 1:1 when SAR is undefined. Same
> >> > > >>> assumption is done in ffplay to create the play window. Usually
> >> > > >>> DAR is more useful metadata than SAR when e.g. choosing which
> >> > > >>> media of multiple versions to use to fit the display.
> >> > > >>
> >> > > >>I don't think it's good idea to generally assume 1:1
> >> > > >>display_aspect_ratio for every undefined sample aspect ratio. It
> >> > > >>depends heavily on your actual use case. If MOV/MP4 specifies that
> >> > > >>1:1 SAR should be used, then maybe you should fix
> >> > > >>av_guess_sample_aspect_ratio instead, and return 1:1 there if the
> >> > > >>format context is MOV/MP4. You may add a demuxer (AVFMT) flag to
> >> > > >>signal that such behaviour should be used, and
> >> > > >>av_guess_sample_aspect_ratio can check for that demuxer flag.
> >> > > >
> >> > > >Looking at code, av_guess_sample_aspect_ratio() is used only in
> >> > > >ffplay and ffprobe.
> >> > > >
> >> > > >ffplay implicitly assumes undefined SAR is 1:1 to create the
> >> > > >playback window properly; this happens in calculate_display_rect()
> >> > > >when "bad" aspect_ratio is reset to 1.0.
> >> > > >
> >> > > >I would expect same logic would have been useful in ffprobe. This
> >> > > >would help to report back to user what ffplay is going to do with
> >> > > >the video. Or at least give a hint on how to categorize the clip.
> >> > > >SAR 1:1 is pretty good guess for most formats.
> >> > >
> >> > > I really don't see why don't you fix your application instead which
> >> > > parses ffprobe output? If you see N/A aspect ratio, use 1:1.
> >> > >
> >> > > To be frank, I am not sure if ffprobe should use
> >> > > av_guess_aspect_ratio when it displays stream metadata. It is only
> >> > > there now to av_reduce the aspect
> >> >
> >> > > ratios and to sanitize some invalid aspect ratios to 0/1. FFprobe's
> >> > > job is to return stream metadata as is, not to make guesses.
> >> >
> >> > a very minor somewhat on topic nitpick, 0/0 would be mathamtically
> >> > more correct as unknown than 0/1. If one doesnt immedeatly see why,
> >> > one can look at width/height vs height/width to see one of many
> >> > reasons why
> >>
> >> See my earlier patch that changes it to report as "N/A". This is what
> >
> > i meant the function. Which cannot output N/A as it outputs a "rational
> > number" not a string.
> > and for such numbers 0/0 closer represents undefined than 0/1 in a
> > mathematical sense
> >
> 
> Internally we have been using 0/1 for undefined aspect ratios since
> like forever,

> I'm not sure why that was chosen, but maybe to avoid an
> accidental division by zero?

that doesnt work at all

aspects are likely as often multiplied as well as divided by.
so 0 doesnt really avoid the issue

And 0/0 requires very significantly fewer checks, this is why i
suggest it

Just try

1. convert aspect from width/height to height/width
    0/0 -> 0/0 works, 0/1 -> 1/0 fails
    
2. take the average of 2 aspects (as a arbitrary operation, the behavior is similar in other operations)
    (a/b + c/d)/2 ==  (ad + cb) / db2
    if one if 0/0 result is 0/0, if one is 0/1 its nonsense

3. find the height from the aspect and width
    height = width / aspect
    with floats that will give you NaN correctly for a 0/0 aspect.
    with 0/1 it will give infinite
    now compare this to the opposite, finding width from height and aspect
    width = height * aspect
    0/0 will give Nan again
    0/1 will give 0
    its different for each case with 0/1 but behaving the same with 0/0
   
4. compare 2 aspects
    this can be done by looking at the factor between them and how far it is
    from 1.0 or by using the difference comapring to 0.0
    with floats 0/0 will give NaN in all cases
    with floats 0/1 will give 0, infinite and everything in between depending
    on which is 0/1 and what variant is used to compare
    with rationals 0/0 will give 0/0 still undefined as difference in all cases
    with rationals 0/1 will give a similarly wide range of values as floats
    
Thats why i suggest 0/0 as undefined. it behaves much more consistent with
"undefined" in computations. If you put undefined aspect into a computation
you will almost always get the same "0/0" undefined out without any need for
checks. OTOH 0/1 often needs checks and carefully placed ones for the results not
to be just wrong

thanks

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

"You are 36 times more likely to die in a bathtub than at the hands of a
terrorist. Also, you are 2.5 times more likely to become a president and
2 times more likely to become an astronaut, than to die in a terrorist
attack." -- Thoughty2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180417/93c6445d/attachment.sig>