[FFmpeg-devel] avformat/mxfenc: fix stored/sampled/displayed width/height

[FFmpeg-devel] avformat/mxfenc: fix stored/sampled/displayed width/height

[Jerome Martinez]

[-- Attachment #1: Type: text/plain, Size: 865 bytes --]

Before the patch:
- stored values were rounded to upper 16 multiple also for formats not 
using macroblocks (should be st->codecpar->width and 
st->codecpar->height when not MPEG formats; note that I found no other 
muxer doing the rounding for AVC, only for MPEG-2 Video, but I find no 
reason in specs for doing the difference so I kept the rounding for AVC)
- sampled and displayed widths were stored width (should be 
st->codecpar->width like it is already done for height, with the 
DV50/100 exception)

Could be tested with e.g.
- fixed stored width (1912 instead of 1920) and height (1080 instead of 
1088) not multiple of 16 :
ffmpeg -f lavfi -i testsrc=duration=1:size=1912x1080 -c:v jpeg2000 
test_prores.mxf
- fixed sampled/displayed width (1912 instead of 1920):
ffmpeg -f lavfi -i testsrc=duration=1:size=1912x1080 -c:v mpeg2video 
test_mpeg2video.mxf

[-- Attachment #2: 0001-avformat-mxfenc-fix-stored-sampled-displayed-width-h.patch --]
[-- Type: text/plain, Size: 2925 bytes --]

From cda353059886182aab2e258023c4d027c448344b Mon Sep 17 00:00:00 2001
From: Jerome Martinez <jerome@mediaarea.net>
Date: Sat, 14 Jan 2023 13:32:36 +0100
Subject: [PATCH] avformat/mxfenc: fix stored/sampled/displayed width/height

Stored values are rounded to upper 16 multiple only for MPEG related formats
Sampled and displayed widths are codecpar ones (with DV exception)
---
 libavformat/mxfenc.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index 58c551c83c..0b7e83ba4d 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -1109,8 +1109,9 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
 {
     MXFStreamContext *sc = st->priv_data;
     AVIOContext *pb = s->pb;
-    int stored_width = 0;
-    int stored_height = (st->codecpar->height+15)/16*16;
+    int stored_width = st->codecpar->width;
+    int stored_height = st->codecpar->height;
+    int display_width;
     int display_height;
     int f1, f2;
     const MXFCodecUL *color_primaries_ul;
@@ -1129,12 +1130,25 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
         else if (st->codecpar->height == 720)
             stored_width = 1280;
     }
-    if (!stored_width)
-        stored_width = (st->codecpar->width+15)/16*16;
+    display_width = stored_width;
 
+    switch (st->codecpar->codec_id) {
+    case AV_CODEC_ID_MPEG2VIDEO:
+    case AV_CODEC_ID_DVVIDEO:
+    case AV_CODEC_ID_H264:
+        //Based on 16x16 macroblocks
+        stored_width = (stored_width+15)/16*16;
+        stored_height = (stored_height+15)/16*16;
+        break;
+    default:
+        break;
+    }
+
+    //Stored width
     mxf_write_local_tag(s, 4, 0x3203);
     avio_wb32(pb, stored_width);
 
+    //Stored height
     mxf_write_local_tag(s, 4, 0x3202);
     avio_wb32(pb, stored_height>>sc->interlaced);
 
@@ -1154,7 +1168,7 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
 
     //Sampled width
     mxf_write_local_tag(s, 4, 0x3205);
-    avio_wb32(pb, stored_width);
+    avio_wb32(pb, display_width);
 
     //Samples height
     mxf_write_local_tag(s, 4, 0x3204);
@@ -1168,8 +1182,9 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
     mxf_write_local_tag(s, 4, 0x3207);
     avio_wb32(pb, 0);
 
+    //Display width
     mxf_write_local_tag(s, 4, 0x3209);
-    avio_wb32(pb, stored_width);
+    avio_wb32(pb, display_width);
 
     if (st->codecpar->height == 608) // PAL + VBI
         display_height = 576;
@@ -1178,6 +1193,7 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
     else
         display_height = st->codecpar->height;
 
+    //Display height
     mxf_write_local_tag(s, 4, 0x3208);
     avio_wb32(pb, display_height>>sc->interlaced);
 
-- 
2.13.3.windows.1


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] avformat/mxfenc: fix stored/sampled/displayed width/height

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 3360 bytes --]

On Sat, Jan 14, 2023 at 04:48:10PM +0100, Jerome Martinez wrote:
> Before the patch:
> - stored values were rounded to upper 16 multiple also for formats not using
> macroblocks (should be st->codecpar->width and st->codecpar->height when not
> MPEG formats; note that I found no other muxer doing the rounding for AVC,
> only for MPEG-2 Video, but I find no reason in specs for doing the
> difference so I kept the rounding for AVC)
> - sampled and displayed widths were stored width (should be
> st->codecpar->width like it is already done for height, with the DV50/100
> exception)
> 
> Could be tested with e.g.
> - fixed stored width (1912 instead of 1920) and height (1080 instead of
> 1088) not multiple of 16 :
> ffmpeg -f lavfi -i testsrc=duration=1:size=1912x1080 -c:v jpeg2000
> test_prores.mxf
> - fixed sampled/displayed width (1912 instead of 1920):
> ffmpeg -f lavfi -i testsrc=duration=1:size=1912x1080 -c:v mpeg2video
> test_mpeg2video.mxf

