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

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?

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@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@nsup.org>
> Signed-off-by: James Almer <jamrial@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".

Le quintidi 25 vendémiaire, an CCXXV, James Almer a écrit :
> > 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?

> > 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".

> > -            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".

The logic seems fine, and a range comparison is probably better indeed.

By the way, in this spec:
https://www.matroska.org/technical/specs/index.html
I only see values 0-3, not 4 for unknown. Is there a more authoritative
version of the spec?

Regards,

On 10/16/2016 2:01 PM, Nicolas George wrote:
> Le quintidi 25 vendémiaire, an CCXXV, James Almer a écrit :
>>> 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?
> 
>>> 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".
> 
>>> -            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".
> 
> The logic seems fine, and a range comparison is probably better indeed.
> 
> By the way, in this spec:
> https://www.matroska.org/technical/specs/index.html
> I only see values 0-3, not 4 for unknown. Is there a more authoritative
> version of the spec?
> 
> Regards,

Yes, https://github.com/Matroska-Org/matroska-specification is afaik the
best version of the spec since it's the one that's going to be standardized.
The website should eventually get synced with it, though.

4 is a new value added to the element recently, which works in a backwards
compatible way as demuxers should consider anything above 3 as "unknown"
anyway, as per the spec guidelines.

https://mailarchive.ietf.org/arch/msg/cellar/x1F00MwqytPjrcNru6Kk2CWQ474
https://github.com/Matroska-Org/matroska-specification/pull/34