[FFmpeg-devel] [PATCH] mfxenc add jpeg2000 frame field interlacing support
Tomas Härdin
tjoppen at acc.umu.se
Thu Oct 28 17:32:30 EEST 2021
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
More information about the ffmpeg-devel
mailing list