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

Submitted by Timo Teräs on April 15, 2018, 11:57 a.m.

Details

Message ID 20180415115706.17172-1-timo.teras@iki.fi
State New
Headers show

Commit Message

Timo Teräs April 15, 2018, 11:57 a.m.
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.

Normally undefined SAR means 1:1. E.g. in mov/mp4 files there's
'pasp' atom to explicitly define SAR. If that is not specified,
the video codec's SAR should be used. Should it be also undefined,
the SAR should be assumed to be 1:1. It makes sense to not change
SAR in the demux info, so ffmpeg can make copies with matching
atom structure and codec extra data. So the simplest way to
report DAR is assume SAR 1:1 when undefined.

Signed-off-by: Timo Teräs <timo.teras@iki.fi>
---
This applies on top of the previous DAR/SAR reporting fix:
https://ffmpeg.org/pipermail/ffmpeg-devel/2018-April/228141.html

For more details behind this patch, see:
https://ffmpeg.org/pipermail/ffmpeg-devel/2018-April/228146.html

This seemed to be the most simple and logical fix. Alternatively,
we could add a new 'effective_display_aspect_ratio' or add a flag
to enable this assumption if the existing functionality should be
kept unchanged. I felt this would be the most sensible thing to do.
I am happy this if there's a more desirable approach to the issue.

Hopefully this and the DAR/SAR reporting fix can be applied before
next release tag. Thanks for considering!

 fftools/ffprobe.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Marton Balint April 15, 2018, 2:42 p.m.
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.
>
> Normally undefined SAR means 1:1. E.g. in mov/mp4 files there's
> 'pasp' atom to explicitly define SAR. If that is not specified,
> the video codec's SAR should be used. Should it be also undefined,
> the SAR should be assumed to be 1:1. It makes sense to not change
> SAR in the demux info, so ffmpeg can make copies with matching
> atom structure and codec extra data. So the simplest way to
> report DAR is assume SAR 1:1 when undefined.
>
> Signed-off-by: Timo Teräs <timo.teras@iki.fi>
> ---
> This applies on top of the previous DAR/SAR reporting fix:
> https://ffmpeg.org/pipermail/ffmpeg-devel/2018-April/228141.html
>
> For more details behind this patch, see:
> https://ffmpeg.org/pipermail/ffmpeg-devel/2018-April/228146.html
>
> This seemed to be the most simple and logical fix. Alternatively,
> we could add a new 'effective_display_aspect_ratio' or add a flag
> to enable this assumption if the existing functionality should be
> kept unchanged. I felt this would be the most sensible thing to do.
> I am happy this if there's a more desirable approach to the issue.
>
> Hopefully this and the DAR/SAR reporting fix can be applied before
> next release tag. Thanks for considering!

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.

Regards,
Marton
Carl Eugen Hoyos April 15, 2018, 10:07 p.m.
2018-04-15 16:42 GMT+02:00, Marton Balint <cus@passwd.hu>:
>
>
> 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.
>>
>> Normally undefined SAR means 1:1. E.g. in mov/mp4 files there's
>> 'pasp' atom to explicitly define SAR. If that is not specified,
>> the video codec's SAR should be used. Should it be also undefined,
>> the SAR should be assumed to be 1:1. It makes sense to not change
>> SAR in the demux info, so ffmpeg can make copies with matching
>> atom structure and codec extra data. So the simplest way to
>> report DAR is assume SAR 1:1 when undefined.
>>
>> Signed-off-by: Timo Teräs <timo.teras@iki.fi>
>> ---
>> This applies on top of the previous DAR/SAR reporting fix:
>> https://ffmpeg.org/pipermail/ffmpeg-devel/2018-April/228141.html
>>
>> For more details behind this patch, see:
>> https://ffmpeg.org/pipermail/ffmpeg-devel/2018-April/228146.html
>>
>> This seemed to be the most simple and logical fix. Alternatively,
>> we could add a new 'effective_display_aspect_ratio' or add a flag
>> to enable this assumption if the existing functionality should be
>> kept unchanged. I felt this would be the most sensible thing to do.
>> I am happy this if there's a more desirable approach to the issue.
>>
>> Hopefully this and the DAR/SAR reporting fix can be applied before
>> next release tag. Thanks for considering!
>
> I don't think it's good idea to generally assume 1:1 display_aspect_ratio
> for every undefined sample aspect ratio.