>  mxfenc.c |   28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> ff6497b7983ccb5ec7555b55ad0040a2c065d6d3  0001-avformat-mxfenc-fix-stored-sampled-displayed-width-h.patch
> From cda353059886182aab2e258023c4d027c448344b Mon Sep 17 00:00:00 2001
> From: Jerome Martinez <jerome@mediaarea.net>
> Date: Sat, 14 Jan 2023 13:32:36 +0100
> Subject: [PATCH] avformat/mxfenc: fix stored/sampled/displayed width/height
> 
> Stored values are rounded to upper 16 multiple only for MPEG related formats
> Sampled and displayed widths are codecpar ones (with DV exception)
> ---
>  libavformat/mxfenc.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index 58c551c83c..0b7e83ba4d 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -1109,8 +1109,9 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
>  {
>      MXFStreamContext *sc = st->priv_data;
>      AVIOContext *pb = s->pb;
> -    int stored_width = 0;
> -    int stored_height = (st->codecpar->height+15)/16*16;
> +    int stored_width = st->codecpar->width;
> +    int stored_height = st->codecpar->height;
> +    int display_width;
>      int display_height;
>      int f1, f2;
>      const MXFCodecUL *color_primaries_ul;
> @@ -1129,12 +1130,25 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
>          else if (st->codecpar->height == 720)
>              stored_width = 1280;
>      }
> -    if (!stored_width)
> -        stored_width = (st->codecpar->width+15)/16*16;
> +    display_width = stored_width;
>  
> +    switch (st->codecpar->codec_id) {
> +    case AV_CODEC_ID_MPEG2VIDEO:
> +    case AV_CODEC_ID_DVVIDEO:
> +    case AV_CODEC_ID_H264:
> +        //Based on 16x16 macroblocks
> +        stored_width = (stored_width+15)/16*16;

> +        stored_height = (stored_height+15)/16*16;

If this is supposed to match the actual macroblocks, then this would
have to consider field pictures and interlacing as it differs from
progressive

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] avformat/mxfenc: fix stored/sampled/displayed width/height

[Jerome Martinez]

On 14/01/2023 21:04, Michael Niedermayer wrote:
> On Sat, Jan 14, 2023 at 04:48:10PM +0100, Jerome Martinez wrote:
> [...]
>> +        stored_height = (stored_height+15)/16*16;
> If this is supposed to match the actual macroblocks, then this would
> have to consider field pictures and interlacing as it differs from
> progressive

There is no change on that side, no addition, no change about it, this 
code is moved, and interlace is already managed in the current code:

     //Stored height
     mxf_write_local_tag(s, 4, 0x3202);
     avio_wb32(pb, stored_height>>sc->interlaced);
[...]
    //Display height
     mxf_write_local_tag(s, 4, 0x3208);
     avio_wb32(pb, display_height>>sc->interlaced);

The patch disables the rounding for some formats, it is not intended to 
change the behavior for MPEG-2 Video and AVC formats as it seems fine 
(FFmpeg has the same behavior as other muxers, at least for MPEG-2 
Video, also for interlaced content).

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] avformat/mxfenc: fix stored/sampled/displayed width/height

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1356 bytes --]

On Sun, Jan 15, 2023 at 03:24:37PM +0100, Jerome Martinez wrote:
> On 14/01/2023 21:04, Michael Niedermayer wrote:
> > On Sat, Jan 14, 2023 at 04:48:10PM +0100, Jerome Martinez wrote:
> > [...]
> > > +        stored_height = (stored_height+15)/16*16;
> > If this is supposed to match the actual macroblocks, then this would
> > have to consider field pictures and interlacing as it differs from
> > progressive
> 
> There is no change on that side, no addition, no change about it, this code
> is moved, and interlace is already managed in the current code:
> 
>     //Stored height
>     mxf_write_local_tag(s, 4, 0x3202);
>     avio_wb32(pb, stored_height>>sc->interlaced);
> [...]
>    //Display height
>     mxf_write_local_tag(s, 4, 0x3208);
>     avio_wb32(pb, display_height>>sc->interlaced);
> 
> The patch disables the rounding for some formats, it is not intended to
> change the behavior for MPEG-2 Video and AVC formats as it seems fine
> (FFmpeg has the same behavior as other muxers, at least for MPEG-2 Video,
> also for interlaced content).

ok

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] avformat/mxfenc: fix stored/sampled/displayed width/height

[Tomas Härdin]

lör 2023-01-14 klockan 16:48 +0100 skrev Jerome Martinez:
> Before the patch:
> - stored values were rounded to upper 16 multiple also for formats
> not 
> using macroblocks (should be st->codecpar->width and 
> st->codecpar->height when not MPEG formats; note that I found no
> other 
> muxer doing the rounding for AVC, only for MPEG-2 Video, but I find
> no 
> reason in specs for doing the difference so I kept the rounding for
> AVC)
> - sampled and displayed widths were stored width (should be 
> st->codecpar->width like it is already done for height, with the 
> DV50/100 exception)

Another option might be to omit these values for non-macroblock codecs.
There is also potentially one use-value: when remuxing BMP to MXF it
may be necessary to deal with BMP's 16-bit alignment. I'm not sure if
this happens in the wild, and we certainly don't support muxing it.

Another very close reading of the spec seems appropriate.

/Tomas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] avformat/mxfenc: fix stored/sampled/displayed width/height

[Jerome Martinez]

On 16/01/2023 14:50, Tomas Härdin wrote:
> lör 2023-01-14 klockan 16:48 +0100 skrev Jerome Martinez:
>> Before the patch:
>> - stored values were rounded to upper 16 multiple also for formats
>> not
>> using macroblocks (should be st->codecpar->width and
>> st->codecpar->height when not MPEG formats; note that I found no
>> other
>> muxer doing the rounding for AVC, only for MPEG-2 Video, but I find
>> no
>> reason in specs for doing the difference so I kept the rounding for
>> AVC)
>> - sampled and displayed widths were stored width (should be
>> st->codecpar->width like it is already done for height, with the
>> DV50/100 exception)
> Another option might be to omit these values for non-macroblock codecs.

I planned to send a patch for removing all redundant information 
(sampled width/height is stored width/height by default etc), but as it 
is changing the current behavior of FFmpeg so can be more controversial 
I preferred to have it in a different patch.
After this patch it would be only a bunch of "if (sampled!=stored)" 
additions.
Would it be fine to do that separately?


> There is also potentially one use-value: when remuxing BMP to MXF it
> may be necessary to deal with BMP's 16-bit alignment. I'm not sure if
> this happens in the wild, and we certainly don't support muxing it.

I don't get what I should change in the patch for that, AFAIK I don't 
touch stuff around that.


> Another very close reading of the spec seems appropriate.

Tried to do so, as well as checking the other files I have from other 
muxers.
When I don't get the difference between what is in specs and FFmpeg or 
other muxer behavior, I let current FFmpeg behavior untouched.

Jérôme
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] avformat/mxfenc: fix stored/sampled/displayed width/height

[Tomas Härdin]

