diff mbox

[FFmpeg-devel] avformat/mxfenc: correctly set width values for dvcprohd

Message ID 20191109234652.16763-1-baptiste.coudurier@gmail.com
State Accepted
Commit 11a38be99cd0c6685a65940cf0c9114d338235a3
Headers show

Commit Message

Baptiste Coudurier Nov. 9, 2019, 11:46 p.m. UTC 
---
 libavformat/mxfenc.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

Tomas Härdin Nov. 11, 2019, 11:30 p.m. UTC | #1 
lör 2019-11-09 klockan 15:46 -0800 skrev Baptiste Coudurier:
> ---
>  libavformat/mxfenc.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index b7ae5cc637..f7df9c3daf 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -1092,7 +1092,7 @@ 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  = (st->codecpar->width +15)/16*16;
> +    int stored_width = 0;
>      int stored_height = (st->codecpar->height+15)/16*16;
>      int display_height;
>      int f1, f2;
> @@ -1101,6 +1101,15 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
>  
>      get_trc(transfer_ul, st->codecpar->color_trc);
>  
> +    if (st->codecpar->codec_id == AV_CODEC_ID_DVVIDEO) {
> +        if (st->codecpar->height == 1080)
> +            stored_width = 1920;
> +        else if (st->codecpar->height == 720)
> +            stored_width = 1280;

What about the other DV variants?

> +    }
> +    if (!stored_width)
> +        stored_width = (st->codecpar->width+15)/16*16;

Shouldn't this have SAR applied?

/Tomas
Baptiste Coudurier Nov. 11, 2019, 11:52 p.m. UTC | #2 
> On Nov 11, 2019, at 3:30 PM, Tomas Härdin <tjoppen@acc.umu.se> wrote:
> 
> lör 2019-11-09 klockan 15:46 -0800 skrev Baptiste Coudurier:
>> ---
>> libavformat/mxfenc.c | 15 ++++++++++++---
>> 1 file changed, 12 insertions(+), 3 deletions(-)
>> 
>> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
>> index b7ae5cc637..f7df9c3daf 100644
>> --- a/libavformat/mxfenc.c
>> +++ b/libavformat/mxfenc.c
>> @@ -1092,7 +1092,7 @@ 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  = (st->codecpar->width +15)/16*16;
>> +    int stored_width = 0;
>>     int stored_height = (st->codecpar->height+15)/16*16;
>>     int display_height;
>>     int f1, f2;
>> @@ -1101,6 +1101,15 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
>> 
>>     get_trc(transfer_ul, st->codecpar->color_trc);
>> 
>> +    if (st->codecpar->codec_id == AV_CODEC_ID_DVVIDEO) {
>> +        if (st->codecpar->height == 1080)
>> +            stored_width = 1920;
>> +        else if (st->codecpar->height == 720)
>> +            stored_width = 1280;
> 
> What about the other DV variants?

These get default value

>> +    }
>> +    if (!stored_width)
>> +        stored_width = (st->codecpar->width+15)/16*16;
> 
> Shouldn't this have SAR applied?

I didn’t change this, the code was like that before

— 
Baptiste
Baptiste Coudurier Nov. 13, 2019, 5:08 p.m. UTC | #3 
> On Nov 12, 2019, at 1:14 PM, Baptiste Coudurier <baptiste.coudurier@gmail.com> wrote:
> 
>> 
>> On Nov 11, 2019, at 3:30 PM, Tomas Härdin <tjoppen@acc.umu.se <mailto:tjoppen@acc.umu.se>> wrote:
>> 
>> lör 2019-11-09 klockan 15:46 -0800 skrev Baptiste Coudurier:
>>> ---
>>> libavformat/mxfenc.c | 15 ++++++++++++---
>>> 1 file changed, 12 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
>>> index b7ae5cc637..f7df9c3daf 100644
>>> --- a/libavformat/mxfenc.c
>>> +++ b/libavformat/mxfenc.c
>>> @@ -1092,7 +1092,7 @@ 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  = (st->codecpar->width +15)/16*16;
>>> +    int stored_width = 0;
>>>     int stored_height = (st->codecpar->height+15)/16*16;
>>>     int display_height;
>>>     int f1, f2;
>>> @@ -1101,6 +1101,15 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
>>> 
>>>     get_trc(transfer_ul, st->codecpar->color_trc);
>>> 
>>> +    if (st->codecpar->codec_id == AV_CODEC_ID_DVVIDEO) {
>>> +        if (st->codecpar->height == 1080)
>>> +            stored_width = 1920;
>>> +        else if (st->codecpar->height == 720)
>>> +            stored_width = 1280;
>> 
>> What about the other DV variants?
> 
> These get default value
> 
>>> +    }
>>> +    if (!stored_width)
>>> +        stored_width = (st->codecpar->width+15)/16*16;
>> 
>> Shouldn't this have SAR applied?
> 
> I didn’t change this, the code was like that before
> 

