diff mbox

[FFmpeg-devel,1/2] lavf/mxfdec: add support for all of the picture size and offset fields

Message ID 1474847562-22945-1-git-send-email-jeebjp@gmail.com
State Changes Requested
Headers show

Commit Message

Jan Ekström Sept. 25, 2016, 11:52 p.m. UTC 
* Renames the `width` and `height` entries to `stored_{width,height}`
  according to the specification's naming.
* Switches the data type of widths and heights to uint32_t according
  to specification, adds additional checks to make sure we don't
  overflow INT_MAX as codecpar->{width,height} are signed integers.
* Adds and parses the sampled and display width and height entries.
* Adds and parses the sampled and display X/Y offsets. These together
  with the additional widths and heights enable future support for
  container cropping if and when there will be generic tooling for
  it within FFmpeg.

Signed-off-by: Jan Ekström <jeebjp@gmail.com>
---
 libavformat/mxfdec.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 54 insertions(+), 6 deletions(-)

Comments

Carl Eugen Hoyos Sept. 26, 2016, 6:29 a.m. UTC | #1 
2016-09-26 1:52 GMT+02:00 Jan Ekström <jeebjp@gmail.com>:
> * Renames the `width` and `height` entries to `stored_{width,height}`
>   according to the specification's naming.

This should be a separate patch. (Although I wonder how useful it is:
Are you planning to add yourself as mxf maintainer? This would be
very welcome.)

> * Switches the data type of widths and heights to uint32_t according
>   to specification, adds additional checks to make sure we don't
>   overflow INT_MAX as codecpar->{width,height} are signed integers.

If this fixes anything (I am not sure I understand: Are you changing a
type which introduces a possible undefined behaviour, so you add an
additional check at the same time?) it should be part of above new
patch imo (or an independent patch).

> * Adds and parses the sampled and display width and height entries.

The rest should be added together with the side data injection.

Carl Eugen
Jan Ekström Sept. 26, 2016, 6:42 a.m. UTC | #2 
On Sep 26, 2016 09:29, "Carl Eugen Hoyos" <ceffmpeg@gmail.com> wrote:
>
> 2016-09-26 1:52 GMT+02:00 Jan Ekström <jeebjp@gmail.com>:
>
> If this fixes anything (I am not sure I understand: Are you changing a
> type which introduces a possible undefined behaviour, so you add an
> additional check at the same time?) it should be part of above new
> patch imo (or an independent patch).
>

