diff mbox series

[FFmpeg-devel] mfxenc add jpeg2000 frame field interlacing support. SPONSORED BY INA (Institut National de l'Audiovisuel) * the use of a jpeg2000 frame/field input is signaled by the option flag mxf_j2kinterlace * according to smpte 0400-2012, the codec d

Message ID b7eae57a-6fa3-49f4-a184-26154b3ce006@NUC42.local
State New
Headers show
Series [FFmpeg-devel] mfxenc add jpeg2000 frame field interlacing support. SPONSORED BY INA (Institut National de l'Audiovisuel) * the use of a jpeg2000 frame/field input is signaled by the option flag mxf_j2kinterlace * according to smpte 0400-2012, the codec d | expand

Checks

Context Check Description
andriy/commit_msg_x86 warning The first line of the commit message is way too long.
andriy/make_x86 success Make finished
andriy/make_fate_x86 fail Make fate failed
andriy/commit_msg_ppc warning The first line of the commit message is way too long.
andriy/make_ppc success Make finished
andriy/make_fate_ppc fail Make fate failed

Commit Message

Erwann RENAN Oct. 28, 2021, 2:23 p.m. UTC 
---
 libavformat/mxf.h    |   1 +
 libavformat/mxfenc.c | 371 +++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 361 insertions(+), 11 deletions(-)

Comments

Tomas Härdin Oct. 28, 2021, 2:32 p.m. UTC | #1 
Looks like you messed up the subject. You need two newlines between the
title of the patch and the description.

This patch also mixes a lot of different changes. Please split it up.
The bigger a patch is the more rounds of review it tends to have to go
through.

> +    { 0x840B,
> {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x0B,0x0
> 0,0x00,0x00}}, /* 8+3n bytes : Array of picture components where each
> component comprises 3 bytes named Ssizi, XRSizi, YRSizi  The array of
> 3-byte groups is preceded by the array header comprising a 4-byte
> value of the number of components followed by a 4-byte value of �3�.
> */

some kind of encoding problem in the comment

> @@ -843,7 +886,28 @@ static void mxf_write_track(AVFormatContext *s,
> AVStream *st, MXFPackage *packag
>  
>      mxf_write_metadata_key(pb, 0x013b00);
>      PRINT_KEY(s, "track key", pb->buf_ptr - 16);
> -    klv_encode_ber_length(pb, 80);
> +
> +    if (st->codecpar) {
> +        static const char * pcTrackName_Video = "Picture" ;
> +        static const char * pcTrackName_Audio = "Sound" ;
> +        static const char * pcTrackName_Anc = "Ancillary" ;

static is redundant here

> +        if ( st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO )
> +        {
> +            //TrackName Video
> +            klv_encode_ber_length(pb, 80 +
> mxf_utf16_local_tag_length(pcTrackName_Video));
> +            mxf_write_local_tag_utf16(s, 0x4802 ,
> pcTrackName_Video);
> +        } else if ( st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO )
> {
> +            //TrackName Audio
> +            klv_encode_ber_length(pb, 80 +
> mxf_utf16_local_tag_length(pcTrackName_Audio));
> +            mxf_write_local_tag_utf16(s, 0x4802, pcTrackName_Audio);
> +        } else {
> +            //TrackName Ancillary
> +            klv_encode_ber_length(pb, 80 +
> mxf_utf16_local_tag_length(pcTrackName_Anc));
> +            mxf_write_local_tag_utf16(s, 0x4802, pcTrackName_Anc);
> +        }
> +    } else {
> +        klv_encode_ber_length(pb, 80);
> +    }

Is this hunk really necessary?

> @@ -1327,6 +1420,182 @@ static int64_t
> mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
>      return pos;
>  }
>  
> +static int64_t mxf_write_jpeg2000_common(AVFormatContext *s,
> AVStream *st, const UID key)
> +{
> +    MXFStreamContext *sc = st->priv_data;
> +    MXFContext *mxf = s->priv_data;
> +    AVIOContext *pb = s->pb;
> +    int stored_width = st->codecpar->width;
> +    int stored_height = st->codecpar->height;
> +    int display_height;
> +    int f1, f2;
> +    UID transfer_ul = {0};
> +    int64_t pos = mxf_write_generic_desc(s, st, key);
> +    get_trc(transfer_ul, st->codecpar->color_trc);

Return value of get_trc() is not checked. Not sure if invalid values
can ever get in here.

> +
> +    mxf_write_local_tag(s, 4, 0x3203);
> +    avio_wb32(pb, stored_width);
> +    mxf_write_local_tag(s, 4, 0x3202);
> +    avio_wb32(pb, stored_height);
> +
> +    //Sampled width
> +    mxf_write_local_tag(s, 4, 0x3205);
> +    avio_wb32(pb, st->codecpar->width);
> +
> +    //Samples height
> +    mxf_write_local_tag(s, 4, 0x3204);
> +    avio_wb32(pb, st->codecpar->height);

Why use stored_* and codecpar->*? Just use codecpar->* in both places.

> +
> +    //Sampled X Offset
> +    mxf_write_local_tag(s, 4, 0x3206);
> +    avio_wb32(pb, 0);
> +
> +    //Sampled Y Offset
> +    mxf_write_local_tag(s, 4, 0x3207);
> +    avio_wb32(pb, 0);
> +
> +    mxf_write_local_tag(s, 4, 0x3209);
> +    avio_wb32(pb, st->codecpar->width);
> +
> +    if (st->codecpar->height == 608) // PAL + VBI
> +        display_height = 576;
> +    else if (st->codecpar->height == 512)  // NTSC + VBI
> +        display_height = 486;
> +    else
> +        display_height = st->codecpar->height;
> +
> +    mxf_write_local_tag(s, 4, 0x3208);
> +    avio_wb32(pb, display_height);
> +
> +    // display X offset
> +    mxf_write_local_tag(s, 4, 0x320A);
> +    avio_wb32(pb, 0);
> +
> +    //display Y offset
> +    mxf_write_local_tag(s, 4, 0x320B);
> +    avio_wb32(pb, (st->codecpar->height - display_height));

Would be better if we could get this information via metadata instead
of adding hacks to the muxer

> +    // // Padding Bits
> +    // mxf_write_local_tag(s, 2, 0x3307);
> +    // avio_wb16(pb, 0);

Stray dead code

> +    // video line map
> +    {
> +        int _field_height = (mxf->mxf_j2kinterlace) ? (2*st-
> >codecpar->height) : st->codecpar->height;
> +
> +        if (st->codecpar->sample_aspect_ratio.num && st->codecpar-
> >sample_aspect_ratio.den) {
> +            AVRational _ratio = av_mul_q(st->codecpar-
> >sample_aspect_ratio,
> +                               av_make_q(st->codecpar->width, st-
> >codecpar->height));
> +            sc->aspect_ratio = _ratio;
> +
> +            switch (_field_height) {
> +                case  576: f1 = 23; f2 = st->codecpar->codec_id ==
> AV_CODEC_ID_DVVIDEO ? 335 : 336; break;
> +                case  608: f1 =  7; f2 = 320; break;
> +                case  480: f1 = 20; f2 = st->codecpar->codec_id ==
> AV_CODEC_ID_DVVIDEO ? 285 : 283; break;
> +                case  512: f1 =  7; f2 = 270; break;
> +                case  720: f1 = 26; f2 =   0; break; // progressive
> +                case 1080: f1 = 21; f2 = 584; break;
> +                default:   f1 =  0; f2 =   0; break;
> +            }
> +        } else {
> +            switch (_field_height) {
> +                case  576: sc->aspect_ratio = (AVRational){  4,  3};
> f1 = 23; f2 = st->codecpar->codec_id == AV_CODEC_ID_DVVIDEO ? 335 :
> 336; break;
> +                case  608: sc->aspect_ratio = (AVRational){  4,  3};
> f1 =  7; f2 = 320; break;
> +                case  480: sc->aspect_ratio = (AVRational){  4,  3};
> f1 = 20; f2 = st->codecpar->codec_id == AV_CODEC_ID_DVVIDEO ? 285 :
> 283; break;
> +                case  512: sc->aspect_ratio = (AVRational){  4,  3};
> f1 =  7; f2 = 270; break;
> +                case  720: sc->aspect_ratio = (AVRational){  16, 
> 9}; f1 = 26; f2 =   0; break; // progressive
> +                case 1080: sc->aspect_ratio = (AVRational){  16, 
> 9}; f1 = 21; f2 = 584; break;
> +                default:   f1 =  0; f2 =   0; break;
> +            }
> +        }
> +    }

This again feels like business logic that belongs in ffmpeg.c. I
imagine this applies not just to J2K and not just to MXF. Remuxing MXF
to MOV for example.

> +
> +    if (!sc->interlaced && f2) {
> +        f2  = 0;
> +        f1 *= 2;
> +    }

This looks.. interesting.

> +    mxf_write_local_tag(s, 2, 0x8401);
> +    avio_wb16(pb, 0x0000);
> +    mxf_write_local_tag(s, 4, 0x8402);
> +    avio_wb32(pb, st->codecpar->width);
> +    mxf_write_local_tag(s, 4, 0x8403);
> +    avio_wb32(pb, st->codecpar->height);
> +    mxf_write_local_tag(s, 4, 0x8404);
> +    avio_wb32(pb, 0);
> +    mxf_write_local_tag(s, 4, 0x8405);
> +    avio_wb32(pb, 0);
> +    mxf_write_local_tag(s, 4, 0x8406);
> +    avio_wb32(pb, st->codecpar->width);
> +    mxf_write_local_tag(s, 4, 0x8407);
> +    avio_wb32(pb, st->codecpar->height);
> +    mxf_write_local_tag(s, 4, 0x8408);
> +    avio_wb32(pb, 0);
> +    mxf_write_local_tag(s, 4, 0x8409);
> +    avio_wb32(pb, 0);
> +    mxf_write_local_tag(s, 2, 0x840A);
> +    avio_wb16(pb, component_count);

Please comment these, to the right of each mxf_write_local_tag() is
fine.

> +
> +    mxf_write_local_tag(s, 8 + 3*component_count, 0x840B);
> +    avio_wb32(pb, component_count);
> +    avio_wb32(pb, 3);
> +    {
> +        char _desc [3][3]= {  {0x09,0x01,0x01} , {0x09,0x02,0x01} ,
> {0x09,0x02,0x01} };                  
> +        int comp = 0;
> +        for ( comp = 0; comp< component_count ;comp++ ) {
> +            avio_write(pb, _desc[comp%3] , 3);
> +        }
> +    }

FFmpeg is C99, braces like these are not necessary. You could also do
this as a single 9-byte avio_write(). You could even have the data
inline.

> +    mxf_write_local_tag(s, 16, 0x840C);
> +    {
> +        char _layout[16] = {  'Y' , '\n', 'U' , '\n', 'V' , '\n',
> 'F' , 0x02,
> +                            0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00 };
> +        avio_write(pb, _layout , 16);
> +    }

Same here.

> @@ -1729,7 +2058,7 @@ static int
> mxf_write_header_metadata_sets(AVFormatContext *s)
>  static unsigned klv_fill_size(uint64_t size)
>  {
>      unsigned pad = KAG_SIZE - (size & (KAG_SIZE-1));
> -    if (pad < 20) // smallest fill item possible
> +    if (pad <= 20) // smallest fill item possible
>          return pad + KAG_SIZE;

This should *definitely* be its own patch. Also the existing code is
correct unless I'm missing something majorly wrong. A zero-length ber4
KLV is 20 bytes.

>      else
>          return pad & (KAG_SIZE-1);
> @@ -2762,7 +3091,13 @@ static void
> mxf_write_system_item(AVFormatContext *s)
>      avio_wb64(pb, 0); // creation date/time stamp
>  
>      avio_w8(pb, 0x81); // SMPTE 12M time code
> -    time_code = av_timecode_get_smpte_from_framenum(&mxf->tc,
> frame);
> +    if ( 0 == mxf->mxf_j2kinterlace ) {
> +        time_code = av_timecode_get_smpte_from_framenum(&mxf->tc,
> frame);
> +    } else {
> +        unsigned int _myframenum = frame>>1;
> +        time_code = av_timecode_get_smpte_from_framenum(&mxf->tc,
> _myframenum);
> +    }

This looks.. fun.

> +
>      avio_wb32(pb, time_code);
>      avio_wb32(pb, 0); // binary group data
>      avio_wb64(pb, 0);
> @@ -2928,6 +3263,12 @@ static int mxf_write_packet(AVFormatContext
> *s, AVPacket *pkt)
>              av_log(s, AV_LOG_ERROR, "could not get h264 profile\n");
>              return -1;
>          }
> +    } else if (st->codecpar->codec_id == AV_CODEC_ID_JPEG2000) {
> +        if (mxf->mxf_j2kinterlace) {
> +            st->codecpar->field_order = AV_FIELD_TT;
> +            sc->interlaced = 1;
> +            sc->field_dominance = 1;

Aren't these settable via ffmpeg?

> @@ -2960,11 +3301,13 @@ static int mxf_write_packet(AVFormatContext
> *s, AVPacket *pkt)
>          if (!mxf->edit_unit_byte_count &&
>              (!mxf->edit_units_count || mxf->edit_units_count >
> EDIT_UNITS_PER_BODY) &&
>              !(ie.flags & 0x33)) { // I-frame, GOP start
> -            mxf_write_klv_fill(s);
> -            if ((err = mxf_write_partition(s, 1, 2,
> body_partition_key, 0)) < 0)
> -                return err;
> -            mxf_write_klv_fill(s);
> -            mxf_write_index_table_segment(s);
> +            if (!mxf->mxf_nobody) {

Maybe rename to mxf_no_body

> @@ -3198,6 +3541,12 @@ static const AVOption mxf_options[] = {
>      MXF_COMMON_OPTIONS
>      { "store_user_comments", "",
>        offsetof(MXFContext, store_user_comments), AV_OPT_TYPE_BOOL,
> {.i64 = 1}, 0, 1, AV_OPT_FLAG_ENCODING_PARAM},
> +    { "mxf_nobody", "",
> +      offsetof(MXFContext, mxf_nobody), AV_OPT_TYPE_BOOL, {.i64 =
> 0}, 0, 1, AV_OPT_FLAG_ENCODING_PARAM},
> +    { "mxf_j2kinterlace", "",
> +      offsetof(MXFContext, mxf_j2kinterlace), AV_OPT_TYPE_BOOL,
> {.i64 = 0}, 0, 1, AV_OPT_FLAG_ENCODING_PARAM},
> +    { "mxf_footer_with_hmd", "",
> +      offsetof(MXFContext, footer_with_hmd), AV_OPT_TYPE_BOOL, {.i64
> = 0}, 0, 1, AV_OPT_FLAG_ENCODING_PARAM},

These need descriptions. I don't really see the point in not writing a
body partition. I also suspect it will cause all sorts of issues if any
essence is actually written.

/Tomas
Erwann RENAN Oct. 28, 2021, 3:59 p.m. UTC | #2 
Ok, i will split it

-----Message d'origine-----
De : ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> De la part de Tomas Härdin
Envoyé : jeudi 28 octobre 2021 16:33
À : FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Objet : Re: [FFmpeg-devel] [PATCH] mfxenc add jpeg2000 frame field interlacing support

Looks like you messed up the subject. You need two newlines between the title of the patch and the description.

This patch also mixes a lot of different changes. Please split it up.
The bigger a patch is the more rounds of review it tends to have to go through.

> +    { 0x840B,
> {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x0B,0x0
> 0,0x00,0x00}}, /* 8+3n bytes : Array of picture components where each 
> component comprises 3 bytes named Ssizi, XRSizi, YRSizi  The array of 
> 3-byte groups is preceded by the array header comprising a 4-byte 
> value of the number of components followed by a 4-byte value of  3 .
> */

some kind of encoding problem in the comment

> @@ -843,7 +886,28 @@ static void mxf_write_track(AVFormatContext *s, 
> AVStream *st, MXFPackage *packag
>  
>      mxf_write_metadata_key(pb, 0x013b00);
>      PRINT_KEY(s, "track key", pb->buf_ptr - 16);
> -    klv_encode_ber_length(pb, 80);
> +
> +    if (st->codecpar) {
> +        static const char * pcTrackName_Video = "Picture" ;
> +        static const char * pcTrackName_Audio = "Sound" ;
> +        static const char * pcTrackName_Anc = "Ancillary" ;

static is redundant here

> +        if ( st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO )
> +        {
> +            //TrackName Video
> +            klv_encode_ber_length(pb, 80 +
> mxf_utf16_local_tag_length(pcTrackName_Video));
> +            mxf_write_local_tag_utf16(s, 0x4802 ,
> pcTrackName_Video);
> +        } else if ( st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO )
> {
> +            //TrackName Audio
> +            klv_encode_ber_length(pb, 80 +
> mxf_utf16_local_tag_length(pcTrackName_Audio));
> +            mxf_write_local_tag_utf16(s, 0x4802, pcTrackName_Audio);
> +        } else {
> +            //TrackName Ancillary
> +            klv_encode_ber_length(pb, 80 +
> mxf_utf16_local_tag_length(pcTrackName_Anc));
> +            mxf_write_local_tag_utf16(s, 0x4802, pcTrackName_Anc);
> +        }
> +    } else {
> +        klv_encode_ber_length(pb, 80);
> +    }

Is this hunk really necessary?

> @@ -1327,6 +1420,182 @@ static int64_t 
> mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
>      return pos;
>  }
>  
> +static int64_t mxf_write_jpeg2000_common(AVFormatContext *s,
> AVStream *st, const UID key)
> +{
> +    MXFStreamContext *sc = st->priv_data;
> +    MXFContext *mxf = s->priv_data;
> +    AVIOContext *pb = s->pb;
> +    int stored_width = st->codecpar->width;
> +    int stored_height = st->codecpar->height;
> +    int display_height;
> +    int f1, f2;
> +    UID transfer_ul = {0};
> +    int64_t pos = mxf_write_generic_desc(s, st, key);
> +    get_trc(transfer_ul, st->codecpar->color_trc);

Return value of get_trc() is not checked. Not sure if invalid values can ever get in here.

> +
> +    mxf_write_local_tag(s, 4, 0x3203);
> +    avio_wb32(pb, stored_width);
> +    mxf_write_local_tag(s, 4, 0x3202);
> +    avio_wb32(pb, stored_height);
> +
> +    //Sampled width
> +    mxf_write_local_tag(s, 4, 0x3205);
> +    avio_wb32(pb, st->codecpar->width);
> +
> +    //Samples height
> +    mxf_write_local_tag(s, 4, 0x3204);
> +    avio_wb32(pb, st->codecpar->height);

Why use stored_* and codecpar->*? Just use codecpar->* in both places.

> +
> +    //Sampled X Offset
> +    mxf_write_local_tag(s, 4, 0x3206);
> +    avio_wb32(pb, 0);
> +
> +    //Sampled Y Offset
> +    mxf_write_local_tag(s, 4, 0x3207);
> +    avio_wb32(pb, 0);
> +
> +    mxf_write_local_tag(s, 4, 0x3209);
> +    avio_wb32(pb, st->codecpar->width);
> +
> +    if (st->codecpar->height == 608) // PAL + VBI
> +        display_height = 576;
> +    else if (st->codecpar->height == 512)  // NTSC + VBI
> +        display_height = 486;
> +    else
> +        display_height = st->codecpar->height;
> +
> +    mxf_write_local_tag(s, 4, 0x3208);
> +    avio_wb32(pb, display_height);
> +
> +    // display X offset
> +    mxf_write_local_tag(s, 4, 0x320A);
> +    avio_wb32(pb, 0);
> +
> +    //display Y offset
> +    mxf_write_local_tag(s, 4, 0x320B);
> +    avio_wb32(pb, (st->codecpar->height - display_height));

Would be better if we could get this information via metadata instead of adding hacks to the muxer

> +    // // Padding Bits
> +    // mxf_write_local_tag(s, 2, 0x3307);
> +    // avio_wb16(pb, 0);

Stray dead code

> +    // video line map
> +    {
> +        int _field_height = (mxf->mxf_j2kinterlace) ? (2*st-
> >codecpar->height) : st->codecpar->height;
> +
> +        if (st->codecpar->sample_aspect_ratio.num && st->codecpar-
> >sample_aspect_ratio.den) {
> +            AVRational _ratio = av_mul_q(st->codecpar-
> >sample_aspect_ratio,
> +                               av_make_q(st->codecpar->width, st-
> >codecpar->height));
> +            sc->aspect_ratio = _ratio;
> +
> +            switch (_field_height) {
> +                case  576: f1 = 23; f2 = st->codecpar->codec_id ==
> AV_CODEC_ID_DVVIDEO ? 335 : 336; break;
> +                case  608: f1 =  7; f2 = 320; break;
> +                case  480: f1 = 20; f2 = st->codecpar->codec_id ==
> AV_CODEC_ID_DVVIDEO ? 285 : 283; break;
> +                case  512: f1 =  7; f2 = 270; break;
> +                case  720: f1 = 26; f2 =   0; break; // progressive
> +                case 1080: f1 = 21; f2 = 584; break;
> +                default:   f1 =  0; f2 =   0; break;
> +            }
> +        } else {
> +            switch (_field_height) {
> +                case  576: sc->aspect_ratio = (AVRational){  4,  3};
> f1 = 23; f2 = st->codecpar->codec_id == AV_CODEC_ID_DVVIDEO ? 335 :
> 336; break;
> +                case  608: sc->aspect_ratio = (AVRational){  4,  3};
> f1 =  7; f2 = 320; break;
> +                case  480: sc->aspect_ratio = (AVRational){  4,  3};
> f1 = 20; f2 = st->codecpar->codec_id == AV_CODEC_ID_DVVIDEO ? 285 :
> 283; break;
> +                case  512: sc->aspect_ratio = (AVRational){  4,  3};
> f1 =  7; f2 = 270; break;
> +                case  720: sc->aspect_ratio = (AVRational){  16,
> 9}; f1 = 26; f2 =   0; break; // progressive
> +                case 1080: sc->aspect_ratio = (AVRational){  16,
> 9}; f1 = 21; f2 = 584; break;
> +                default:   f1 =  0; f2 =   0; break;
> +            }
> +        }
> +    }

This again feels like business logic that belongs in ffmpeg.c. I imagine this applies not just to J2K and not just to MXF. Remuxing MXF to MOV for example.

> +
> +    if (!sc->interlaced && f2) {
> +        f2  = 0;
> +        f1 *= 2;
> +    }

This looks.. interesting.

> +    mxf_write_local_tag(s, 2, 0x8401);
> +    avio_wb16(pb, 0x0000);
> +    mxf_write_local_tag(s, 4, 0x8402);
> +    avio_wb32(pb, st->codecpar->width);
> +    mxf_write_local_tag(s, 4, 0x8403);
> +    avio_wb32(pb, st->codecpar->height);
> +    mxf_write_local_tag(s, 4, 0x8404);
> +    avio_wb32(pb, 0);
> +    mxf_write_local_tag(s, 4, 0x8405);
> +    avio_wb32(pb, 0);
> +    mxf_write_local_tag(s, 4, 0x8406);
> +    avio_wb32(pb, st->codecpar->width);
> +    mxf_write_local_tag(s, 4, 0x8407);
> +    avio_wb32(pb, st->codecpar->height);
> +    mxf_write_local_tag(s, 4, 0x8408);
> +    avio_wb32(pb, 0);
> +    mxf_write_local_tag(s, 4, 0x8409);
> +    avio_wb32(pb, 0);
> +    mxf_write_local_tag(s, 2, 0x840A);
> +    avio_wb16(pb, component_count);

Please comment these, to the right of each mxf_write_local_tag() is fine.

> +
> +    mxf_write_local_tag(s, 8 + 3*component_count, 0x840B);
> +    avio_wb32(pb, component_count);
> +    avio_wb32(pb, 3);
> +    {
> +        char _desc [3][3]= {  {0x09,0x01,0x01} , {0x09,0x02,0x01} ,
> {0x09,0x02,0x01} };
> +        int comp = 0;
> +        for ( comp = 0; comp< component_count ;comp++ ) {
> +            avio_write(pb, _desc[comp%3] , 3);
> +        }
> +    }

FFmpeg is C99, braces like these are not necessary. You could also do this as a single 9-byte avio_write(). You could even have the data inline.

> +    mxf_write_local_tag(s, 16, 0x840C);
> +    {
> +        char _layout[16] = {  'Y' , '\n', 'U' , '\n', 'V' , '\n',
> 'F' , 0x02,
> +                            0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00 };
> +        avio_write(pb, _layout , 16);
> +    }

Same here.

> @@ -1729,7 +2058,7 @@ static int
> mxf_write_header_metadata_sets(AVFormatContext *s)
>  static unsigned klv_fill_size(uint64_t size)
>  {
>      unsigned pad = KAG_SIZE - (size & (KAG_SIZE-1));
> -    if (pad < 20) // smallest fill item possible
> +    if (pad <= 20) // smallest fill item possible
>          return pad + KAG_SIZE;

This should *definitely* be its own patch. Also the existing code is correct unless I'm missing something majorly wrong. A zero-length ber4 KLV is 20 bytes.

>      else
>          return pad & (KAG_SIZE-1);
> @@ -2762,7 +3091,13 @@ static void
> mxf_write_system_item(AVFormatContext *s)
>      avio_wb64(pb, 0); // creation date/time stamp
>  
>      avio_w8(pb, 0x81); // SMPTE 12M time code
> -    time_code = av_timecode_get_smpte_from_framenum(&mxf->tc,
> frame);
> +    if ( 0 == mxf->mxf_j2kinterlace ) {
> +        time_code = av_timecode_get_smpte_from_framenum(&mxf->tc,
> frame);
> +    } else {
> +        unsigned int _myframenum = frame>>1;
> +        time_code = av_timecode_get_smpte_from_framenum(&mxf->tc,
> _myframenum);
> +    }

This looks.. fun.

> +
>      avio_wb32(pb, time_code);
>      avio_wb32(pb, 0); // binary group data
>      avio_wb64(pb, 0);
> @@ -2928,6 +3263,12 @@ static int mxf_write_packet(AVFormatContext *s, 
> AVPacket *pkt)
>              av_log(s, AV_LOG_ERROR, "could not get h264 profile\n");
>              return -1;
>          }
> +    } else if (st->codecpar->codec_id == AV_CODEC_ID_JPEG2000) {
> +        if (mxf->mxf_j2kinterlace) {
> +            st->codecpar->field_order = AV_FIELD_TT;
> +            sc->interlaced = 1;
> +            sc->field_dominance = 1;

Aren't these settable via ffmpeg?

> @@ -2960,11 +3301,13 @@ static int mxf_write_packet(AVFormatContext 
> *s, AVPacket *pkt)
>          if (!mxf->edit_unit_byte_count &&
>              (!mxf->edit_units_count || mxf->edit_units_count >
> EDIT_UNITS_PER_BODY) &&
>              !(ie.flags & 0x33)) { // I-frame, GOP start
> -            mxf_write_klv_fill(s);
> -            if ((err = mxf_write_partition(s, 1, 2, 
> body_partition_key, 0)) < 0)
> -                return err;
> -            mxf_write_klv_fill(s);
> -            mxf_write_index_table_segment(s);
> +            if (!mxf->mxf_nobody) {

Maybe rename to mxf_no_body

> @@ -3198,6 +3541,12 @@ static const AVOption mxf_options[] = {
>      MXF_COMMON_OPTIONS
>      { "store_user_comments", "",
>        offsetof(MXFContext, store_user_comments), AV_OPT_TYPE_BOOL,
> {.i64 = 1}, 0, 1, AV_OPT_FLAG_ENCODING_PARAM},
> +    { "mxf_nobody", "",
> +      offsetof(MXFContext, mxf_nobody), AV_OPT_TYPE_BOOL, {.i64 =
> 0}, 0, 1, AV_OPT_FLAG_ENCODING_PARAM},
> +    { "mxf_j2kinterlace", "",
> +      offsetof(MXFContext, mxf_j2kinterlace), AV_OPT_TYPE_BOOL,
> {.i64 = 0}, 0, 1, AV_OPT_FLAG_ENCODING_PARAM},
> +    { "mxf_footer_with_hmd", "",
> +      offsetof(MXFContext, footer_with_hmd), AV_OPT_TYPE_BOOL, {.i64
> = 0}, 0, 1, AV_OPT_FLAG_ENCODING_PARAM},

These need descriptions. I don't really see the point in not writing a body partition. I also suspect it will cause all sorts of issues if any essence is actually written.

/Tomas
diff mbox series

Patch

diff --git a/libavformat/mxf.h b/libavformat/mxf.h
index fe9c52732c..2ce637d4a8 100644
--- a/libavformat/mxf.h
+++ b/libavformat/mxf.h
@@ -50,6 +50,7 @@  enum MXFMetadataSetType {
     TaggedValue,
     TapeDescriptor,
     AVCSubDescriptor,
+    JPEG2000SubDescriptor,
 };
 
 enum MXFFrameLayout {
diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index c36ebef932..5b8ec60358 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -99,6 +99,7 @@  typedef struct MXFStreamContext {
     int b_picture_count;     ///< maximum number of consecutive b pictures, used in mpeg-2 descriptor
     int low_delay;           ///< low delay, used in mpeg-2 descriptor
     int avc_intra;
+    char * pcTrackName;
 } MXFStreamContext;
 
 typedef struct MXFContainerEssenceEntry {
@@ -151,6 +152,7 @@  static void mxf_write_h264_desc(AVFormatContext *s, AVStream *st);
 static void mxf_write_cdci_desc(AVFormatContext *s, AVStream *st);
 static void mxf_write_generic_sound_desc(AVFormatContext *s, AVStream *st);
 static void mxf_write_s436m_anc_desc(AVFormatContext *s, AVStream *st);
+static void mxf_write_jpeg2000_desc(AVFormatContext *s, AVStream *st);
 
 static const MXFContainerEssenceEntry mxf_essence_container_uls[] = {
     { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x02,0x0D,0x01,0x03,0x01,0x02,0x04,0x60,0x01 },
@@ -190,6 +192,10 @@  static const MXFContainerEssenceEntry mxf_essence_container_uls[] = {
       { 0x06,0x0e,0x2b,0x34,0x01,0x02,0x01,0x01,0x0d,0x01,0x03,0x01,0x15,0x01,0x08,0x00 },
       { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x07,0x04,0x01,0x02,0x02,0x03,0x01,0x01,0x00 },
       mxf_write_cdci_desc },
+    { { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x07,0x0d,0x01,0x03,0x01,0x02,0x0c,0x01,0x00 },
+      { 0x06,0x0e,0x2b,0x34,0x01,0x02,0x01,0x01,0x0d,0x01,0x03,0x01,0x15,0x01,0x08,0x00 },
+      { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0A,0x04,0x01,0x02,0x02,0x03,0x01,0x01,0x7F },
+      mxf_write_jpeg2000_desc },
     // H.264
     { { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x0a,0x0D,0x01,0x03,0x01,0x02,0x10,0x60,0x01 },
       { 0x06,0x0E,0x2B,0x34,0x01,0x02,0x01,0x01,0x0D,0x01,0x03,0x01,0x15,0x01,0x05,0x00 },
@@ -295,6 +301,8 @@  static const MXFLocalTagPair mxf_local_tag_batch[] = {
     { 0x4B01, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x02,0x05,0x30,0x04,0x05,0x00,0x00,0x00,0x00}}, /* Edit Rate */
     { 0x4B02, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x02,0x07,0x02,0x01,0x03,0x01,0x03,0x00,0x00}}, /* Origin */
     { 0x4803, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x02,0x06,0x01,0x01,0x04,0x02,0x04,0x00,0x00}}, /* Sequence reference */
+    // Track name
+    { 0x4802, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x02,0x01,0x07,0x01,0x02,0x01,0x00,0x00,0x00}}, /* TrackName */
     // Sequence
     { 0x0201, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x02,0x04,0x07,0x01,0x00,0x00,0x00,0x00,0x00}}, /* Data Definition UL */
     { 0x0202, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x02,0x07,0x02,0x02,0x01,0x01,0x03,0x00,0x00}}, /* Duration */
@@ -386,6 +394,20 @@  static const MXFLocalTagPair mxf_local_tag_batch[] = {
     { 0x8302, FF_MXF_MasteringDisplayWhitePointChromaticity },
     { 0x8303, FF_MXF_MasteringDisplayMaximumLuminance },
     { 0x8304, FF_MXF_MasteringDisplayMinimumLuminance },
+    // ff_mxf_jpeg2000_local_tags
+    { 0x8400, {0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x09,0x06,0x01,0x01,0x04,0x06,0x10,0x00,0x00}}, /* Sub Descriptors / Opt Ordered array of strong references to sub descriptor sets */
+    { 0x8401, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x01,0x00,0x00,0x00}}, /* 2 bytes : An enumerated value that defines the decoder capabilities.  */
+    { 0x8402, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x02,0x00,0x00,0x00}}, /* 4 bytes : Width of the reference grid */
+    { 0x8403, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x03,0x00,0x00,0x00}}, /* 4 bytes : Height of the reference grid */
+    { 0x8404, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x04,0x00,0x00,0x00}}, /* 4 bytes : Horizontal offset from the origin of the reference grid to the left side of the image area */
+    { 0x8405, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x05,0x00,0x00,0x00}}, /* 4 bytes : Vertical offset from the origin of the reference grid to the left side of the image area */
+    { 0x8406, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x06,0x00,0x00,0x00}}, /* 4 bytes : Width of one reference tile with respect to the reference grid, */
+    { 0x8407, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x07,0x00,0x00,0x00}}, /* 4 bytes : Height of one reference tile with respect to the reference grid, */
+    { 0x8408, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x08,0x00,0x00,0x00}}, /* 4 bytes : Horizontal offset from the origin of the reference grid to the left side of the first tile */
+    { 0x8409, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x09,0x00,0x00,0x00}}, /* 4 bytes : Vertical offset from the origin of the reference grid to the left side of the first tile */
+    { 0x840A, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x0A,0x00,0x00,0x00}}, /* 2 bytes : The number of components in the picture */
+    { 0x840B, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x0B,0x00,0x00,0x00}}, /* 8+3n bytes : Array of picture components where each component comprises 3 bytes named Ssizi, XRSizi, YRSizi  The array of 3-byte groups is preceded by the array header comprising a 4-byte value of the number of components followed by a 4-byte value of �3�. */
+    { 0x840C, {0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0a,0x04,0x01,0x06,0x03,0x0E,0x00,0x00,0x00}}, /* The nature and order of the image components in the compressed domain as carried in the J2C codestream.. */
 };
 
 #define MXF_NUM_TAGS FF_ARRAY_ELEMS(mxf_local_tag_batch)