Applied

— 
Baptiste
Tomas Härdin Nov. 13, 2019, 8:03 p.m. UTC | #4 
mån 2019-11-11 klockan 15:52 -0800 skrev Baptiste Coudurier:
> > On Nov 11, 2019, at 3:30 PM, Tomas Härdin <tjoppen@acc.umu.se> wrote:
> > 
> > lör 2019-11-09 klockan 15:46 -0800 skrev Baptiste Coudurier:
> > > ---
> > > libavformat/mxfenc.c | 15 ++++++++++++---
> > > 1 file changed, 12 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> > > index b7ae5cc637..f7df9c3daf 100644
> > > --- a/libavformat/mxfenc.c
> > > +++ b/libavformat/mxfenc.c
> > > @@ -1092,7 +1092,7 @@ 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  = (st->codecpar->width +15)/16*16;
> > > +    int stored_width = 0;
> > >     int stored_height = (st->codecpar->height+15)/16*16;
> > >     int display_height;
> > >     int f1, f2;
> > > @@ -1101,6 +1101,15 @@ static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
> > > 
> > >     get_trc(transfer_ul, st->codecpar->color_trc);
> > > 
> > > +    if (st->codecpar->codec_id == AV_CODEC_ID_DVVIDEO) {
> > > +        if (st->codecpar->height == 1080)
> > > +            stored_width = 1920;
> > > +        else if (st->codecpar->height == 720)
> > > +            stored_width = 1280;
> > 
> > What about the other DV variants?
> 
> These get default value
> 
> > > +    }
> > > +    if (!stored_width)
> > > +        stored_width = (st->codecpar->width+15)/16*16;
> > 
> > Shouldn't this have SAR applied?
> 
> I didn’t change this, the code was like that before

Right. I must say S377m is very confusing when it comes to the
difference between these rectangles tbh

/Tomas
diff mbox

Patch

diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index b7ae5cc637..f7df9c3daf 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -1092,7 +1092,7 @@  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  = (st->codecpar->width +15)/16*16;
+    int stored_width = 0;
     int stored_height = (st->codecpar->height+15)/16*16;
     int display_height;
     int f1, f2;
@@ -1101,6 +1101,15 @@  static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
 
     get_trc(transfer_ul, st->codecpar->color_trc);
 
+    if (st->codecpar->codec_id == AV_CODEC_ID_DVVIDEO) {
+        if (st->codecpar->height == 1080)
+            stored_width = 1920;
+        else if (st->codecpar->height == 720)
+            stored_width = 1280;
+    }
+    if (!stored_width)
+        stored_width = (st->codecpar->width+15)/16*16;
+
     mxf_write_local_tag(pb, 4, 0x3203);
     avio_wb32(pb, stored_width);
 
@@ -1123,7 +1132,7 @@  static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
 
     //Sampled width
     mxf_write_local_tag(pb, 4, 0x3205);
-    avio_wb32(pb, st->codecpar->width);
+    avio_wb32(pb, stored_width);
 
     //Samples height
     mxf_write_local_tag(pb, 4, 0x3204);
@@ -1138,7 +1147,7 @@  static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
     avio_wb32(pb, 0);
 
     mxf_write_local_tag(pb, 4, 0x3209);
-    avio_wb32(pb, st->codecpar->width);
+    avio_wb32(pb, stored_width);
 
     if (st->codecpar->height == 608) // PAL + VBI
         display_height = 576;