Ok, i will split it
-----Message d'origine-----
De : ffmpeg-devel <ffmpeg-devel-boun...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email ffmpeg-devel-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