mån 2023-01-16 klockan 15:28 +0100 skrev Jerome Martinez:
> On 16/01/2023 14:50, Tomas Härdin wrote:
> > lör 2023-01-14 klockan 16:48 +0100 skrev Jerome Martinez:
> > > Before the patch:
> > > - stored values were rounded to upper 16 multiple also for
> > > formats
> > > not
> > > using macroblocks (should be st->codecpar->width and
> > > st->codecpar->height when not MPEG formats; note that I found no
> > > other
> > > muxer doing the rounding for AVC, only for MPEG-2 Video, but I
> > > find
> > > no
> > > reason in specs for doing the difference so I kept the rounding
> > > for
> > > AVC)
> > > - sampled and displayed widths were stored width (should be
> > > st->codecpar->width like it is already done for height, with the
> > > DV50/100 exception)
> > Another option might be to omit these values for non-macroblock
> > codecs.
> 
> I planned to send a patch for removing all redundant information 
> (sampled width/height is stored width/height by default etc), but as
> it 
> is changing the current behavior of FFmpeg so can be more
> controversial 
> I preferred to have it in a different patch.

Such information may or may not be redundant depending on context.
Which is part of why MXF is such a headache. Outputting as correct
information as possible is probably the best approach.

> After this patch it would be only a bunch of "if (sampled!=stored)" 
> additions.
> Would it be fine to do that separately?

I don't know. The problem here is that we don't know what workflows
users have.  On the other hand if we're bold and remove redundant
information then we'll learn what users are actually relying on it.

> > There is also potentially one use-value: when remuxing BMP to MXF
> > it
> > may be necessary to deal with BMP's 16-bit alignment. I'm not sure
> > if
> > this happens in the wild, and we certainly don't support muxing it.
> 
> I don't get what I should change in the patch for that, AFAIK I don't
> touch stuff around that.

No need to do anything, it's just me noting when it might be the case
that stored != sampled for non-MB formats.

> > Another very close reading of the spec seems appropriate.
> 
> Tried to do so, as well as checking the other files I have from other
> muxers.
> When I don't get the difference between what is in specs and FFmpeg
> or 
> other muxer behavior, I let current FFmpeg behavior untouched.

Ugh, cargo culting. Not that I'm not guilty of the same..

/Tomas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] avformat/mxfenc: fix stored/sampled/displayed width/height

[Nicolas Gaullier]

>Before the patch:
>- stored values were rounded to upper 16 multiple also for formats not using macroblocks (should be st->codecpar->width and 
>st->codecpar->height when not MPEG formats; note that I found no other
>muxer doing the rounding for AVC, only for MPEG-2 Video, but I find no reason in specs for doing the difference so I kept the rounding for AVC)
>- sampled and displayed widths were stored width (should be 
>st->codecpar->width like it is already done for height, with the
>DV50/100 exception)

The width is one thing; for whatever reason, there is a divergence between DV100 on one hand and AVCI/XDCAMHD35 on the other. In my understanding, in current practices, DV obey s337 (stored width includes scaling) but xdcam&avci does not, so current code is fine but maybe this is an opportunity to document this ?
The height is another topic, and in my information (checked against some samples from K2/Harmonic/bmx), DV height should not be rounded : maybe this patch is an opportunity to fix this ?

Note: please mind update fate (make fate-lavf-mxf_opatom GEN=1).

Nicolas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] avformat/mxfenc: fix stored/sampled/displayed width/height

[Jerome Martinez]

On 16/01/2023 15:00, Nicolas Gaullier wrote:
>> Before the patch:
>> - stored values were rounded to upper 16 multiple also for formats not using macroblocks (should be st->codecpar->width and
>> st->codecpar->height when not MPEG formats; note that I found no other
>> muxer doing the rounding for AVC, only for MPEG-2 Video, but I find no reason in specs for doing the difference so I kept the rounding for AVC)
>> - sampled and displayed widths were stored width (should be
>> st->codecpar->width like it is already done for height, with the
>> DV50/100 exception)
> The width is one thing; for whatever reason, there is a divergence between DV100 on one hand and AVCI/XDCAMHD35 on the other. In my understanding, in current practices, DV obey s337 (stored width includes scaling) but xdcam&avci does not, so current code is fine but maybe this is an opportunity to document this ?

AFAIK:
- DV in MXF: found old Omneon with no scaling for stored value, no 
sampled value (so stored value), scaled value for displayed value, old 
Quantel with scaling everywhere. From my understanding of spec, I would 
keep the scaling.
- MPEG-2 Video including XDCAMHD35 in MXF obey "including any decoder 
scaling or padding" wording with a 16x16 rounding for height, I have no 
file not 1920 or 3840 width
- AVC in MXF: found old Omneon or old Quantel  or old Telestream with no 
padding value for stored value (height of 540 for interlaced). I don't 
understand why it is not same as with MPEG-2 Video so I don't touch 
FFmpeg behavior there (rounding). Actually checking again SMPTE ST 
381-2013, there is an explicit example: "1088: 1080-line progressive".

Do you want me to add a comment line e.g. "obey 'including any decoder 
scaling or padding' from SMPTE ST 377"?

> The height is another topic, and in my information (checked against some samples from K2/Harmonic/bmx), DV height should not be rounded : maybe this patch is an opportunity to fix this ?

I don't have DV in MXF with non multiple of 16 (I thought that DV is 
only 720x576 or 720x480 or 1280x720 or 1920x1080, all values multiple of 
16) and don't know about video encoding in DV so I didn't want to change 
the behavior of FFmpeg when I don't know, but
case AV_CODEC_ID_DVVIDEO:
line could be definitely removed if it is fine for you.


>
> Note: please mind update fate (make fate-lavf-mxf_opatom GEN=1).

Checked (only the stored height changes, from 1088 to 1080 for this 
DNxHD file) and fate updated for next patch.


_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] avformat/mxfenc: fix stored/sampled/displayed width/height

[Nicolas Gaullier]

>> The width is one thing; for whatever reason, there is a divergence between DV100 on one hand and AVCI/XDCAMHD35 on the other. In my understanding, in current practices, DV obey s337 (stored width includes scaling) but >xdcam&avci does not, so current code is fine but maybe this is an opportunity to document this ?
>
>AFAIK:
>- DV in MXF: found old Omneon with no scaling for stored value, no sampled value (so stored value), scaled value for displayed value, old Quantel with scaling everywhere. From my understanding of spec, I would keep the scaling.
>- MPEG-2 Video including XDCAMHD35 in MXF obey "including any decoder scaling or padding" wording with a 16x16 rounding for height, I have no file not 1920 or 3840 width
>- AVC in MXF: found old Omneon or old Quantel  or old Telestream with no padding value for stored value (height of 540 for interlaced). I don't understand why it is not same as with MPEG-2 Video so I don't touch FFmpeg behavior >there (rounding). Actually checking again SMPTE ST 381-2013, there is an explicit example: "1088: 1080-line progressive".

