| Message ID | 20210104002816.2321974-7-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | ac0e75b647e434aa0291a8f9de373dd63580d5ae |
| Headers | show |
| Series | [FFmpeg-devel,01/16] avcodec/g723_1: Deduplicate arrays | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
mån 2021-01-04 klockan 01:28 +0100 skrev Andreas Rheinhardt: > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > There is also another key that is duplicated: header_open_partition_key > and mxf_jp2k_rsiz. But they seem to mean something different which makes > me wonder if one of these keys is incorrect; I have therefore refrained > from deduplicating them. Looks like I didn't notice this when I reviewed the patch. This indicates that the MXF muxer that wrote the file that 2af260e3 adds support for is broken. S422m-2006 says that the UL designator for Rsiz should be 04.01.06.03.01.00.00.00. S400m-2004 defines the prefix to this designator, which is the usual 06.0E.2B.34.04.01.01.VV where VV is the registry version. Did we have a sample for this? If you decide to deduplicate rsiz then you should have kind of comment to this effect, and/or a link to this thread and http://ffmpeg.org/pipermail/ffmpeg-devel/2015-March/169591.html The patch itself looks fine. Might as well deduplicate all keys. There's little sense in having separate sets of keys defined for the muxer and demuxer. /Tomas
Tomas Härdin: > mån 2021-01-04 klockan 01:28 +0100 skrev Andreas Rheinhardt: >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> There is also another key that is duplicated: header_open_partition_key >> and mxf_jp2k_rsiz. But they seem to mean something different which makes >> me wonder if one of these keys is incorrect; I have therefore refrained >> from deduplicating them. > > Looks like I didn't notice this when I reviewed the patch. This > indicates that the MXF muxer that wrote the file that 2af260e3 adds > support for is broken. > > S422m-2006 says that the UL designator for Rsiz should be > 04.01.06.03.01.00.00.00. S400m-2004 defines the prefix to this > designator, which is the usual 06.0E.2B.34.04.01.01.VV where VV is the > registry version. > > Did we have a sample for this? > I don't have any sample for it; it is also untested by FATE. > If you decide to deduplicate rsiz then you should have kind of comment > to this effect, and/or a link to this thread and > http://ffmpeg.org/pipermail/ffmpeg-devel/2015-March/169591.html > If the usage of this key in the demuxer is wrong, then this should be fixed and not deduplicated. > The patch itself looks fine. Might as well deduplicate all keys. > There's little sense in having separate sets of keys defined for the > muxer and demuxer. The muxer has some of its ULs in a structure that contains pointers to muxing-related functions, so that can't be deduplicated without lots of modifications. And then there are several d-10 keys only used by the muxer. Is this intended? - Andreas
diff --git a/libavformat/mxf.c b/libavformat/mxf.c index 88f69ebcfb..1901b24c68 100644 --- a/libavformat/mxf.c +++ b/libavformat/mxf.c @@ -24,6 +24,8 @@ const uint8_t ff_mxf_mastering_display_prefix[13] = { 0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x20,0x04,0x01,0x01 }; +const uint8_t ff_mxf_random_index_pack_key[16] = { 0x06,0x0e,0x2b,0x34,0x02,0x05,0x01,0x01,0x0d,0x01,0x02,0x01,0x01,0x11,0x01,0x00 }; + /* be careful to update references to this array if reordering */ /* local tags are dynamic and must not clash with others in mxfenc.c */ const MXFLocalTagPair ff_mxf_mastering_display_local_tags[4] = { diff --git a/libavformat/mxf.h b/libavformat/mxf.h index 7fa10bcca1..5219abc767 100644 --- a/libavformat/mxf.h +++ b/libavformat/mxf.h @@ -84,6 +84,7 @@ typedef struct MXFLocalTagPair { } MXFLocalTagPair; extern const uint8_t ff_mxf_mastering_display_prefix[13]; +extern const uint8_t ff_mxf_random_index_pack_key[16]; extern const MXFLocalTagPair ff_mxf_mastering_display_local_tags[4]; #define FF_MXF_MASTERING_CHROMA_DEN 50000 diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c index b563f2abe1..784f43d6d1 100644 --- a/libavformat/mxfdec.c +++ b/libavformat/mxfdec.c @@ -328,7 +328,6 @@ static const uint8_t mxf_apple_coll_prefix[] = { 0x06,0x0e,0x2b,0x static const uint8_t mxf_crypto_source_container_ul[] = { 0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x09,0x06,0x01,0x01,0x02,0x02,0x00,0x00,0x00 }; static const uint8_t mxf_encrypted_triplet_key[] = { 0x06,0x0e,0x2b,0x34,0x02,0x04,0x01,0x07,0x0d,0x01,0x03,0x01,0x02,0x7e,0x01,0x00 }; static const uint8_t mxf_encrypted_essence_container[] = { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x07,0x0d,0x01,0x03,0x01,0x02,0x0b,0x01,0x00 }; -static const uint8_t mxf_random_index_pack_key[] = { 0x06,0x0e,0x2b,0x34,0x02,0x05,0x01,0x01,0x0d,0x01,0x02,0x01,0x01,0x11,0x01,0x00 }; static const uint8_t mxf_sony_mpeg4_extradata[] = { 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x01,0x0e,0x06,0x06,0x02,0x02,0x01,0x00,0x00 }; static const uint8_t mxf_avid_project_name[] = { 0xa5,0xfb,0x7b,0x25,0xf6,0x15,0x94,0xb9,0x62,0xfc,0x37,0x17,0x49,0x2d,0x42,0xbf }; static const uint8_t mxf_jp2k_rsiz[] = { 0x06,0x0e,0x2b,0x34,0x02,0x05,0x01,0x01,0x0d,0x01,0x02,0x01,0x01,0x02,0x01,0x00 }; @@ -3264,7 +3263,7 @@ static void mxf_read_random_index_pack(AVFormatContext *s) goto end; avio_seek(s->pb, file_size - length, SEEK_SET); if (klv_read_packet(&klv, s->pb) < 0 || - !IS_KLV_KEY(klv.key, mxf_random_index_pack_key)) + !IS_KLV_KEY(klv.key, ff_mxf_random_index_pack_key)) goto end; if (klv.next_klv != file_size || klv.length <= 4 || (klv.length - 4) % 12) { av_log(s, AV_LOG_WARNING, "Invalid RIP KLV length\n"); diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c index d8678c9d25..6c5331ad62 100644 --- a/libavformat/mxfenc.c +++ b/libavformat/mxfenc.c @@ -271,7 +271,6 @@ static const uint8_t opatom_ul[] = { 0x06,0x0E,0x2B,0x34,0x04, static const uint8_t footer_partition_key[] = { 0x06,0x0E,0x2B,0x34,0x02,0x05,0x01,0x01,0x0D,0x01,0x02,0x01,0x01,0x04,0x04,0x00 }; // ClosedComplete static const uint8_t primer_pack_key[] = { 0x06,0x0E,0x2B,0x34,0x02,0x05,0x01,0x01,0x0D,0x01,0x02,0x01,0x01,0x05,0x01,0x00 }; static const uint8_t index_table_segment_key[] = { 0x06,0x0E,0x2B,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x02,0x01,0x01,0x10,0x01,0x00 }; -static const uint8_t random_index_pack_key[] = { 0x06,0x0E,0x2B,0x34,0x02,0x05,0x01,0x01,0x0D,0x01,0x02,0x01,0x01,0x11,0x01,0x00 }; static const uint8_t header_open_partition_key[] = { 0x06,0x0E,0x2B,0x34,0x02,0x05,0x01,0x01,0x0D,0x01,0x02,0x01,0x01,0x02,0x01,0x00 }; // OpenIncomplete static const uint8_t header_closed_partition_key[] = { 0x06,0x0E,0x2B,0x34,0x02,0x05,0x01,0x01,0x0D,0x01,0x02,0x01,0x01,0x02,0x04,0x00 }; // ClosedComplete static const uint8_t klv_fill_key[] = { 0x06,0x0E,0x2B,0x34,0x01,0x01,0x01,0x02,0x03,0x01,0x02,0x10,0x01,0x00,0x00,0x00 }; @@ -2956,7 +2955,7 @@ static void mxf_write_random_index_pack(AVFormatContext *s) uint64_t pos = avio_tell(pb); int i; - avio_write(pb, random_index_pack_key, 16); + avio_write(pb, ff_mxf_random_index_pack_key, 16); klv_encode_ber_length(pb, 28 + 12LL*mxf->body_partitions_count); if (mxf->edit_unit_byte_count && s->oformat != &ff_mxf_opatom_muxer)
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- There is also another key that is duplicated: header_open_partition_key and mxf_jp2k_rsiz. But they seem to mean something different which makes me wonder if one of these keys is incorrect; I have therefore refrained from deduplicating them. libavformat/mxf.c | 2 ++ libavformat/mxf.h | 1 + libavformat/mxfdec.c | 3 +-- libavformat/mxfenc.c | 3 +-- 4 files changed, 5 insertions(+), 4 deletions(-)