@@ -421,6 +443,9 @@  typedef struct MXFContext {
     int track_instance_count; // used to generate MXFTrack uuids
     int cbr_index;           ///< use a constant bitrate index
     uint8_t unused_tags[MXF_NUM_TAGS];  ///< local tags that we know will not be used
+    int mxf_nobody;
+    int mxf_j2kinterlace;
+    int footer_with_hmd;
 } MXFContext;
 
 static void mxf_write_uuid(AVIOContext *pb, enum MXFMetadataSetType type, int value)
@@ -521,7 +546,7 @@  static void mxf_write_primer_pack(AVFormatContext *s)
     MXFContext *mxf = s->priv_data;
     AVIOContext *pb = s->pb;
     int local_tag_number = MXF_NUM_TAGS, i;
-    int will_have_avc_tags = 0, will_have_mastering_tags = 0;
+    int will_have_avc_tags = 0, will_have_mastering_tags = 0, will_have_jpeg2000_tags = 0;
 
     for (i = 0; i < s->nb_streams; i++) {
         MXFStreamContext *sc = s->streams[i]->priv_data;
@@ -531,6 +556,9 @@  static void mxf_write_primer_pack(AVFormatContext *s)
         if (av_stream_get_side_data(s->streams[i], AV_PKT_DATA_MASTERING_DISPLAY_METADATA, NULL)) {
             will_have_mastering_tags = 1;
         }
+        if (s->streams[i]->codecpar->codec_id == AV_CODEC_ID_JPEG2000){
+            will_have_jpeg2000_tags = 1;
+        }
     }
 
     if (!mxf->store_user_comments) {
@@ -553,6 +581,21 @@  static void mxf_write_primer_pack(AVFormatContext *s)
         mxf_mark_tag_unused(mxf, 0x8304);
     }
 
