On 10/16/2016 5:58 AM, Nicolas George wrote:
> Le quartidi 24 vendémiaire, an CCXXV, James Almer a écrit :
>> A missing DisplayUnit element or one with the default value of 0 means
>> DisplayWidth and DisplayHeight should be interpreted as pixels.
>>
>> The current code setting st->sample_aspect_ratio is wrong when DisplayUnit
>> is anything else.
> 
> Sorry to react after it was pushed, but: are you sure about the logic?
> Naively, I think that a/b makes sense whatever the unit for a and b, as long
> as it is known and the same: the logic should be applied for all units
> except UNKNOWN. What am I missing?

Nothing probably, just me not giving it much thought after i couldn't find
any sample using cm, in or dar instead of pixels, and assuming the
calculations were tailored specifically for sizes in pixels.
I manually created some and you're right it seems to work fine with all of
them.

Is the attached patch ok?

>From 14a71576f69a7fdb4fd48502ec2ea36c26297004 Mon Sep 17 00:00:00 2001
From: James Almer <jamr...@gmail.com>
Date: Sun, 16 Oct 2016 10:13:45 -0300
Subject: [PATCH] Partially revert "avformat/matroskadec: set aspect ratio only
 when DisplayWidth and DisplayHeight are in pixels"

The code works just fine regardless of unit, so only make sure DisplayUnit
is not "unknown".

Found-by: Nicolas George <geo...@nsup.org>
Signed-off-by: James Almer <jamr...@gmail.com>
---
 libavformat/matroskadec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 0d17a7e..f9f54ff 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -2297,7 +2297,7 @@ static int matroska_parse_tracks(AVFormatContext *s)
             if (track->video.stereo_mode && track->video.stereo_mode < MATROSKA_VIDEO_STEREOMODE_TYPE_NB)
                 mkv_stereo_mode_display_mul(track->video.stereo_mode, &display_width_mul, &display_height_mul);
 
-            if (track->video.display_unit == MATROSKA_VIDEO_DISPLAYUNIT_PIXELS) {
+            if (track->video.display_unit != MATROSKA_VIDEO_DISPLAYUNIT_UNKNOWN) {
                 av_reduce(&st->sample_aspect_ratio.num,
                           &st->sample_aspect_ratio.den,
                           st->codecpar->height * track->video.display_width  * display_width_mul,
-- 
2.9.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Reply via email to