| Message ID | 20180412080736.27965-1-timo.teras@iki.fi |
|---|---|
| State | Accepted |
| Commit | c663dce031b3973e37c83ae1818f1484e1cf482c |
| Headers | show |
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
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
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.
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,
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(-)