I totally agree they are so many weird things in the wild, particularly looking at some early implementations. I also have fully broken DV100 and XDCAMHD35 Omneon records with release v6.1 (2010) at the early stages of HD, but it was fixed afterwards (with many other issues too!). Looking at GVG, 1440x1088i stored size was implemented from the early beginnings (2010 too) : sample clips are still available here : http://www.gvgdevelopers.com/concrete/products/k2/test_clips/
There is also "kind of" reference sony implementation available here both for xdcamhd35/avc-1440:  https://www.sonycreativesoftware.com/catalystbrowse
Anyway, I think we all agree not to change anything related to MPEG2 and AVC.

>I don't have DV in MXF with non multiple of 16 (I thought that DV is only 720x576 or 720x480 or 1280x720 or 1920x1080, all values multiple of
>16) and don't know about video encoding in DV so I didn't want to change the behavior of FFmpeg when I don't know, but case AV_CODEC_ID_DVVIDEO:
>line could be definitely removed if it is fine for you.
DV is questionable. Currently, the dust is under the rug (as a defaults behaviour), which is an issue with very little concern.
Now, with the patch, the dust become visible, the DV rule is made explicit and moreover it is presented as an exception, sharing code with macroblock codecs... I think it is time to fix, even if it was not your prior intention.
I don't have an extensive experience with DV too, I just have samples here and there like you, but it seems we share the same information.
Let see if someone else react and ask for keeping the current 1088 lines for DV stored height, but if nobody does, I think it should be okay.

> Do you want me to add a comment line e.g. "obey 'including any decoder scaling or padding' from SMPTE ST 377"?
I am not a core developer and will let others give their feedback. My personal opinion is that the spec is supposed to be well known and does not require commenting, but that it would be interesting to explicit why we make a difference between DV and MPEG2/AVC. And personally, I don't have the answer to this question. If nobody has one, maybe a comment could say "because this is the observed common practice".

Nicolas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] avformat/mxfenc: fix stored/sampled/displayed width/height

[Nicolas Gaullier]

>>> The width is one thing; for whatever reason, there is a divergence between DV100 on one hand and AVCI/XDCAMHD35 on the other. In my understanding, in current practices, DV obey s337 (stored width includes scaling) but >xdcam&avci does not, so current code is fine >but maybe this is an opportunity to document this ?
>>
>>AFAIK:
>>- DV in MXF: found old Omneon with no scaling for stored value, no sampled value (so stored value), scaled value for displayed value, old Quantel with scaling everywhere. From my understanding of spec, I would keep the scaling.
>>- MPEG-2 Video including XDCAMHD35 in MXF obey "including any decoder 
>>scaling or padding" wording with a 16x16 rounding for height, I have no 
>>file not 1920 or 3840 width
>>- AVC in MXF: found old Omneon or old Quantel  or old Telestream with no padding value for stored value (height of 540 for interlaced). I don't understand why it is not same as with MPEG-2 Video so I don't touch FFmpeg behavior >there (rounding). Actually checking >again SMPTE ST 381-2013, there is an explicit example: "1088: 1080-line progressive".
>
>I totally agree they are so many weird things in the wild, particularly looking at some early implementations. I also have fully broken DV100 and XDCAMHD35 Omneon records with release v6.1 (2010) at the early stages of HD, but it was fixed afterwards (with many other >issues too!). Looking at GVG, 1440x1088i stored size was implemented from the early beginnings (2010 too) : sample clips are still available here : http://www.gvgdevelopers.com/concrete/products/k2/test_clips/
>There is also "kind of" reference sony implementation available here both for xdcamhd35/avc-1440:  https://www.sonycreativesoftware.com/catalystbrowse
>Anyway, I think we all agree not to change anything related to MPEG2 and AVC.
>
>>I don't have DV in MXF with non multiple of 16 (I thought that DV is 
>>only 720x576 or 720x480 or 1280x720 or 1920x1080, all values multiple of
>>16) and don't know about video encoding in DV so I didn't want to change the behavior of FFmpeg when I don't know, but case AV_CODEC_ID_DVVIDEO:
>>line could be definitely removed if it is fine for you.
>DV is questionable. Currently, the dust is under the rug (as a defaults behaviour), which is an issue with very little concern.
>Now, with the patch, the dust become visible, the DV rule is made explicit and moreover it is presented as an exception, sharing code with macroblock codecs... I think it is time to fix, even if it was not your prior intention.
>I don't have an extensive experience with DV too, I just have samples here and there like you, but it seems we share the same information.
>Let see if someone else react and ask for keeping the current 1088 lines for DV stored height, but if nobody does, I think it should be okay.
>
>> Do you want me to add a comment line e.g. "obey 'including any decoder scaling or padding' from SMPTE ST 377"?
>I am not a core developer and will let others give their feedback. My personal opinion is that the spec is supposed to be well known and does not require commenting, but that it would be interesting to explicit why we make a difference between DV and MPEG2/AVC. And >personally, I don't have the answer to this question. If nobody has one, maybe a comment could say "because this is the observed common practice".
>
>Nicolas

Some weeks later now and no replies, maybe time to go on ?
I think the "case AV_CODEC_ID_DVVIDEO:" can be removed as discussed, fate updated and that should be ok for everybody.
(Ideally, it could have been an opportunity to document why we have this "DV exception", but I understand it is not very comfortable to write as there is no meaningful reason, so forget about this, this won't hold up the patch anyway)
For information, there was a long thread recently on ffmpeg-user about a "bug" in dnxhd stored_height (will be fixed with your patch):
https://ffmpeg.org/pipermail/ffmpeg-user/2023-February/056111.html

Nicolas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] avformat/mxfenc: fix stored/sampled/displayed width/height

[Marton Balint]

On Mon, 6 Mar 2023, Nicolas Gaullier wrote:

