| Message ID | 20180415115706.17172-1-timo.teras@iki.fi |
|---|---|
| State | New |
| Headers | show |
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
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
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
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 ? [...]
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
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 [...]
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
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
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 [...]
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
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,
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
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 [...]
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");
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(-)