diff mbox

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

Message ID 20161015220943.660-1-jamrial@gmail.com
State Superseded
Headers show

Commit Message

James Almer Oct. 15, 2016, 10:09 p.m. UTC 
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.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavformat/matroska.h    |  8 ++++++++
 libavformat/matroskadec.c | 14 ++++++++------
 2 files changed, 16 insertions(+), 6 deletions(-)

Comments

Carl Eugen Hoyos Oct. 15, 2016, 11:07 p.m. UTC | #1 
2016-10-16 0:09 GMT+02:00 James Almer <jamrial@gmail.com>:

> +            if (track->video.display_unit == MATROSKA_VIDEO_DISPLAYUNIT_PIXELS)

If you don't mind please add braces here.

Both patches make sense afaict.

Thank you, Carl Eugen
James Almer Oct. 15, 2016, 11:54 p.m. UTC | #2 
On 10/15/2016 8:07 PM, Carl Eugen Hoyos wrote:
> 2016-10-16 0:09 GMT+02:00 James Almer <jamrial@gmail.com>:
> 
>> +            if (track->video.display_unit == MATROSKA_VIDEO_DISPLAYUNIT_PIXELS)
> 
> If you don't mind please add braces here.

Added brackets, which i assume is what you meant.

> 
> Both patches make sense afaict.
> 
> Thank you, Carl Eugen

Pushed. Thanks.
Nicolas George Oct. 16, 2016, 8:58 a.m. UTC | #3 
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?

Regards,
diff mbox

Patch

diff --git a/libavformat/matroska.h b/libavformat/matroska.h
index 15e401c..8ad89da 100644
--- a/libavformat/matroska.h
+++ b/libavformat/matroska.h
@@ -309,6 +309,14 @@  typedef enum {
   MATROSKA_VIDEO_STEREOMODE_TYPE_NB,
 } MatroskaVideoStereoModeType;
 
+typedef enum {
+  MATROSKA_VIDEO_DISPLAYUNIT_PIXELS      = 0,
+  MATROSKA_VIDEO_DISPLAYUNIT_CENTIMETERS = 1,
+  MATROSKA_VIDEO_DISPLAYUNIT_INCHES      = 2,
+  MATROSKA_VIDEO_DISPLAYUNIT_DAR         = 3,
+  MATROSKA_VIDEO_DISPLAYUNIT_UNKNOWN     = 4,
+} MatroskaVideoDisplayUnit;
+
 /*
  * Matroska Codec IDs, strings
  */
diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index cfe4692..a0afbb9 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -168,6 +168,7 @@  typedef struct MatroskaTrackVideo {
     uint64_t pixel_width;
     uint64_t pixel_height;
     EbmlBin color_space;
+    uint64_t display_unit;
     uint64_t interlaced;
     uint64_t field_order;
     uint64_t stereo_mode;
@@ -436,7 +437,7 @@  static const EbmlSyntax matroska_track_video[] = {
     { MATROSKA_ID_VIDEOPIXELCROPT,     EBML_NONE },
     { MATROSKA_ID_VIDEOPIXELCROPL,     EBML_NONE },
     { MATROSKA_ID_VIDEOPIXELCROPR,     EBML_NONE },
-    { MATROSKA_ID_VIDEODISPLAYUNIT,    EBML_NONE },
+    { MATROSKA_ID_VIDEODISPLAYUNIT,    EBML_UINT,  0, offsetof(MatroskaTrackVideo, display_unit), { .u= MATROSKA_VIDEO_DISPLAYUNIT_PIXELS } },
     { MATROSKA_ID_VIDEOFLAGINTERLACED, EBML_UINT,  0, offsetof(MatroskaTrackVideo, interlaced),  { .u = MATROSKA_VIDEO_INTERLACE_FLAG_UNDETERMINED } },
     { MATROSKA_ID_VIDEOFIELDORDER,     EBML_UINT,  0, offsetof(MatroskaTrackVideo, field_order), { .u = MATROSKA_VIDEO_FIELDORDER_UNDETERMINED } },
     { MATROSKA_ID_VIDEOSTEREOMODE,     EBML_UINT,  0, offsetof(MatroskaTrackVideo, stereo_mode), { .u = MATROSKA_VIDEO_STEREOMODE_TYPE_NB } },
@@ -2300,11 +2301,12 @@  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);
 
-            av_reduce(&st->sample_aspect_ratio.num,
-                      &st->sample_aspect_ratio.den,
-                      st->codecpar->height * track->video.display_width  * display_width_mul,
-                      st->codecpar->width  * track->video.display_height * display_height_mul,
-                      255);
+            if (track->video.display_unit == MATROSKA_VIDEO_DISPLAYUNIT_PIXELS)
+                av_reduce(&st->sample_aspect_ratio.num,
+                          &st->sample_aspect_ratio.den,
+                          st->codecpar->height * track->video.display_width  * display_width_mul,
+                          st->codecpar->width  * track->video.display_height * display_height_mul,
+                          255);
             if (st->codecpar->codec_id != AV_CODEC_ID_HEVC)
                 st->need_parsing = AVSTREAM_PARSE_HEADERS;