>>>> The width is one thing; for whatever reason, there is a divergence between DV100 on one hand and AVCI/XDCAMHD35 on the other. In my understanding, in current practices, DV obey s337 (stored width includes scaling) but >xdcam&avci does not, so current code is fine >but maybe this is an opportunity to document this ?
>>>
>>> AFAIK:
>>> - DV in MXF: found old Omneon with no scaling for stored value, no sampled value (so stored value), scaled value for displayed value, old Quantel with scaling everywhere. From my understanding of spec, I would keep the scaling.
>>> - MPEG-2 Video including XDCAMHD35 in MXF obey "including any decoder
>>> scaling or padding" wording with a 16x16 rounding for height, I have no
>>> file not 1920 or 3840 width
>>> - AVC in MXF: found old Omneon or old Quantel  or old Telestream with no padding value for stored value (height of 540 for interlaced). I don't understand why it is not same as with MPEG-2 Video so I don't touch FFmpeg behavior >there (rounding). Actually checking >again SMPTE ST 381-2013, there is an explicit example: "1088: 1080-line progressive".
>>
>> I totally agree they are so many weird things in the wild, particularly looking at some early implementations. I also have fully broken DV100 and XDCAMHD35 Omneon records with release v6.1 (2010) at the early stages of HD, but it was fixed afterwards (with many other >issues too!). Looking at GVG, 1440x1088i stored size was implemented from the early beginnings (2010 too) : sample clips are still available here : http://www.gvgdevelopers.com/concrete/products/k2/test_clips/
>> There is also "kind of" reference sony implementation available here both for xdcamhd35/avc-1440:  https://www.sonycreativesoftware.com/catalystbrowse
>> Anyway, I think we all agree not to change anything related to MPEG2 and AVC.
>>
>>> I don't have DV in MXF with non multiple of 16 (I thought that DV is
>>> only 720x576 or 720x480 or 1280x720 or 1920x1080, all values multiple of
>>> 16) and don't know about video encoding in DV so I didn't want to change the behavior of FFmpeg when I don't know, but case AV_CODEC_ID_DVVIDEO:
>>> line could be definitely removed if it is fine for you.
>> DV is questionable. Currently, the dust is under the rug (as a defaults behaviour), which is an issue with very little concern.
>> Now, with the patch, the dust become visible, the DV rule is made explicit and moreover it is presented as an exception, sharing code with macroblock codecs... I think it is time to fix, even if it was not your prior intention.
>> I don't have an extensive experience with DV too, I just have samples here and there like you, but it seems we share the same information.
>> Let see if someone else react and ask for keeping the current 1088 lines for DV stored height, but if nobody does, I think it should be okay.
>>
>>> Do you want me to add a comment line e.g. "obey 'including any decoder scaling or padding' from SMPTE ST 377"?
>> I am not a core developer and will let others give their feedback. My personal opinion is that the spec is supposed to be well known and does not require commenting, but that it would be interesting to explicit why we make a difference between DV and MPEG2/AVC. And >personally, I don't have the answer to this question. If nobody has one, maybe a comment could say "because this is the observed common practice".
>>
>> Nicolas
>
> Some weeks later now and no replies, maybe time to go on ?
> I think the "case AV_CODEC_ID_DVVIDEO:" can be removed as discussed, fate updated and that should be ok for everybody.
> (Ideally, it could have been an opportunity to document why we have this "DV exception", but I understand it is not very comfortable to write as there is no meaningful reason, so forget about this, this won't hold up the patch anyway)
> For information, there was a long thread recently on ffmpeg-user about a "bug" in dnxhd stored_height (will be fixed with your patch):
> https://ffmpeg.org/pipermail/ffmpeg-user/2023-February/056111.html

Will apply the patch in a couple of days unless somebody objects. If you 
want to change DV height (seems reasonable), please send a follow up 
patch with fate updates after that.

Thanks,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] avformat/mxfenc: fix stored/sampled/displayed width/height

[Marton Balint]

On Fri, 10 Mar 2023, Marton Balint wrote:

>
>
> On Mon, 6 Mar 2023, Nicolas Gaullier wrote:
>
>>>>>  The width is one thing; for whatever reason, there is a divergence
>>>>>  between DV100 on one hand and AVCI/XDCAMHD35 on the other. In my
>>>>>  understanding, in current practices, DV obey s337 (stored width
>>>>>  includes scaling) but >xdcam&avci does not, so current code is fine
>>>>>  >but maybe this is an opportunity to document this ?
>>>>
>>>>  AFAIK:
>>>>  - DV in MXF: found old Omneon with no scaling for stored value, no
>>>>  sampled value (so stored value), scaled value for displayed value, old
>>>>  Quantel with scaling everywhere. From my understanding of spec, I would
>>>>  keep the scaling.
>>>>  - MPEG-2 Video including XDCAMHD35 in MXF obey "including any decoder
>>>>  scaling or padding" wording with a 16x16 rounding for height, I have no
>>>>  file not 1920 or 3840 width
>>>>  - AVC in MXF: found old Omneon or old Quantel  or old Telestream with no
>>>>  padding value for stored value (height of 540 for interlaced). I don't
>>>>  understand why it is not same as with MPEG-2 Video so I don't touch
>>>>  FFmpeg behavior >there (rounding). Actually checking >again SMPTE ST
>>>>  381-2013, there is an explicit example: "1088: 1080-line progressive".
>>>
>>>  I totally agree they are so many weird things in the wild, particularly
>>>  looking at some early implementations. I also have fully broken DV100 and
>>>  XDCAMHD35 Omneon records with release v6.1 (2010) at the early stages of
>>>  HD, but it was fixed afterwards (with many other >issues too!). Looking
>>>  at GVG, 1440x1088i stored size was implemented from the early beginnings
>>>  (2010 too) : sample clips are still available here :
>>>  http://www.gvgdevelopers.com/concrete/products/k2/test_clips/
>>>  There is also "kind of" reference sony implementation available here both
>>>  for xdcamhd35/avc-1440:
>>>  https://www.sonycreativesoftware.com/catalystbrowse
>>>  Anyway, I think we all agree not to change anything related to MPEG2 and
>>>  AVC.
>>>
>>>>  I don't have DV in MXF with non multiple of 16 (I thought that DV is
>>>>  only 720x576 or 720x480 or 1280x720 or 1920x1080, all values multiple of
>>>>  16) and don't know about video encoding in DV so I didn't want to change
>>>>  the behavior of FFmpeg when I don't know, but case AV_CODEC_ID_DVVIDEO:
>>>>  line could be definitely removed if it is fine for you.
>>>  DV is questionable. Currently, the dust is under the rug (as a defaults
>>>  behaviour), which is an issue with very little concern.
>>>  Now, with the patch, the dust become visible, the DV rule is made
>>>  explicit and moreover it is presented as an exception, sharing code with
>>>  macroblock codecs... I think it is time to fix, even if it was not your
>>>  prior intention.
>>>  I don't have an extensive experience with DV too, I just have samples
>>>  here and there like you, but it seems we share the same information.
>>>  Let see if someone else react and ask for keeping the current 1088 lines
>>>  for DV stored height, but if nobody does, I think it should be okay.
>>>
>>>>  Do you want me to add a comment line e.g. "obey 'including any decoder
>>>>  scaling or padding' from SMPTE ST 377"?
>>>  I am not a core developer and will let others give their feedback. My
>>>  personal opinion is that the spec is supposed to be well known and does
>>>  not require commenting, but that it would be interesting to explicit why
>>>  we make a difference between DV and MPEG2/AVC. And >personally, I don't
>>>  have the answer to this question. If nobody has one, maybe a comment
>>>  could say "because this is the observed common practice".
>>>
>>>  Nicolas
>>
>>  Some weeks later now and no replies, maybe time to go on ?
>>  I think the "case AV_CODEC_ID_DVVIDEO:" can be removed as discussed, fate
>>  updated and that should be ok for everybody.
>>  (Ideally, it could have been an opportunity to document why we have this
>>  "DV exception", but I understand it is not very comfortable to write as
>>  there is no meaningful reason, so forget about this, this won't hold up
>>  the patch anyway)
>>  For information, there was a long thread recently on ffmpeg-user about a
>>  "bug" in dnxhd stored_height (will be fixed with your patch):
>>  https://ffmpeg.org/pipermail/ffmpeg-user/2023-February/056111.html
>
> Will apply the patch in a couple of days unless somebody objects. If you want 
> to change DV height (seems reasonable), please send a follow up patch with 
> fate updates after that.

