[FFmpeg-devel] [PATCH 1/2] avformat/matroskadec: set aspect ratio only when DisplayWidth and DisplayHeight are in pixels

[James Almer]

On 10/16/2016 10:28 AM, James Almer wrote:
> 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?
> 
> 
> 0001-Partially-revert-avformat-matroskadec-set-aspect-rat.patch
> 
> 
> From 14a71576f69a7fdb4fd48502ec2ea36c26297004 Mon Sep 17 00:00:00 2001
> From: James Almer <jamrial at 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 <george at nsup.org>
> Signed-off-by: James Almer <jamrial at 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) {

Alternatively, i could make it check for "less than" rather than "not equal",
since values that are not defined (5 and above) should be ignored as they are,
as per the spec guidelines, "unknown".