[FFmpeg-devel,2/2] lavf/mxfdec: begin utilizing the newly parsed widths and heights

Submitted by Jan Ekström on Sept. 25, 2016, 11:52 p.m.

Details

Message ID 1474847562-22945-2-git-send-email-jeebjp@gmail.com
State New
Headers show

Commit Message

Jan Ekström Sept. 25, 2016, 11:52 p.m.
* Updates the width/height sanity check to check all values against
  INT_MAX.
* Correctly utilizes the stored width or height by default, and if
  sampled or display values are available they are utilized.

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

Comments

Marton Balint Sept. 26, 2016, 1:05 a.m.
On Mon, 26 Sep 2016, Jan Ekström wrote:

> * Updates the width/height sanity check to check all values against
>  INT_MAX.
> * Correctly utilizes the stored width or height by default, and if
>  sampled or display values are available they are utilized.
>
> Signed-off-by: Jan Ekström <jeebjp@gmail.com>
> ---
> libavformat/mxfdec.c | 30 +++++++++++++++++++++++++++---
> 1 file changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index 54fc6fb..f90a85c 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -2061,15 +2061,39 @@ static int mxf_parse_structural_metadata(MXFContext *mxf)
>             if (st->codecpar->codec_id == AV_CODEC_ID_NONE)
>                 st->codecpar->codec_id = container_ul->id;
> 
> -            if (descriptor->stored_width > INT_MAX || descriptor->stored_height > INT_MAX) {
> +            if (descriptor->stored_width > INT_MAX || descriptor->stored_height > INT_MAX ||
> +                descriptor->sampled_width > INT_MAX || descriptor->sampled_height > INT_MAX ||
> +                descriptor->display_width > INT_MAX || descriptor->display_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);
> +                       "One or more of the descriptor's storage width/height values does not fit within an integer:\n"
> +                       "(stored_width=%"PRIu32", stored_height=%"PRIu32")\n"
> +                       "(sampled_width=%"PRIu32", sampled_height=%"PRIu32")\n"
> +                       "(display_width=%"PRIu32", display_height=%"PRIu32")\n",
> +                       descriptor->stored_width, descriptor->stored_height,
> +                       descriptor->sampled_width, descriptor->sampled_height,
> +                       descriptor->display_width, descriptor->display_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 */
> +
> +            /*
> +               Widths and heights get overridden storage->sampled->display.
> +               In case of an unset value the previous value shall be used.
> +            */
> +            if (descriptor->sampled_width)
> +                st->codecpar->width = descriptor->sampled_width;
> +
> +            if (descriptor->display_width)
> +                st->codecpar->width = descriptor->display_width;
> +
> +            if (descriptor->sampled_height)
> +                st->codecpar->height = descriptor->sampled_height;
> +
> +            if (descriptor->display_height)
> +                st->codecpar->height = descriptor->display_height;

Overriding width/height with display width/height does not seem right, 
check what happens with a PAL IMX50 MXF file for example. If 
you want to signal this, then a stream side data with some AVPanScan 
values might make more sense.

Regards,
Marton
Jan Ekström Sept. 26, 2016, 5:29 p.m.
On Sep 26, 2016 04:05, "Marton Balint" <cus@passwd.hu> wrote:
>
> Overriding width/height with display width/height does not seem right, check what happens with a PAL IMX50 MXF file for example. If you want to signal this, then a stream side data with some AVPanScan values might make more sense.
>

Such a file was actually the reason why I started looking into this :)
. And it would all depend on the definition of width/height in
codecpar, which as far as I can tell is not clearly cut (see the notes
regarding container crop in at least the AVC decoder, I think?). My
understanding was that it would be the displayed width/height. Of
course, the container cropping/padding makes this less simple, since
you have:

1) Decoded picture
2) Decoder cropped picture (what the decoders *currently* output)
3) Decoder cropped picture cropped/padded according to available metadata