Oh, this patch needs a fate update as well. On that note, DNXHD is a 
macroblock-based codec, so why are we changing 1088 height to 1080? I 
could ask the same for ProRes. The patch should explain better why those 
need to be changed, if they do.

Thanks,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] avformat/mxfenc: fix stored/sampled/displayed width/height

[Pierre-Anthony Lemieux]

On Mon, Mar 13, 2023 at 3:30 PM Marton Balint <cus@passwd.hu> wrote:
>
>
>
> On Fri, 10 Mar 2023, Marton Balint wrote:
>
> >
> >
> > On Mon, 6 Mar 2023, Nicolas Gaullier wrote:
> >
> >>>>>  The width is one thing; for whatever reason, there is a divergence
> >>>>>  between DV100 on one hand and AVCI/XDCAMHD35 on the other. In my
> >>>>>  understanding, in current practices, DV obey s337 (stored width
> >>>>>  includes scaling) but >xdcam&avci does not, so current code is fine
> >>>>>  >but maybe this is an opportunity to document this ?
> >>>>
> >>>>  AFAIK:
> >>>>  - DV in MXF: found old Omneon with no scaling for stored value, no
> >>>>  sampled value (so stored value), scaled value for displayed value, old
> >>>>  Quantel with scaling everywhere. From my understanding of spec, I would
> >>>>  keep the scaling.
> >>>>  - MPEG-2 Video including XDCAMHD35 in MXF obey "including any decoder
> >>>>  scaling or padding" wording with a 16x16 rounding for height, I have no
> >>>>  file not 1920 or 3840 width
> >>>>  - AVC in MXF: found old Omneon or old Quantel  or old Telestream with no
> >>>>  padding value for stored value (height of 540 for interlaced). I don't
> >>>>  understand why it is not same as with MPEG-2 Video so I don't touch
> >>>>  FFmpeg behavior >there (rounding). Actually checking >again SMPTE ST
> >>>>  381-2013, there is an explicit example: "1088: 1080-line progressive".
> >>>
> >>>  I totally agree they are so many weird things in the wild, particularly
> >>>  looking at some early implementations. I also have fully broken DV100 and
> >>>  XDCAMHD35 Omneon records with release v6.1 (2010) at the early stages of
> >>>  HD, but it was fixed afterwards (with many other >issues too!). Looking
> >>>  at GVG, 1440x1088i stored size was implemented from the early beginnings
> >>>  (2010 too) : sample clips are still available here :
> >>>  http://www.gvgdevelopers.com/concrete/products/k2/test_clips/
> >>>  There is also "kind of" reference sony implementation available here both
> >>>  for xdcamhd35/avc-1440:
> >>>  https://www.sonycreativesoftware.com/catalystbrowse
> >>>  Anyway, I think we all agree not to change anything related to MPEG2 and
> >>>  AVC.
> >>>
> >>>>  I don't have DV in MXF with non multiple of 16 (I thought that DV is
> >>>>  only 720x576 or 720x480 or 1280x720 or 1920x1080, all values multiple of
> >>>>  16) and don't know about video encoding in DV so I didn't want to change
> >>>>  the behavior of FFmpeg when I don't know, but case AV_CODEC_ID_DVVIDEO:
> >>>>  line could be definitely removed if it is fine for you.
> >>>  DV is questionable. Currently, the dust is under the rug (as a defaults
> >>>  behaviour), which is an issue with very little concern.
> >>>  Now, with the patch, the dust become visible, the DV rule is made
> >>>  explicit and moreover it is presented as an exception, sharing code with
> >>>  macroblock codecs... I think it is time to fix, even if it was not your
> >>>  prior intention.
> >>>  I don't have an extensive experience with DV too, I just have samples
> >>>  here and there like you, but it seems we share the same information.
> >>>  Let see if someone else react and ask for keeping the current 1088 lines
> >>>  for DV stored height, but if nobody does, I think it should be okay.
> >>>
> >>>>  Do you want me to add a comment line e.g. "obey 'including any decoder
> >>>>  scaling or padding' from SMPTE ST 377"?
> >>>  I am not a core developer and will let others give their feedback. My
> >>>  personal opinion is that the spec is supposed to be well known and does
> >>>  not require commenting, but that it would be interesting to explicit why
> >>>  we make a difference between DV and MPEG2/AVC. And >personally, I don't
> >>>  have the answer to this question. If nobody has one, maybe a comment
> >>>  could say "because this is the observed common practice".
> >>>
> >>>  Nicolas
> >>
> >>  Some weeks later now and no replies, maybe time to go on ?
> >>  I think the "case AV_CODEC_ID_DVVIDEO:" can be removed as discussed, fate
> >>  updated and that should be ok for everybody.
> >>  (Ideally, it could have been an opportunity to document why we have this
> >>  "DV exception", but I understand it is not very comfortable to write as
> >>  there is no meaningful reason, so forget about this, this won't hold up
> >>  the patch anyway)
> >>  For information, there was a long thread recently on ffmpeg-user about a
> >>  "bug" in dnxhd stored_height (will be fixed with your patch):
> >>  https://ffmpeg.org/pipermail/ffmpeg-user/2023-February/056111.html
> >
> > Will apply the patch in a couple of days unless somebody objects. If you want
> > to change DV height (seems reasonable), please send a follow up patch with
> > fate updates after that.
>
> Oh, this patch needs a fate update as well. On that note, DNXHD is a
> macroblock-based codec, so why are we changing 1088 height to 1080? I
> could ask the same for ProRes. The patch should explain better why those
> need to be changed, if they do.