+    if (!will_have_jpeg2000_tags) {
+        mxf_mark_tag_unused(mxf, 0x8401);
+        mxf_mark_tag_unused(mxf, 0x8402);
+        mxf_mark_tag_unused(mxf, 0x8403);
+        mxf_mark_tag_unused(mxf, 0x8404);
+        mxf_mark_tag_unused(mxf, 0x8405);
+        mxf_mark_tag_unused(mxf, 0x8406);
+        mxf_mark_tag_unused(mxf, 0x8407);
+        mxf_mark_tag_unused(mxf, 0x8408);
+        mxf_mark_tag_unused(mxf, 0x8409);
+        mxf_mark_tag_unused(mxf, 0x840A);
+        mxf_mark_tag_unused(mxf, 0x840B);
+        mxf_mark_tag_unused(mxf, 0x840C);
+    }
+
     for (i = 0; i < MXF_NUM_TAGS; i++) {
         if (mxf->unused_tags[i]) {
             local_tag_number--;
@@ -843,7 +886,28 @@  static void mxf_write_track(AVFormatContext *s, AVStream *st, MXFPackage *packag
 
     mxf_write_metadata_key(pb, 0x013b00);
     PRINT_KEY(s, "track key", pb->buf_ptr - 16);
-    klv_encode_ber_length(pb, 80);
+
+    if (st->codecpar) {
+        static const char * pcTrackName_Video = "Picture" ;
+        static const char * pcTrackName_Audio = "Sound" ;
+        static const char * pcTrackName_Anc = "Ancillary" ;
+        if ( st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO )
+        {
+            //TrackName Video
+            klv_encode_ber_length(pb, 80 + mxf_utf16_local_tag_length(pcTrackName_Video));
+            mxf_write_local_tag_utf16(s, 0x4802 , pcTrackName_Video);
+        } else if ( st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO ) {
+            //TrackName Audio
+            klv_encode_ber_length(pb, 80 + mxf_utf16_local_tag_length(pcTrackName_Audio));
+            mxf_write_local_tag_utf16(s, 0x4802, pcTrackName_Audio);
+        } else {
+            //TrackName Ancillary
+            klv_encode_ber_length(pb, 80 + mxf_utf16_local_tag_length(pcTrackName_Anc));
+            mxf_write_local_tag_utf16(s, 0x4802, pcTrackName_Anc);
+        }
+    } else {
+        klv_encode_ber_length(pb, 80);
+    }
 
     // write track uid
     mxf_write_local_tag(s, 16, 0x3C0A);
@@ -1092,6 +1156,35 @@  static const UID mxf_cdci_descriptor_key      = { 0x06,0x0E,0x2B,0x34,0x02,0x53,
 static const UID mxf_generic_sound_descriptor_key = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0D,0x01,0x01,0x01,0x01,0x01,0x42,0x00 };
 
 static const UID mxf_avc_subdescriptor_key = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01,0x6E,0x00 };
+static const UID mxf_jpeg2000_subdescriptor_key = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0D,0x01,0x01,0x01,0x01,0x01,0x5A,00 };
+
+static int get_trc(UID ul, enum AVColorTransferCharacteristic trc)
+{
+    switch (trc) {
+    case AVCOL_TRC_GAMMA28   :
+    case AVCOL_TRC_GAMMA22   :
+        memcpy(ul, ((UID){0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x04,0x01,0x01,0x01,0x01,0x01,0x00,0x00}), 16);
+        return 0;
+    case AVCOL_TRC_BT709     :
+    case AVCOL_TRC_SMPTE170M :
+        memcpy(ul, ((UID){0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x04,0x01,0x01,0x01,0x01,0x02,0x00,0x00}), 16);
+        return 0;
+    case AVCOL_TRC_SMPTE240M :
+        memcpy(ul, ((UID){0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x04,0x01,0x01,0x01,0x01,0x03,0x00,0x00}), 16);
+        return 0;
+    case AVCOL_TRC_BT1361_ECG:
+        memcpy(ul, ((UID){0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x06,0x04,0x01,0x01,0x01,0x01,0x05,0x00,0x00}), 16);
+        return 0;
+    case AVCOL_TRC_LINEAR    :
+        memcpy(ul, ((UID){0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x06,0x04,0x01,0x01,0x01,0x01,0x06,0x00,0x00}), 16);
+        return 0;
+    case AVCOL_TRC_SMPTE428  :
+        memcpy(ul, ((UID){0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x08,0x04,0x01,0x01,0x01,0x01,0x07,0x00,0x00}), 16);
+        return 0;
+    default:
+        return -1;
+    }
+}
 
 static inline uint16_t rescale_mastering_chroma(AVRational q)
 {
@@ -1327,6 +1420,182 @@  static int64_t mxf_write_cdci_common(AVFormatContext *s, AVStream *st, const UID
     return pos;
 }
 
+static int64_t mxf_write_jpeg2000_common(AVFormatContext *s, AVStream *st, const UID key)
+{
+    MXFStreamContext *sc = st->priv_data;
+    MXFContext *mxf = s->priv_data;
+    AVIOContext *pb = s->pb;
+    int stored_width = st->codecpar->width;
+    int stored_height = st->codecpar->height;
+    int display_height;
+    int f1, f2;
+    UID transfer_ul = {0};
+    int64_t pos = mxf_write_generic_desc(s, st, key);
+    get_trc(transfer_ul, st->codecpar->color_trc);
+
+    mxf_write_local_tag(s, 4, 0x3203);
+    avio_wb32(pb, stored_width);
+    mxf_write_local_tag(s, 4, 0x3202);
+    avio_wb32(pb, stored_height);
+
+    //Sampled width
+    mxf_write_local_tag(s, 4, 0x3205);
+    avio_wb32(pb, st->codecpar->width);
+
+    //Samples height
+    mxf_write_local_tag(s, 4, 0x3204);
+    avio_wb32(pb, st->codecpar->height);
+
+    //Sampled X Offset
+    mxf_write_local_tag(s, 4, 0x3206);
+    avio_wb32(pb, 0);
+
+    //Sampled Y Offset
+    mxf_write_local_tag(s, 4, 0x3207);
+    avio_wb32(pb, 0);
+
+    mxf_write_local_tag(s, 4, 0x3209);
+    avio_wb32(pb, st->codecpar->width);
+
+    if (st->codecpar->height == 608) // PAL + VBI
+        display_height = 576;
+    else if (st->codecpar->height == 512)  // NTSC + VBI
+        display_height = 486;
+    else
+        display_height = st->codecpar->height;
+
+    mxf_write_local_tag(s, 4, 0x3208);
+    avio_wb32(pb, display_height);
+
+    // display X offset
+    mxf_write_local_tag(s, 4, 0x320A);
+    avio_wb32(pb, 0);
+
+    //display Y offset
+    mxf_write_local_tag(s, 4, 0x320B);
+    avio_wb32(pb, (st->codecpar->height - display_height));
+
+    if (sc->interlaced) {
+        //Display F2 Offset
+        mxf_write_local_tag(s, 4, 0x3217);
+        avio_wb32(pb, -((st->codecpar->height - display_height)&1));
+    }
+
+    // component depth
+    mxf_write_local_tag(s, 4, 0x3301);
+    avio_wb32(pb, sc->component_depth);
+
+    // horizontal subsampling
+    mxf_write_local_tag(s, 4, 0x3302);
+    avio_wb32(pb, sc->h_chroma_sub_sample);
+
+    // vertical subsampling
+    mxf_write_local_tag(s, 4, 0x3308);
+    avio_wb32(pb, sc->v_chroma_sub_sample);
+
+    // color siting
+    mxf_write_local_tag(s, 1, 0x3303);
+    avio_w8(pb, sc->color_siting);
+
+    // // Padding Bits
+    // mxf_write_local_tag(s, 2, 0x3307);
+    // avio_wb16(pb, 0);
+
+    if (st->codecpar->color_range != AVCOL_RANGE_UNSPECIFIED) {
+        int black = 0,
+            white = (1<<sc->component_depth) - 1,
+            color = (1<<sc->component_depth) - 1;
+        if (st->codecpar->color_range == AVCOL_RANGE_MPEG) {
+            black = 1   << (sc->component_depth - 4);
+            white = 235 << (sc->component_depth - 8);
+            color = (14 << (sc->component_depth - 4)) + 1;
+        }
+        mxf_write_local_tag(s, 4, 0x3304);
+        avio_wb32(pb, black);
+        mxf_write_local_tag(s, 4, 0x3305);
+        avio_wb32(pb, white);
+        mxf_write_local_tag(s, 4, 0x3306);
+        avio_wb32(pb, color);
+    }
+
+    if (sc->signal_standard) {
+        mxf_write_local_tag(s, 1, 0x3215);
+        avio_w8(pb, sc->signal_standard);
+    }
+
+    // frame layout
+    mxf_write_local_tag(s, 1, 0x320C);
+    avio_w8(pb, sc->interlaced);
+
+    // video line map
+    {
+        int _field_height = (mxf->mxf_j2kinterlace) ? (2*st->codecpar->height) : st->codecpar->height;
+
+        if (st->codecpar->sample_aspect_ratio.num && st->codecpar->sample_aspect_ratio.den) {
+            AVRational _ratio = av_mul_q(st->codecpar->sample_aspect_ratio,
+                               av_make_q(st->codecpar->width, st->codecpar->height));
+            sc->aspect_ratio = _ratio;
+
+            switch (_field_height) {
+                case  576: f1 = 23; f2 = st->codecpar->codec_id == AV_CODEC_ID_DVVIDEO ? 335 : 336; break;
+                case  608: f1 =  7; f2 = 320; break;
+                case  480: f1 = 20; f2 = st->codecpar->codec_id == AV_CODEC_ID_DVVIDEO ? 285 : 283; break;
+                case  512: f1 =  7; f2 = 270; break;
+                case  720: f1 = 26; f2 =   0; break; // progressive
+                case 1080: f1 = 21; f2 = 584; break;
+                default:   f1 =  0; f2 =   0; break;
+            }
+        } else {
+            switch (_field_height) {
+                case  576: sc->aspect_ratio = (AVRational){  4,  3}; f1 = 23; f2 = st->codecpar->codec_id == AV_CODEC_ID_DVVIDEO ? 335 : 336; break;
+                case  608: sc->aspect_ratio = (AVRational){  4,  3}; f1 =  7; f2 = 320; break;
+                case  480: sc->aspect_ratio = (AVRational){  4,  3}; f1 = 20; f2 = st->codecpar->codec_id == AV_CODEC_ID_DVVIDEO ? 285 : 283; break;
+                case  512: sc->aspect_ratio = (AVRational){  4,  3}; f1 =  7; f2 = 270; break;
+                case  720: sc->aspect_ratio = (AVRational){  16,  9}; f1 = 26; f2 =   0; break; // progressive
+                case 1080: sc->aspect_ratio = (AVRational){  16,  9}; f1 = 21; f2 = 584; break;
+                default:   f1 =  0; f2 =   0; break;
+            }
+        }
+    }
+
+    if (!sc->interlaced && f2) {
+        f2  = 0;
+        f1 *= 2;
+    }
+
+    mxf_write_local_tag(s, 16, 0x320D);
+    avio_wb32(pb, 2);
+    avio_wb32(pb, 4);
+    avio_wb32(pb, f1);
+    avio_wb32(pb, f2);
+    mxf_write_local_tag(s, 8, 0x320E);
+    avio_wb32(pb, sc->aspect_ratio.num);
+    avio_wb32(pb, sc->aspect_ratio.den);
+
+    //Transfer characteristic
+    if (transfer_ul[0]) {
+        mxf_write_local_tag(s, 16, 0x3210);
+        avio_write(pb, transfer_ul, 16);
+    }
+
+    mxf_write_local_tag(s, 16, 0x3201);
+    avio_write(pb, *sc->codec_ul, 16);
+
+    mxf_write_local_tag(s, 1, 0x3212);
+    avio_w8(pb, sc->field_dominance);
+
+/* The JPEG 2000 picture sub-descriptor includes only those properties from the main header of the codestream
+that are required in ISO/IEC 15444-1, Annex A, Table A.2. */
+    {
+        // write avc sub descriptor ref
+        mxf_write_local_tag(s, 8 + 16, 0x8400);
+        mxf_write_refs_count(pb, 1);
+        mxf_write_uuid(pb, JPEG2000SubDescriptor, 0);
+    }
+
+    return pos;
+}
+
 static void mxf_update_klv_size(AVIOContext *pb, int64_t pos)
 {
     int64_t cur_pos = avio_tell(pb);
@@ -1360,6 +1629,60 @@  static void mxf_write_avc_subdesc(AVFormatContext *s, AVStream *st)
     mxf_update_klv_size(s->pb, pos);
 }
 
+static void mxf_write_jpeg2000_subdesc(AVFormatContext *s, AVStream *st)
+{
+    AVIOContext *pb = s->pb;
+    int64_t pos;
+
+    int component_count = av_pix_fmt_count_planes(st->codecpar->format);
+
+    avio_write(pb, mxf_jpeg2000_subdescriptor_key, 16);
+    klv_encode_ber4_length(pb, 0);
+    pos = avio_tell(pb);
+
+    mxf_write_local_tag(s, 16, 0x3C0A);
+    mxf_write_uuid(pb, JPEG2000SubDescriptor, 0);
+
+    mxf_write_local_tag(s, 2, 0x8401);
+    avio_wb16(pb, 0x0000);
+    mxf_write_local_tag(s, 4, 0x8402);
+    avio_wb32(pb, st->codecpar->width);
+    mxf_write_local_tag(s, 4, 0x8403);
+    avio_wb32(pb, st->codecpar->height);
+    mxf_write_local_tag(s, 4, 0x8404);
+    avio_wb32(pb, 0);
+    mxf_write_local_tag(s, 4, 0x8405);
+    avio_wb32(pb, 0);
+    mxf_write_local_tag(s, 4, 0x8406);
+    avio_wb32(pb, st->codecpar->width);
+    mxf_write_local_tag(s, 4, 0x8407);
+    avio_wb32(pb, st->codecpar->height);
+    mxf_write_local_tag(s, 4, 0x8408);
+    avio_wb32(pb, 0);
+    mxf_write_local_tag(s, 4, 0x8409);
+    avio_wb32(pb, 0);
+    mxf_write_local_tag(s, 2, 0x840A);
+    avio_wb16(pb, component_count);
+
+    mxf_write_local_tag(s, 8 + 3*component_count, 0x840B);
+    avio_wb32(pb, component_count);
+    avio_wb32(pb, 3);
+    {
+        char _desc [3][3]= {  {0x09,0x01,0x01} , {0x09,0x02,0x01} , {0x09,0x02,0x01} };                  
+        int comp = 0;
+        for ( comp = 0; comp< component_count ;comp++ ) {
+            avio_write(pb, _desc[comp%3] , 3);
+        }
+    }
+    mxf_write_local_tag(s, 16, 0x840C);
+    {
+        char _layout[16] = {  'Y' , '\n', 'U' , '\n', 'V' , '\n', 'F' , 0x02,
+                            0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
+        avio_write(pb, _layout , 16);
+    }
+    mxf_update_klv_size(pb, pos);
+}
+
 static void mxf_write_cdci_desc(AVFormatContext *s, AVStream *st)
 {
     int64_t pos = mxf_write_cdci_common(s, st, mxf_cdci_descriptor_key);
@@ -1370,6 +1693,12 @@  static void mxf_write_cdci_desc(AVFormatContext *s, AVStream *st)
     }
 }
 
+static void mxf_write_jpeg2000_desc(AVFormatContext *s, AVStream *st)
+{
+    int64_t pos = mxf_write_jpeg2000_common(s, st, mxf_cdci_descriptor_key);
+    mxf_update_klv_size(s->pb, pos);
+    mxf_write_jpeg2000_subdesc(s, st);
+}
 static void mxf_write_h264_desc(AVFormatContext *s, AVStream *st)
 {
     MXFStreamContext *sc = st->priv_data;
@@ -1729,7 +2058,7 @@  static int mxf_write_header_metadata_sets(AVFormatContext *s)
 static unsigned klv_fill_size(uint64_t size)
 {
     unsigned pad = KAG_SIZE - (size & (KAG_SIZE-1));
-    if (pad < 20) // smallest fill item possible
+    if (pad <= 20) // smallest fill item possible
         return pad + KAG_SIZE;
     else
         return pad & (KAG_SIZE-1);
@@ -2762,7 +3091,13 @@  static void mxf_write_system_item(AVFormatContext *s)
     avio_wb64(pb, 0); // creation date/time stamp
 
     avio_w8(pb, 0x81); // SMPTE 12M time code
-    time_code = av_timecode_get_smpte_from_framenum(&mxf->tc, frame);
+    if ( 0 == mxf->mxf_j2kinterlace ) {
+        time_code = av_timecode_get_smpte_from_framenum(&mxf->tc, frame);
+    } else {
+        unsigned int _myframenum = frame>>1;
+        time_code = av_timecode_get_smpte_from_framenum(&mxf->tc, _myframenum);
+    }
+
     avio_wb32(pb, time_code);
     avio_wb32(pb, 0); // binary group data
     avio_wb64(pb, 0);
@@ -2928,6 +3263,12 @@  static int mxf_write_packet(AVFormatContext *s, AVPacket *pkt)
             av_log(s, AV_LOG_ERROR, "could not get h264 profile\n");
             return -1;
         }
+    } else if (st->codecpar->codec_id == AV_CODEC_ID_JPEG2000) {
+        if (mxf->mxf_j2kinterlace) {
+            st->codecpar->field_order = AV_FIELD_TT;
+            sc->interlaced = 1;
+            sc->field_dominance = 1;
+        }
     }
 
     if (mxf->cbr_index) {
@@ -2960,11 +3301,13 @@  static int mxf_write_packet(AVFormatContext *s, AVPacket *pkt)
         if (!mxf->edit_unit_byte_count &&
             (!mxf->edit_units_count || mxf->edit_units_count > EDIT_UNITS_PER_BODY) &&
             !(ie.flags & 0x33)) { // I-frame, GOP start
-            mxf_write_klv_fill(s);
-            if ((err = mxf_write_partition(s, 1, 2, body_partition_key, 0)) < 0)
-                return err;
-            mxf_write_klv_fill(s);
-            mxf_write_index_table_segment(s);
+            if (!mxf->mxf_nobody) {
+                mxf_write_klv_fill(s);
+                if ((err = mxf_write_partition(s, 1, 2, body_partition_key, 0)) < 0)
+                    return err;
+                mxf_write_klv_fill(s);
+                mxf_write_index_table_segment(s);
+            }
         }
 
         mxf_write_klv_fill(s);
@@ -3044,10 +3387,10 @@  static int mxf_write_footer(AVFormatContext *s)
     mxf_write_klv_fill(s);
     mxf->footer_partition_offset = avio_tell(pb);
     if (mxf->edit_unit_byte_count && s->oformat != &ff_mxf_opatom_muxer) { // no need to repeat index
-        if ((err = mxf_write_partition(s, 0, 0, footer_partition_key, 0)) < 0)
+        if ((err = mxf_write_partition(s, 0, 0, footer_partition_key, mxf->footer_with_hmd)) < 0)
             return err;
     } else {
-        if ((err = mxf_write_partition(s, 0, 2, footer_partition_key, 0)) < 0)
+        if ((err = mxf_write_partition(s, 0, 2, footer_partition_key, mxf->footer_with_hmd)) < 0)
             return err;
         mxf_write_klv_fill(s);
         mxf_write_index_table_segment(s);
@@ -3198,6 +3541,12 @@  static const AVOption mxf_options[] = {
     MXF_COMMON_OPTIONS
     { "store_user_comments", "",
       offsetof(MXFContext, store_user_comments), AV_OPT_TYPE_BOOL, {.i64 = 1}, 0, 1, AV_OPT_FLAG_ENCODING_PARAM},
+    { "mxf_nobody", "",
+      offsetof(MXFContext, mxf_nobody), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1, AV_OPT_FLAG_ENCODING_PARAM},
+    { "mxf_j2kinterlace", "",
+      offsetof(MXFContext, mxf_j2kinterlace), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1, AV_OPT_FLAG_ENCODING_PARAM},
+    { "mxf_footer_with_hmd", "",
+      offsetof(MXFContext, footer_with_hmd), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1, AV_OPT_FLAG_ENCODING_PARAM},
     { NULL },
 };