So my understanding was - since a decoder should output the pictures
according to 3) - that the displayed width/height fields should be
utilized for signaling the final display width/height of the picture.
The X/Y offset handling of course should have had its own fields
*somewhere* so that stuff could be done in some common part of avcodec
(for example). But if the codecpar->width/height is specified to 2)
(at least in lavf), then of course side data would be the correct way
to handle this. Also thanks for the hint regarding AVPanScan, I had no
idea this existed. Will have to check if it actually is used anywhere.

Best regards,
Jan
Marton Balint Sept. 27, 2016, 7:53 a.m.
On Mon, 26 Sep 2016, Jan Ekstrom wrote:

> On Sep 26, 2016 04:05, "Marton Balint" <cus@passwd.hu> wrote:
>>
>> Overriding width/height with display width/height does not seem right, check what happens with a PAL IMX50 MXF file for example. If you want to signal this, then a stream side data with some AVPanScan values might make more sense.
>>
>
> Such a file was actually the reason why I started looking into this :)
> . And it would all depend on the definition of width/height in
> codecpar, which as far as I can tell is not clearly cut (see the notes
> regarding container crop in at least the AVC decoder, I think?). My
> understanding was that it would be the displayed width/height. Of
> course, the container cropping/padding makes this less simple, since
> you have:
>
> 1) Decoded picture
> 2) Decoder cropped picture (what the decoders *currently* output)
> 3) Decoder cropped picture cropped/padded according to available metadata
>
> So my understanding was - since a decoder should output the pictures
> according to 3) - that the displayed width/height fields should be
> utilized for signaling the final display width/height of the picture.
> The X/Y offset handling of course should have had its own fields
> *somewhere* so that stuff could be done in some common part of avcodec
> (for example). But if the codecpar->width/height is specified to 2)
> (at least in lavf), then of course side data would be the correct way
> to handle this. Also thanks for the hint regarding AVPanScan, I had no
> idea this existed. Will have to check if it actually is used anywhere.
>

Thanks, I think it is 2) indeed. Also container width/height information 
is overridden from what is probed in the codec, so even if you set 
height to 576 in the demuxer part, later, when the first frame is 
analyzed, it will get written back to 608.

You might also check some earlier mails in a similar topic: 
http://ffmpeg.org/pipermail/ffmpeg-devel/2012-September/131203.html

Regards,
Marton

Patch hide | download patch | download mbox

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index 54fc6fb..f90a85c 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -2061,15 +2061,39 @@  static int mxf_parse_structural_metadata(MXFContext *mxf)
             if (st->codecpar->codec_id == AV_CODEC_ID_NONE)
                 st->codecpar->codec_id = container_ul->id;
 
-            if (descriptor->stored_width > INT_MAX || descriptor->stored_height > INT_MAX) {
+            if (descriptor->stored_width > INT_MAX || descriptor->stored_height > INT_MAX ||
+                descriptor->sampled_width > INT_MAX || descriptor->sampled_height > INT_MAX ||
+                descriptor->display_width > INT_MAX || descriptor->display_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);
+                       "One or more of the descriptor's storage width/height values does not fit within an integer:\n"
+                       "(stored_width=%"PRIu32", stored_height=%"PRIu32")\n"
+                       "(sampled_width=%"PRIu32", sampled_height=%"PRIu32")\n"
+                       "(display_width=%"PRIu32", display_height=%"PRIu32")\n",
+                       descriptor->stored_width, descriptor->stored_height,
+                       descriptor->sampled_width, descriptor->sampled_height,
+                       descriptor->display_width, descriptor->display_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 */
+
+            /*
+               Widths and heights get overridden storage->sampled->display.
+               In case of an unset value the previous value shall be used.
+            */
+            if (descriptor->sampled_width)
+                st->codecpar->width = descriptor->sampled_width;
+
+            if (descriptor->display_width)
+                st->codecpar->width = descriptor->display_width;
+
+            if (descriptor->sampled_height)
+                st->codecpar->height = descriptor->sampled_height;
+
+            if (descriptor->display_height)
+                st->codecpar->height = descriptor->display_height;
+
             switch (descriptor->frame_layout) {
                 case FullFrame:
                     st->codecpar->field_order = AV_FIELD_PROGRESSIVE;