The specification notes that width and height are uint32 in MXF. Thus the
old type of "int" was incorrect. The width and height of codecpar are "int"
so by fixing the type it can overflow (you'd get negative values), and thus
have to be capped.

The old code would have had thus an overflow happen naturally in the case
of a large enough unsigned integer as the value would have been read into
an int.

Jan
Carl Eugen Hoyos Sept. 26, 2016, 7 a.m. UTC | #3 
2016-09-26 8:42 GMT+02:00 Jan Ekstrom <jeebjp@gmail.com>:
> On Sep 26, 2016 09:29, "Carl Eugen Hoyos" <ceffmpeg@gmail.com> wrote:
>>
>> 2016-09-26 1:52 GMT+02:00 Jan Ekström <jeebjp@gmail.com>:
>>
>> If this fixes anything (I am not sure I understand: Are you changing a
>> type which introduces a possible undefined behaviour, so you add an
>> additional check at the same time?) it should be part of above new
>> patch imo (or an independent patch).
>
> The specification notes that width and height are uint32 in MXF. Thus the
> old type of "int" was incorrect. The width and height of codecpar are "int"
> so by fixing the type it can overflow (you'd get negative values), and thus
> have to be capped.
>
> The old code would have had thus an overflow happen naturally in the
> case of a large enough unsigned integer as the value would have been
> read into an int.

I thought no overflow happens in that case but it does not really matter:
FFmpeg ignores even 16bit width / height (for msb set).

Carl Eugen
diff mbox

Patch

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index 1939761..54fc6fb 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -170,8 +170,17 @@  typedef struct MXFDescriptor {
     UID codec_ul;
     AVRational sample_rate;
     AVRational aspect_ratio;
-    int width;
-    int height; /* Field height, not frame height */
+    /* In case of frame_layout == SeparateFields or SegmentedFrame, height is of field, not frame */
+    uint32_t stored_width;
+    uint32_t stored_height;
+    uint32_t sampled_width;
+    uint32_t sampled_height;
+    int32_t  sampled_x_offset;
+    int32_t  sampled_y_offset;
+    uint32_t display_width;
+    uint32_t display_height;
+    int32_t  display_x_offset;
+    int32_t  display_y_offset;
     int frame_layout; /* See MXFFrameLayout enum */
 #define MXF_TFF 1
 #define MXF_BFF 2
@@ -988,10 +997,34 @@  static int mxf_read_generic_descriptor(void *arg, AVIOContext *pb, int tag, int
         avio_read(pb, descriptor->essence_codec_ul, 16);
         break;
     case 0x3203:
-        descriptor->width = avio_rb32(pb);
+        descriptor->stored_width = avio_rb32(pb);
         break;
     case 0x3202:
-        descriptor->height = avio_rb32(pb);
+        descriptor->stored_height = avio_rb32(pb);
+        break;
+    case 0x3205:
+        descriptor->sampled_width = avio_rb32(pb);
+        break;
+    case 0x3204:
+        descriptor->sampled_height = avio_rb32(pb);
+        break;
+    case 0x3206:
+        descriptor->sampled_x_offset = avio_rb32(pb);
+        break;
+    case 0x3207:
+        descriptor->sampled_y_offset = avio_rb32(pb);
+        break;
+    case 0x3209:
+        descriptor->display_width = avio_rb32(pb);
+        break;
+    case 0x3208:
+        descriptor->display_height = avio_rb32(pb);
+        break;
+    case 0x320A:
+        descriptor->display_x_offset = avio_rb32(pb);
+        break;
+    case 0x320B:
+        descriptor->display_y_offset = avio_rb32(pb);
         break;
     case 0x320C:
         descriptor->frame_layout = avio_r8(pb);
@@ -2027,8 +2060,16 @@  static int mxf_parse_structural_metadata(MXFContext *mxf)
             container_ul = mxf_get_codec_ul(mxf_picture_essence_container_uls, essence_container_ul);
             if (st->codecpar->codec_id == AV_CODEC_ID_NONE)
                 st->codecpar->codec_id = container_ul->id;
-            st->codecpar->width = descriptor->width;
-            st->codecpar->height = descriptor->height; /* Field height, not frame height */
+
+            if (descriptor->stored_width > INT_MAX || descriptor->stored_height > INT_MAX) {
+                av_log(mxf->fc, AV_LOG_ERROR,
+                       "One or both of the descriptor's storage width/height values does not fit within an integer! "
+                       "(width=%"PRIu32", height=%"PRIu32")\n", descriptor->stored_width, descriptor->stored_height);
+                ret = AVERROR(AVERROR_PATCHWELCOME);
+                goto fail_and_free;
+            }
+            st->codecpar->width = descriptor->stored_width;
+            st->codecpar->height = descriptor->stored_height; /* Field height, not frame height */
             switch (descriptor->frame_layout) {
                 case FullFrame:
                     st->codecpar->field_order = AV_FIELD_PROGRESSIVE;
@@ -2058,6 +2099,13 @@  static int mxf_parse_structural_metadata(MXFContext *mxf)
                     case 0: // we already have many samples with field_dominance == unknown
                         break;
                     }
+                    if ((st->codecpar->height * 2) > INT_MAX) {
+                        av_log(mxf->fc, AV_LOG_ERROR,
+                               "Field height duplicated does not fit within an integer! (height*2=%"PRIu64")\n",
+                               ((uint64_t)st->codecpar->height) * 2);
+                        ret = AVERROR(AVERROR_PATCHWELCOME);
+                        goto fail_and_free;
+                    }
                     /* Turn field height into frame height. */
                     st->codecpar->height *= 2;
                     break;