I don't think that this is what the patch does (and it would definitely
be wrong and not what FFplay does), I don't know if the patch is a
good idea though.

Carl Eugen
Timo Teräs April 16, 2018, 6:06 a.m.
On Sun, 15 Apr 2018 16:42:01 +0200 (CEST)
Marton Balint <cus@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.

For this reason, my preferred solution was to patch ffprobe so we can
give a guess for all files. If the above patch is not a good idea, how
about adding new "effective_{sample,display}_aspect_ratio" fields? Or
just a flag "aspect_ratio_guessed" to tell if it's not defined in the
file?

I would prefer not to do any file type specific special handling if
possible. However, if that's the only acceptable solution, I'm happy to
implement that too. But then I'd prefer to have a 'no default SAR of
1:1' flags so file formats can inhibit the assumption instead of
explicitly needing to enable it. Is there any formats where this would
be useful? Or how about just making av_guess_sample_aspect_ratio()
return 1:1 in case nothing better exists? It's called "guess" after all
and is used in ffplay/ffprobe only...

Thanks
Timo
Michael Niedermayer April 16, 2018, 5:40 p.m.
On Mon, Apr 16, 2018 at 09:06:55AM +0300, Timo Teras wrote:
> On Sun, 15 Apr 2018 16:42:01 +0200 (CEST)
> Marton Balint <cus@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.
> 
> For this reason, my preferred solution was to patch ffprobe so we can
> give a guess for all files. If the above patch is not a good idea, how
> about adding new "effective_{sample,display}_aspect_ratio" fields? Or
> just a flag "aspect_ratio_guessed" to tell if it's not defined in the
> file?
> 
> I would prefer not to do any file type specific special handling if
> possible. However, if that's the only acceptable solution, I'm happy to
> implement that too. But then I'd prefer to have a 'no default SAR of
> 1:1' flags so file formats can inhibit the assumption instead of
> explicitly needing to enable it. Is there any formats where this would
> be useful? Or how about just making av_guess_sample_aspect_ratio()
> return 1:1 in case nothing better exists? It's called "guess" after all
> and is used in ffplay/ffprobe only...

unknown is not the same as 1:1
the user (of av_guess_sample_aspect_ratio) may want to know if theres
a 1:1 actually stored in some place or just nothing.

applications like ffplay cannot use "unknown" so they have to pick 1:1
but why is "unknown" a problem for ffprobe if thats how it is ?


[...]
Marton Balint April 16, 2018, 5:56 p.m.
On Mon, 16 Apr 2018, Timo Teras wrote:

> On Sun, 15 Apr 2018 16:42:01 +0200 (CEST)
> Marton Balint <cus@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.

>
> For this reason, my preferred solution was to patch ffprobe so we can
> give a guess for all files. If the above patch is not a good idea, how
> about adding new "effective_{sample,display}_aspect_ratio" fields? Or
> just a flag "aspect_ratio_guessed" to tell if it's not defined in the
> file?

Effective is not a good name here. Use best_effort_* or 
guessed_* if you have to, but I don't see the much benefit in adding extra 
fields to ffprobe output for such simple and trivial heuristics. Like I 
said, it's not ffprobe's job. The next guy will want to add a 
guessed_color_primaries based on resolution, and I can think of more 
complicated guesses :)

>
> I would prefer not to do any file type specific special handling if
> possible. However, if that's the only acceptable solution, I'm happy to
> implement that too. But then I'd prefer to have a 'no default SAR of
> 1:1' flags so file formats can inhibit the assumption instead of
> explicitly needing to enable it. Is there any formats where this would
> be useful? Or how about just making av_guess_sample_aspect_ratio()
> return 1:1 in case nothing better exists? It's called "guess" after all
> and is used in ffplay/ffprobe only...

Opt in for MOV/MP4 is required because *the specs says* that renderers 
should behave this way. So the aspect ratio guess is based on 
something, and not a wild guess. I think most of the other format specs 
does not say such things.

Regards,
Marton
Michael Niedermayer April 16, 2018, 11:02 p.m.
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@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