FYI. The relationship between stored height/width and the
corresponding Prores parameters (horizontal_size, vertical_size and
bottomFieldVerticalSize) is specified in SMPTE RDD 44.

>
> Thanks,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] avformat/mxfenc: fix stored/sampled/displayed width/height

[Jerome Martinez]

[-- Attachment #1: Type: text/plain, Size: 1352 bytes --]

On 10/03/2023 22:10, Marton Balint wrote:
>
>
> On Mon, 6 Mar 2023, Nicolas Gaullier wrote:
>
> [...]
>>
>> Some weeks later now and no replies, maybe time to go on ?
>> I think the "case AV_CODEC_ID_DVVIDEO:" can be removed as discussed, 
>> fate updated and that should be ok for everybody.
>> (Ideally, it could have been an opportunity to document why we have 
>> this "DV exception", but I understand it is not very comfortable to 
>> write as there is no meaningful reason, so forget about this, this 
>> won't hold up the patch anyway)
>> For information, there was a long thread recently on ffmpeg-user 
>> about a "bug" in dnxhd stored_height (will be fixed with your patch):
>> https://ffmpeg.org/pipermail/ffmpeg-user/2023-February/056111.html
>
> Will apply the patch in a couple of days unless somebody objects. If 
> you want to change DV height (seems reasonable), please send a follow 
> up patch with fate updates after that.


Apologizes for the huge delay.
Attached is an updated patch.

I changed the DV part (also removed from the 16x16 macroblock thing)
I added some comments about specs (summary: DNxHD is explicit, others 
are not but implementation I know don't do the 16x16 macroblock thing).
The FATE tests for DV are not impacted because they are SD and SD 
width/height are multiple of 16 so I added a DV100 test.

Jérôme

[-- Attachment #2: 0001-avformat-mxfenc-fix-stored-sampled-displayed-width-h.patch --]
[-- Type: text/plain, Size: 6332 bytes --]

From c93c73164d427b2bcd29744b5a26ab82b1fe223a Mon Sep 17 00:00:00 2001
From: Jerome Martinez <jerome@mediaarea.net>
Date: Sat, 14 Jan 2023 13:32:36 +0100
Subject: [PATCH] avformat/mxfenc: fix stored/sampled/displayed width/height

Stored values are rounded to upper 16 multiple only for MPEG related formats (DV is not 16x16 macroblocks based, RDD 44 only refers to ST 377-1 without precision and no known implementation puts 1088 or 544, ST 2019-4 explicitly indicates 1080 for 1080p and 540 for 1080i)
Sampled and displayed widths are codecpar ones (with DV exception)
---
 libavformat/mxfenc.c          | 27 +++++++++++++++++++++------
 tests/fate/lavf-container.mak |  3 ++-
 tests/ref/lavf/mxf_dvcpro100  |  3 +++
 tests/ref/lavf/mxf_opatom     |  2 +-
 4 files changed, 27 insertions(+), 8 deletions(-)
 create mode 100644 tests/ref/lavf/mxf_dvcpro100

diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index 58c551c83c..614fc5ec89 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -1109,8 +1109,9 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
 {
     MXFStreamContext *sc = st->priv_data;
     AVIOContext *pb = s->pb;
-    int stored_width = 0;
-    int stored_height = (st->codecpar->height+15)/16*16;
+    int stored_width = st->codecpar->width;
+    int stored_height = st->codecpar->height;
+    int display_width;
     int display_height;
     int f1, f2;
     const MXFCodecUL *color_primaries_ul;
@@ -1129,12 +1130,24 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
         else if (st->codecpar->height == 720)
             stored_width = 1280;
     }
-    if (!stored_width)
-        stored_width = (st->codecpar->width+15)/16*16;
+    display_width = stored_width;
 
+    switch (st->codecpar->codec_id) {
+    case AV_CODEC_ID_MPEG2VIDEO:
+    case AV_CODEC_ID_H264:
+        //Based on 16x16 macroblocks
+        stored_width = (stored_width+15)/16*16;
+        stored_height = (stored_height+15)/16*16;
+        break;
+    default:
+        break;
+    }
+
+    //Stored width
     mxf_write_local_tag(s, 4, 0x3203);
     avio_wb32(pb, stored_width);
 
+    //Stored height
     mxf_write_local_tag(s, 4, 0x3202);
     avio_wb32(pb, stored_height>>sc->interlaced);
 
@@ -1154,7 +1167,7 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
 
     //Sampled width
     mxf_write_local_tag(s, 4, 0x3205);
-    avio_wb32(pb, stored_width);
+    avio_wb32(pb, display_width);
 
     //Samples height
     mxf_write_local_tag(s, 4, 0x3204);
@@ -1168,8 +1181,9 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
     mxf_write_local_tag(s, 4, 0x3207);
     avio_wb32(pb, 0);
 
+    //Display width
     mxf_write_local_tag(s, 4, 0x3209);
-    avio_wb32(pb, stored_width);
+    avio_wb32(pb, display_width);
 
     if (st->codecpar->height == 608) // PAL + VBI
         display_height = 576;
@@ -1178,6 +1192,7 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
     else
         display_height = st->codecpar->height;
 
+    //Display height
     mxf_write_local_tag(s, 4, 0x3208);
     avio_wb32(pb, display_height>>sc->interlaced);
 
diff --git a/tests/fate/lavf-container.mak b/tests/fate/lavf-container.mak
index 4bf7636b56..a04d31156a 100644
--- a/tests/fate/lavf-container.mak
+++ b/tests/fate/lavf-container.mak
@@ -8,7 +8,7 @@ FATE_LAVF_CONTAINER-$(call ENCDEC2, MPEG4,      MP2,       MATROSKA)           +
 FATE_LAVF_CONTAINER-$(call ENCDEC2, MPEG4,      PCM_ALAW,  MOV)                += mov mov_rtphint ismv
 FATE_LAVF_CONTAINER-$(call ENCDEC,  MPEG4,                 MOV)                += mp4
 FATE_LAVF_CONTAINER-$(call ENCDEC2, MPEG1VIDEO, MP2,       MPEG1SYSTEM MPEGPS) += mpg
-FATE_LAVF_CONTAINER-$(call ENCDEC2, MPEG2VIDEO, PCM_S16LE, MXF)                += mxf mxf_dv25 mxf_dvcpro50
+FATE_LAVF_CONTAINER-$(call ENCDEC2, MPEG2VIDEO, PCM_S16LE, MXF)                += mxf mxf_dv25 mxf_dvcpro50 mxf_dvcpro100
 FATE_LAVF_CONTAINER-$(call ENCDEC2, MPEG2VIDEO, PCM_S16LE, MXF_D10 MXF)        += mxf_d10
 FATE_LAVF_CONTAINER-$(call ENCDEC2, DNXHD,      PCM_S16LE, MXF_OPATOM MXF)     += mxf_opatom mxf_opatom_audio
 FATE_LAVF_CONTAINER-$(call ENCDEC2, MPEG4,      MP2,       NUT)                += nut
@@ -55,6 +55,7 @@ fate-lavf-mxf: CMD = lavf_container_timecode "-ar 48000 -bf 2 -threads 1"
 fate-lavf-mxf_d10: CMD = lavf_container "-ar 48000 -ac 2" "-r 25 -vf scale=720:576,pad=720:608:0:32 -c:v mpeg2video -g 0 -flags +ildct+low_delay -dc 10 -non_linear_quant 1 -intra_vlc 1 -qscale 1 -ps 1 -qmin 1 -rc_max_vbv_use 1 -rc_min_vbv_use 1 -pix_fmt yuv422p -minrate 30000k -maxrate 30000k -b 30000k -bufsize 1200000 -top 1 -rc_init_occupancy 1200000 -qmax 12 -f mxf_d10"
 fate-lavf-mxf_dv25: CMD = lavf_container "-ar 48000 -ac 2" "-r 25 -vf scale=720:576,setdar=4/3 -c:v dvvideo -pix_fmt yuv420p -b 25000k -top 0 -f mxf"
 fate-lavf-mxf_dvcpro50: CMD = lavf_container "-ar 48000 -ac 2" "-r 25 -vf scale=720:576,setdar=16/9 -c:v dvvideo -pix_fmt yuv422p -b 50000k -top 0 -f mxf"
+fate-lavf-mxf_dvcpro100: CMD = lavf_container "-ar 48000 -ac 2" "-r 25 -vf scale=1440:1080,setdar=16/9 -c:v dvvideo -pix_fmt yuv422p -b 100000k -top 0 -f mxf"
 fate-lavf-mxf_opatom: CMD = lavf_container "" "-s 1920x1080 -c:v dnxhd -pix_fmt yuv422p -vb 36M -f mxf_opatom -map 0"
 fate-lavf-mxf_opatom_audio: CMD = lavf_container "-ar 48000 -ac 1" "-f mxf_opatom -mxf_audio_edit_rate 25 -map 1"
 fate-lavf-smjpeg:  CMD = lavf_container "" "-f smjpeg"
diff --git a/tests/ref/lavf/mxf_dvcpro100 b/tests/ref/lavf/mxf_dvcpro100
new file mode 100644
index 0000000000..8193828e2c
--- /dev/null
+++ b/tests/ref/lavf/mxf_dvcpro100
@@ -0,0 +1,3 @@
+efa5cdde54ac38b02827ee8b6d7f03a4 *tests/data/lavf/lavf.mxf_dvcpro100
+14637613 tests/data/lavf/lavf.mxf_dvcpro100
+tests/data/lavf/lavf.mxf_dvcpro100 CRC=0x245e9a5f
diff --git a/tests/ref/lavf/mxf_opatom b/tests/ref/lavf/mxf_opatom
index 75f85b604e..fa72241813 100644
--- a/tests/ref/lavf/mxf_opatom
+++ b/tests/ref/lavf/mxf_opatom
@@ -1,3 +1,3 @@
-215ea72602bfeb70e99fc9d79fef073c *tests/data/lavf/lavf.mxf_opatom
+6418766ca9b8b23fae74a80d66f26f3e *tests/data/lavf/lavf.mxf_opatom
 4717625 tests/data/lavf/lavf.mxf_opatom
 tests/data/lavf/lavf.mxf_opatom CRC=0xb13ba2f8
-- 
2.13.3.windows.1


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] avformat/mxfenc: fix stored/sampled/displayed width/height

[Marton Balint]

On Tue, 14 Mar 2023, Jerome Martinez wrote:

> On 10/03/2023 22:10, Marton Balint wrote:
>> 
>>
>>  On Mon, 6 Mar 2023, Nicolas Gaullier wrote:
>>
>>  [...]
>>>
>>>  Some weeks later now and no replies, maybe time to go on ?
>>>  I think the "case AV_CODEC_ID_DVVIDEO:" can be removed as discussed, fate
>>>  updated and that should be ok for everybody.
>>>  (Ideally, it could have been an opportunity to document why we have this
>>>  "DV exception", but I understand it is not very comfortable to write as
>>>  there is no meaningful reason, so forget about this, this won't hold up
>>>  the patch anyway)
>>>  For information, there was a long thread recently on ffmpeg-user about a
>>>  "bug" in dnxhd stored_height (will be fixed with your patch):
>>>  https://ffmpeg.org/pipermail/ffmpeg-user/2023-February/056111.html
>>
>>  Will apply the patch in a couple of days unless somebody objects. If you
>>  want to change DV height (seems reasonable), please send a follow up patch
>>  with fate updates after that.
>
>
> Apologizes for the huge delay.
> Attached is an updated patch.
>
> I changed the DV part (also removed from the 16x16 macroblock thing)
> I added some comments about specs (summary: DNxHD is explicit, others are not 
> but implementation I know don't do the 16x16 macroblock thing).
> The FATE tests for DV are not impacted because they are SD and SD 
> width/height are multiple of 16 so I added a DV100 test.

Thanks, will apply with some extended commit description.

Regards,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".