diff mbox series

[FFmpeg-devel,07/16] avformat/mxf: Deduplicate random_index_pack_key

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
Related show

Checks

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

Commit Message

Andreas Rheinhardt Jan. 4, 2021, 12:28 a.m. UTC
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(-)

Comments

Tomas Härdin Jan. 5, 2021, 2:54 p.m. UTC | #1
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
Andreas Rheinhardt Jan. 5, 2021, 3:51 p.m. UTC | #2
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 mbox series

Patch

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)