[...]
Timo Teräs April 17, 2018, 5:32 a.m.
On Tue, 17 Apr 2018 01:02:43 +0200
Michael Niedermayer <michael@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@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
the code thought it was doing earlier, but due to incorrect validity
test was not doing in that specific code path (works on other code
paths):
https://ffmpeg.org/pipermail/ffmpeg-devel/2018-April/228141.html

Do note that calculating the reduced DAR requires some mathematics
which may not be simple to do from e.g. shell or simple scripting
languages. This is the primary reason why I was hoping ffprobe to do
this for me.

Would it be acceptable to print the reduced "raw" height:width ratio as
surface_aspect_ratio (or with some other better name)?

The application interpreting the info can then use that instead of
display_aspect_ratio when it's N/A.

Timo
Tobias Rapp April 17, 2018, 6:52 a.m.
On 17.04.2018 07:32, Timo Teras wrote:
> 
> [...]
> 
> Do note that calculating the reduced DAR requires some mathematics
> which may not be simple to do from e.g. shell or simple scripting
> languages. This is the primary reason why I was hoping ffprobe to do
> this for me.
> 
> Would it be acceptable to print the reduced "raw" height:width ratio as
> surface_aspect_ratio (or with some other better name)?
> 
> The application interpreting the info can then use that instead of
> display_aspect_ratio when it's N/A.

Depending on the application use-case it might be sufficient to use a 
float DAR, i.e. when comparing to a list of values like 4:3, 16:9, etc 
(within some "epsilon" delta, of course).

Regards,
Tobias
Michael Niedermayer April 17, 2018, 9:06 a.m.
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@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@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

[...]
Hendrik Leppkes April 17, 2018, 9:16 a.m.
On Tue, Apr 17, 2018 at 11:06 AM, Michael Niedermayer
<michael@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@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@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?

- Hendrik
Nicolas George April 17, 2018, 9:28 a.m.
Hendrik Leppkes (2018-04-17):
> 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?

There is the need for two special values: unknown/undefined and
invalid/broken.

Possible magic values are 0/1, x/0.

I think 0/1 is fine for unknown/undefined and 0/0 for invalid/broken.
But I am too lazy to check what is implemented right now.

Regards,
Timo Teräs April 17, 2018, 9:32 a.m.
On Tue, 17 Apr 2018 11:06:58 +0200
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Tue, Apr 17, 2018 at 08:32:57AM +0300, Timo Teras wrote:
> > 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

I suppose so. But seems the established de facto standard is to use 0/1
for undefined. Seems that denominator 0 means error or invalid. And
numerator 0 with any denominator means unknown.

This is documented for av_image_check_sar(),
av_guess_sample_aspect_ratio(), AVCodecParameters.sample_aspect_ratio
and various other places. And whole lot of stuff works based on this.

Timo
Michael Niedermayer April 17, 2018, 10:44 a.m.
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@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@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@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

[...]

Patch hide | download patch | download mbox

diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
index 8b2a18b6b1..f7022bd3c2 100644
--- a/fftools/ffprobe.c
+++ b/fftools/ffprobe.c
@@ -2523,15 +2523,15 @@  static int show_stream(WriterContext *w, AVFormatContext *fmt_ctx, int stream_id
         sar = av_guess_sample_aspect_ratio(fmt_ctx, stream, NULL);
         if (sar.num) {
             print_q("sample_aspect_ratio", sar, ':');
-            av_reduce(&dar.num, &dar.den,
-                      par->width  * sar.num,
-                      par->height * sar.den,
-                      1024*1024);
-            print_q("display_aspect_ratio", dar, ':');
         } else {
             print_str_opt("sample_aspect_ratio", "N/A");
-            print_str_opt("display_aspect_ratio", "N/A");
+            sar = (AVRational){ 1, 1 };
         }
+        av_reduce(&dar.num, &dar.den,
+                  par->width  * sar.num,
+                  par->height * sar.den,
+                  1024*1024);
+        print_q("display_aspect_ratio", dar, ':');
         s = av_get_pix_fmt_name(par->format);
         if (s) print_str    ("pix_fmt", s);
         else   print_str_opt("pix_fmt", "unknown");