[FFmpeg-devel] ffprobe: report unavailable SAR correctly in stream info

Submitted by Timo Teräs on April 12, 2018, 8:07 a.m.

Details

Message ID 20180412080736.27965-1-timo.teras@iki.fi
State Accepted
Commit c663dce031b3973e37c83ae1818f1484e1cf482c
Headers show

Commit Message

Timo Teräs April 12, 2018, 8:07 a.m.
av_guess_sample_aspect_ratio() will return undefined or missing
value as {0,1}. This fixes show_stream() to check numerator to
display 'N/A' when appropriate. show_frame() does this already
correctly.

Signed-off-by: Timo Teräs <timo.teras@iki.fi>
---
 fftools/ffprobe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Timo Teräs April 12, 2018, 10:29 a.m.
On Thu, 12 Apr 2018 11:07:36 +0300
Timo Teräs <timo.teras@iki.fi> wrote:

> av_guess_sample_aspect_ratio() will return undefined or missing
> value as {0,1}. This fixes show_stream() to check numerator to
> display 'N/A' when appropriate. show_frame() does this already
> correctly.
> 
> Signed-off-by: Timo Teräs <timo.teras@iki.fi>

Btw. This patch is sent because I have a .mp4 file where 0/1 SAR was
reported. This is the obvious fix to report it as N/A instead.

But I was trying to figure out how to make it display a proper SAR too.
My findings are as follows.

1. MP4 dictates that if 'pasp' atom is found, it declares the aspect
   ratio and should be used. This is now required to be present if
   it's non-square SAR.

2. If 'pasp' is not found, mov demux uses the stream/codec width/height
   ratio to calculate SAR. But only if the widths and heights do not
   match.

ffprobe uses av_guess_sample_aspect_ratio() without frame to figure out
the stream's SAR. This basically return stream->sample_aspect_ratio if
set, and as a fallback it returns (frame's or) codec's SAR. If neither
is set it returns undefined. 

Now my understanding is that mov demux does #2 above so the codec/frame
SAR gets properly used if proper SAR cannot be deduced from the
stream/codec dimensions.

However, mp4 specs (both QuickTime FF specs and ISO/IEC 14496-12:2015
indicate that 'pasp' is optional if SAR is 1:1 (or square pixels). So
IMHO basically .mp4 files without 'pasp' or codec defined SAR should be
reported as 1:1 SAR (at least on ffprobe; it may make sense not to
report it on the API level so ffmpeg can leave 'pasp' out when copying
bitstreams).

This is what is happening with my file. There's no 'pasp' and the h264
codec defined sar is 'undefined'. Thus the mp4 format implied 1:1 SAR
should be assumed as last resort at least for ffprobe results.

So I am wonder how we could fix ffprobe report proper SAR instead of
N/A for above kind files. Could av_guess_sample_aspect_ratio() perhaps
be added a flag to default to 1/1 if nothing else is defined based on
some flag? Or should this be isolated to ffprobe? Any suggestions where
to store this flag, or even last-resort SAR that can be set by demux?

Or would it be reasonable for ffprobe to assume 1/1 SAR for all file
formats if SAR does not seem to be available?

Timo
Rostislav Pehlivanov April 14, 2018, 8:20 p.m.
On 12 April 2018 at 09:07, Timo Teräs <timo.teras@iki.fi> wrote:

> av_guess_sample_aspect_ratio() will return undefined or missing
> value as {0,1}. This fixes show_stream() to check numerator to
> display 'N/A' when appropriate. show_frame() does this already
> correctly.
>
> Signed-off-by: Timo Teräs <timo.teras@iki.fi>
> ---
>  fftools/ffprobe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
> index 82dfe4f58a..8b2a18b6b1 100644
> --- a/fftools/ffprobe.c
> +++ b/fftools/ffprobe.c
> @@ -2521,7 +2521,7 @@ static int show_stream(WriterContext *w,
> AVFormatContext *fmt_ctx, int stream_id
>  #endif
>          print_int("has_b_frames", par->video_delay);
>          sar = av_guess_sample_aspect_ratio(fmt_ctx, stream, NULL);
> -        if (sar.den) {
> +        if (sar.num) {
>              print_q("sample_aspect_ratio", sar, ':');
>              av_reduce(&dar.num, &dar.den,
>                        par->width  * sar.num,
> --
> 2.17.0
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

LGTM
Rostislav Pehlivanov April 17, 2018, 7:14 p.m.
On 14 April 2018 at 21:20, Rostislav Pehlivanov <atomnuker@gmail.com> wrote:

>
>
> On 12 April 2018 at 09:07, Timo Teräs <timo.teras@iki.fi> wrote:
>
>> av_guess_sample_aspect_ratio() will return undefined or missing
>> value as {0,1}. This fixes show_stream() to check numerator to
>> display 'N/A' when appropriate. show_frame() does this already
>> correctly.
>>
>> Signed-off-by: Timo Teräs <timo.teras@iki.fi>
>> ---
>>  fftools/ffprobe.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
>> index 82dfe4f58a..8b2a18b6b1 100644
>> --- a/fftools/ffprobe.c
>> +++ b/fftools/ffprobe.c
>> @@ -2521,7 +2521,7 @@ static int show_stream(WriterContext *w,
>> AVFormatContext *fmt_ctx, int stream_id
>>  #endif
>>          print_int("has_b_frames", par->video_delay);
>>          sar = av_guess_sample_aspect_ratio(fmt_ctx, stream, NULL);
>> -        if (sar.den) {
>> +        if (sar.num) {
>>              print_q("sample_aspect_ratio", sar, ':');
>>              av_reduce(&dar.num, &dar.den,
>>                        par->width  * sar.num,
>> --
>> 2.17.0
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>
> LGTM
>

Applied, thanks.

Patch hide | download patch | download mbox

diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
index 82dfe4f58a..8b2a18b6b1 100644
--- a/fftools/ffprobe.c
+++ b/fftools/ffprobe.c
@@ -2521,7 +2521,7 @@  static int show_stream(WriterContext *w, AVFormatContext *fmt_ctx, int stream_id
 #endif
         print_int("has_b_frames", par->video_delay);
         sar = av_guess_sample_aspect_ratio(fmt_ctx, stream, NULL);
-        if (sar.den) {
+        if (sar.num) {
             print_q("sample_aspect_ratio", sar, ':');
             av_reduce(&dar.num, &dar.den,
                       par->width  * sar.num,