Re: [FFmpeg-devel] [PATCH] mfxenc add jpeg2000 frame field interlacing support
Ok, i will split it
-Message d'origine-
De : ffmpeg-devel De la part de Tomas Härdin
Envoyé : jeudi 28 octobre 2021 16:33
À : FFmpeg development discussions and patches
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);
>
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;
[FFmpeg-devel] [PATCH] 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 fl
---
libavformat/mxf.h| 1 +
libavformat/mxfenc.c | 371 +--
2 files changed, 361 insertions(+), 11 deletions(-)
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
