| Message ID | 1474847562-22945-2-git-send-email-jeebjp@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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;
* 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(-)