[FFmpeg-devel] [PATCH 1/2] avformat/mxfenc: allow more bits for variable part in uuid generation

[FFmpeg-devel] [PATCH 1/2] avformat/mxfenc: allow more bits for variable part in uuid generation

[Marton Balint]

Also make sure we do not change the product UID.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/mxfenc.c                    | 9 +++++----
 tests/ref/fate/copy-trac4914            | 2 +-
 tests/ref/fate/mxf-d10-user-comments    | 6 +++---
 tests/ref/fate/mxf-opatom-user-comments | 2 +-
 tests/ref/fate/mxf-reel_name            | 2 +-
 tests/ref/fate/mxf-user-comments        | 2 +-
 tests/ref/fate/time_base                | 2 +-
 tests/ref/lavf/mxf                      | 6 +++---
 tests/ref/lavf/mxf_d10                  | 2 +-
 tests/ref/lavf/mxf_dv25                 | 2 +-
 tests/ref/lavf/mxf_dvcpro50             | 2 +-
 tests/ref/lavf/mxf_opatom               | 2 +-
 tests/ref/lavf/mxf_opatom_audio         | 2 +-
 13 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index 1e87dc6111..ba8e7babfb 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -227,7 +227,8 @@ static const UID mxf_d10_container_uls[] = {
     { 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x0D,0x01,0x03,0x01,0x02,0x01,0x06,0x01 }, // D-10 525/50 NTSC 30mb/s
 };
 
-static const uint8_t uuid_base[]            = { 0xAD,0xAB,0x44,0x24,0x2f,0x25,0x4d,0xc7,0x92,0xff,0x29,0xbd };
+static const uint8_t product_uid[]          = { 0xAD,0xAB,0x44,0x24,0x2f,0x25,0x4d,0xc7,0x92,0xff,0x29,0xbd,0x00,0x0c,0x00,0x02};
+static const uint8_t uuid_base[]            = { 0xAD,0xAB,0x44,0x24,0x2f,0x25,0x4d,0xc7,0x92,0xff };
 static const uint8_t umid_ul[]              = { 0x06,0x0A,0x2B,0x34,0x01,0x01,0x01,0x05,0x01,0x01,0x0D,0x00,0x13 };
 
 /**
@@ -424,9 +425,9 @@ typedef struct MXFContext {
 
 static void mxf_write_uuid(AVIOContext *pb, enum MXFMetadataSetType type, int value)
 {
-    avio_write(pb, uuid_base, 12);
+    avio_write(pb, uuid_base, 10);
     avio_wb16(pb, type);
-    avio_wb16(pb, value);
+    avio_wb32(pb, value);
 }
 
 static void mxf_write_umid(AVFormatContext *s, int type)
@@ -797,7 +798,7 @@ static void mxf_write_identification(AVFormatContext *s)
 
     // write product uid
     mxf_write_local_tag(s, 16, 0x3C05);
-    mxf_write_uuid(pb, Identification, 2);
+    avio_write(pb, product_uid, 16);
 
     // modification date
     mxf_write_local_tag(s, 8, 0x3C06);
diff --git a/tests/ref/fate/copy-trac4914 b/tests/ref/fate/copy-trac4914
index 743dc8c055..0ed039bf37 100644
--- a/tests/ref/fate/copy-trac4914
+++ b/tests/ref/fate/copy-trac4914
@@ -1,4 +1,4 @@
-f5150fb82c1bb5a90906fce93dcc3f76 *tests/data/fate/copy-trac4914.mxf
+a0f68fa1d9ed5d3030d8244ea0b0299a *tests/data/fate/copy-trac4914.mxf
 561721 tests/data/fate/copy-trac4914.mxf
 #tb 0: 1001/30000
 #media_type 0: video
diff --git a/tests/ref/fate/mxf-d10-user-comments b/tests/ref/fate/mxf-d10-user-comments
index 64a2dec463..1b59beec7c 100644
--- a/tests/ref/fate/mxf-d10-user-comments
+++ b/tests/ref/fate/mxf-d10-user-comments
@@ -1,4 +1,4 @@
-6dc13ae283257e898e069e5041ac8435 *tests/data/fate/mxf-d10-user-comments.mxf_d10
+8c831b8e01aa4c86e98ab87930d06f3e *tests/data/fate/mxf-d10-user-comments.mxf_d10
 3782189 tests/data/fate/mxf-d10-user-comments.mxf_d10
 #extradata 0:       34, 0x716b05c4
 #tb 0: 1/25
@@ -13,8 +13,8 @@
 0,          3,          4,        1,   150000, 0x9bbe6feb, F=0x0
 [FORMAT]
 TAG:operational_pattern_ul=060e2b34.04010101.0d010201.01010900
-TAG:uid=adab4424-2f25-4dc7-92ff-29bd000c0000
-TAG:generation_uid=adab4424-2f25-4dc7-92ff-29bd000c0001
+TAG:uid=adab4424-2f25-4dc7-92ff-000c00000000
+TAG:generation_uid=adab4424-2f25-4dc7-92ff-000c00000001
 TAG:company_name=FATE-company
 TAG:product_name=FATE-test
 TAG:product_version_num=0.0.0.0.0
diff --git a/tests/ref/fate/mxf-opatom-user-comments b/tests/ref/fate/mxf-opatom-user-comments
index ec4fdff425..14a1cee1f2 100644
--- a/tests/ref/fate/mxf-opatom-user-comments
+++ b/tests/ref/fate/mxf-opatom-user-comments
@@ -1 +1 @@
-8475bebf3448a972ae89ba59309fd7d6
+d40b64e492133c74faa07e605eb65b2f
diff --git a/tests/ref/fate/mxf-reel_name b/tests/ref/fate/mxf-reel_name
index d50f0f6990..f3bafcc118 100644
--- a/tests/ref/fate/mxf-reel_name
+++ b/tests/ref/fate/mxf-reel_name
@@ -1 +1 @@
-ce49a0361d3f79106e1952d387eace51
+75e0ac14d5632d709bd805f1cacb1fbb
diff --git a/tests/ref/fate/mxf-user-comments b/tests/ref/fate/mxf-user-comments
index 5fcdc5806a..b4c78744b0 100644
--- a/tests/ref/fate/mxf-user-comments
+++ b/tests/ref/fate/mxf-user-comments
@@ -1 +1 @@
-956f653cd75e1a319569caec9df81b4f
+620ae205fefbb6dba74418f357a62c36
diff --git a/tests/ref/fate/time_base b/tests/ref/fate/time_base
index 28815d0828..fd6cac53fc 100644
--- a/tests/ref/fate/time_base
+++ b/tests/ref/fate/time_base
@@ -1 +1 @@
-78ac0348027b75d73acb8bea14e67a59
+d408aba82d62a90ed7f46a1999b014f1
diff --git a/tests/ref/lavf/mxf b/tests/ref/lavf/mxf
index 21bf2be513..3f0c74818a 100644
--- a/tests/ref/lavf/mxf
+++ b/tests/ref/lavf/mxf
@@ -1,9 +1,9 @@
-8938d5c4a396ff1b24d10d4f917ae1c9 *tests/data/lavf/lavf.mxf
+9ec1ad83b3400de11ca2f70b3b2d4c4d *tests/data/lavf/lavf.mxf
 526393 tests/data/lavf/lavf.mxf
 tests/data/lavf/lavf.mxf CRC=0x8dddfaab
-93ea2cfdf5dda7fffdc0d2fdcfb6a9a4 *tests/data/lavf/lavf.mxf
+546eb8c864c0d76c6a9d5303701e9031 *tests/data/lavf/lavf.mxf
 561721 tests/data/lavf/lavf.mxf
 tests/data/lavf/lavf.mxf CRC=0x96ff1b48
-87bdf844ae34bcc758e44419e80177a0 *tests/data/lavf/lavf.mxf
+5bd0ce691510e6fae969886c32ad7a14 *tests/data/lavf/lavf.mxf
 526393 tests/data/lavf/lavf.mxf
 tests/data/lavf/lavf.mxf CRC=0x8dddfaab
diff --git a/tests/ref/lavf/mxf_d10 b/tests/ref/lavf/mxf_d10
index 47ef244cb1..4644c3424d 100644
--- a/tests/ref/lavf/mxf_d10
+++ b/tests/ref/lavf/mxf_d10
@@ -1,3 +1,3 @@
-7f16902e28718c2a92bc082400a1c6ee *tests/data/lavf/lavf.mxf_d10
+74269c0a64b19269b127f64f3ce7fa6a *tests/data/lavf/lavf.mxf_d10
 5332013 tests/data/lavf/lavf.mxf_d10
 tests/data/lavf/lavf.mxf_d10 CRC=0x6c74d488
diff --git a/tests/ref/lavf/mxf_dv25 b/tests/ref/lavf/mxf_dv25
index 92509cf1f4..5022f1f62d 100644
--- a/tests/ref/lavf/mxf_dv25
+++ b/tests/ref/lavf/mxf_dv25
@@ -1,3 +1,3 @@
-106e33eb1634595623f0334e92204b65 *tests/data/lavf/lavf.mxf_dv25
+3339def72599c79ad5860c6860cc3123 *tests/data/lavf/lavf.mxf_dv25
 3834413 tests/data/lavf/lavf.mxf_dv25
 tests/data/lavf/lavf.mxf_dv25 CRC=0xbdaf7f52
diff --git a/tests/ref/lavf/mxf_dvcpro50 b/tests/ref/lavf/mxf_dvcpro50
index 2d569b0553..5be1b6875e 100644
--- a/tests/ref/lavf/mxf_dvcpro50
+++ b/tests/ref/lavf/mxf_dvcpro50
@@ -1,3 +1,3 @@
-3d5a303c22666996700f0e8f6e4cb938 *tests/data/lavf/lavf.mxf_dvcpro50
+8494122b2a8e8e5998ace60d4d508d46 *tests/data/lavf/lavf.mxf_dvcpro50
 7431213 tests/data/lavf/lavf.mxf_dvcpro50
 tests/data/lavf/lavf.mxf_dvcpro50 CRC=0xe3bbe4b4
diff --git a/tests/ref/lavf/mxf_opatom b/tests/ref/lavf/mxf_opatom
index e5c5276835..75f85b604e 100644
--- a/tests/ref/lavf/mxf_opatom
+++ b/tests/ref/lavf/mxf_opatom
@@ -1,3 +1,3 @@
-b2da9e38d692fd1866101ebae273bfc0 *tests/data/lavf/lavf.mxf_opatom
+215ea72602bfeb70e99fc9d79fef073c *tests/data/lavf/lavf.mxf_opatom
 4717625 tests/data/lavf/lavf.mxf_opatom
 tests/data/lavf/lavf.mxf_opatom CRC=0xb13ba2f8
diff --git a/tests/ref/lavf/mxf_opatom_audio b/tests/ref/lavf/mxf_opatom_audio
index 97362e7aa4..0e034991c0 100644
--- a/tests/ref/lavf/mxf_opatom_audio
+++ b/tests/ref/lavf/mxf_opatom_audio
@@ -1,3 +1,3 @@
-c356a3fdd49a1e015961678e837c12bb *tests/data/lavf/lavf.mxf_opatom_audio
+633b6d373009416f214edcf456208580 *tests/data/lavf/lavf.mxf_opatom_audio
 102969 tests/data/lavf/lavf.mxf_opatom_audio
 tests/data/lavf/lavf.mxf_opatom_audio CRC=0xd155c6ff
-- 
2.31.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/2] avformat/mxfenc: do not write index tables with the same InstanceUID

[Marton Balint]

Only index tables repeating previous index tables should use the same
InstaceUID. Use the index start position when generating the InstanceUID to fix
this.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/mxfenc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index ba8e7babfb..5b972eadaa 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -1757,7 +1757,7 @@ static void mxf_write_index_table_segment(AVFormatContext *s)
 
     // instance id
     mxf_write_local_tag(s, 16, 0x3C0A);
-    mxf_write_uuid(pb, IndexTableSegment, 0);
+    mxf_write_uuid(pb, IndexTableSegment, mxf->last_indexed_edit_unit);
 
     // index edit rate
     mxf_write_local_tag(s, 8, 0x3F0B);
-- 
2.31.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] avformat/mxfenc: do not write index tables with the same InstanceUID

[Tomas Härdin]

mån 2022-03-14 klockan 19:49 +0100 skrev Marton Balint:
> Only index tables repeating previous index tables should use the same
> InstaceUID. Use the index start position when generating the
> InstanceUID to fix
> this.
> 
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavformat/mxfenc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index ba8e7babfb..5b972eadaa 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -1757,7 +1757,7 @@ static void
> mxf_write_index_table_segment(AVFormatContext *s)
>  
>      // instance id
>      mxf_write_local_tag(s, 16, 0x3C0A);
> -    mxf_write_uuid(pb, IndexTableSegment, 0);
> +    mxf_write_uuid(pb, IndexTableSegment, mxf-
> >last_indexed_edit_unit);

Two things: yes, it is good that this fixes the same InstanceUID being
reused. But more importantly, we should not be writing files with over
65536 partitions!

This has been bugging me for quite some time. Honestly I don't know why
the decision was taken initially to write indices every 10 seconds. In
any use-case where seeks are moderately expensive working with files
produced by mxfenc is a nightmare. Prime example being HTTP.

If we do still need to keep writing partitions this way, can we repeat
the IndexTableSegments in the footer so the entire file doesn't have to
be scanned?

/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-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] avformat/mxfenc: do not write index tables with the same InstanceUID

[Marton Balint]

On Mon, 14 Mar 2022, Tomas Härdin wrote:

> mån 2022-03-14 klockan 19:49 +0100 skrev Marton Balint:
>> Only index tables repeating previous index tables should use the same
>> InstaceUID. Use the index start position when generating the
>> InstanceUID to fix
>> this.
>> 
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  libavformat/mxfenc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
>> index ba8e7babfb..5b972eadaa 100644
>> --- a/libavformat/mxfenc.c
>> +++ b/libavformat/mxfenc.c
>> @@ -1757,7 +1757,7 @@ static void
>> mxf_write_index_table_segment(AVFormatContext *s)
>>  
>>      // instance id
>>      mxf_write_local_tag(s, 16, 0x3C0A);
>> -    mxf_write_uuid(pb, IndexTableSegment, 0);
>> +    mxf_write_uuid(pb, IndexTableSegment, mxf-
>> >last_indexed_edit_unit);
>
> Two things: yes, it is good that this fixes the same InstanceUID being
> reused. But more importantly, we should not be writing files with over
> 65536 partitions!

last_indexed_edit_unit is frame based not partition based, so it can 
overflow 65536 realtively easily, that is why I submitted patch 1.

>
> This has been bugging me for quite some time. Honestly I don't know why
> the decision was taken initially to write indices every 10 seconds. In
> any use-case where seeks are moderately expensive working with files
> produced by mxfenc is a nightmare. Prime example being HTTP.

The 10 second body partition limit is coming from some specification 
(XDCAM HD?), so this is kind of intentional.

>
> If we do still need to keep writing partitions this way, can we repeat
> the IndexTableSegments in the footer so the entire file doesn't have to
> be scanned?

Yeah, that is what smart tools like bmxtools are doing.

Regards,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] avformat/mxfenc: do not write index tables with the same InstanceUID

[Tomas Härdin]

mån 2022-03-14 klockan 20:54 +0100 skrev Marton Balint:
> 
> 
> On Mon, 14 Mar 2022, Tomas Härdin wrote:
> 
> > mån 2022-03-14 klockan 19:49 +0100 skrev Marton Balint:
> > > Only index tables repeating previous index tables should use the
> > > same
> > > InstaceUID. Use the index start position when generating the
> > > InstanceUID to fix
> > > this.
> > > 
> > > Signed-off-by: Marton Balint <cus@passwd.hu>
> > > ---
> > >  libavformat/mxfenc.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> > > index ba8e7babfb..5b972eadaa 100644
> > > --- a/libavformat/mxfenc.c
> > > +++ b/libavformat/mxfenc.c
> > > @@ -1757,7 +1757,7 @@ static void
> > > mxf_write_index_table_segment(AVFormatContext *s)
> > >  
> > >      // instance id
> > >      mxf_write_local_tag(s, 16, 0x3C0A);
> > > -    mxf_write_uuid(pb, IndexTableSegment, 0);
> > > +    mxf_write_uuid(pb, IndexTableSegment, mxf-
> > > > last_indexed_edit_unit);
> > 
> > Two things: yes, it is good that this fixes the same InstanceUID
> > being
> > reused. But more importantly, we should not be writing files with
> > over
> > 65536 partitions!
> 
> last_indexed_edit_unit is frame based not partition based, so it can 
> overflow 65536 realtively easily, that is why I submitted patch 1.

Right. But we could use the partition number instead.

> 
> > 
> > This has been bugging me for quite some time. Honestly I don't know
> > why
> > the decision was taken initially to write indices every 10 seconds.
> > In
> > any use-case where seeks are moderately expensive working with
> > files
> > produced by mxfenc is a nightmare. Prime example being HTTP.
> 
> The 10 second body partition limit is coming from some specification 
> (XDCAM HD?), so this is kind of intentional.
> 
> > 
> > If we do still need to keep writing partitions this way, can we
> > repeat
> > the IndexTableSegments in the footer so the entire file doesn't
> > have to
> > be scanned?
> 
> Yeah, that is what smart tools like bmxtools are doing.

If XDCAM requires this amount of partitions then yeah, probably write
the index tables twice. That way a smart reader should be able to
figure out that it doesn't need to read more than the header, RIP and
footer.

/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-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] avformat/mxfenc: do not write index tables with the same InstanceUID

[Marton Balint]

On Mon, 14 Mar 2022, Tomas Härdin wrote:

> mån 2022-03-14 klockan 20:54 +0100 skrev Marton Balint:
>> 
>> 
>> On Mon, 14 Mar 2022, Tomas Härdin wrote:
>> 
>> > mån 2022-03-14 klockan 19:49 +0100 skrev Marton Balint:
>> > > Only index tables repeating previous index tables should use the
>> > > same
>> > > InstaceUID. Use the index start position when generating the
>> > > InstanceUID to fix
>> > > this.
>> > > 
>> > > Signed-off-by: Marton Balint <cus@passwd.hu>
>> > > ---
>> > >  libavformat/mxfenc.c | 2 +-
>> > >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > > 
>> > > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
>> > > index ba8e7babfb..5b972eadaa 100644
>> > > --- a/libavformat/mxfenc.c
>> > > +++ b/libavformat/mxfenc.c
>> > > @@ -1757,7 +1757,7 @@ static void
>> > > mxf_write_index_table_segment(AVFormatContext *s)
>> > >  
>> > >      // instance id
>> > >      mxf_write_local_tag(s, 16, 0x3C0A);
>> > > -    mxf_write_uuid(pb, IndexTableSegment, 0);
>> > > +    mxf_write_uuid(pb, IndexTableSegment, mxf-
>> > > > last_indexed_edit_unit);
>> > 
>> > Two things: yes, it is good that this fixes the same InstanceUID
>> > being
>> > reused. But more importantly, we should not be writing files with
>> > over
>> > 65536 partitions!
>> 
>> last_indexed_edit_unit is frame based not partition based, so it can 
>> overflow 65536 realtively easily, that is why I submitted patch 1.
>
> Right. But we could use the partition number instead.

Well, we could use mxf->body_partitions_count but it is not trivial to see 
that it will work for all cases. For simple indexes, we rewrite the index 
table in the footer when writing the mxf header, opatom may follow another 
layout, so it just felt less error-prone to use actually the start offset 
of the index.

>
>> 
>> > 
>> > This has been bugging me for quite some time. Honestly I don't know
>> > why
>> > the decision was taken initially to write indices every 10 seconds.
>> > In
>> > any use-case where seeks are moderately expensive working with
>> > files
>> > produced by mxfenc is a nightmare. Prime example being HTTP.
>> 
>> The 10 second body partition limit is coming from some specification 
>> (XDCAM HD?), so this is kind of intentional.
>> 
>> > 
>> > If we do still need to keep writing partitions this way, can we
>> > repeat
>> > the IndexTableSegments in the footer so the entire file doesn't
>> > have to
>> > be scanned?
>> 
>> Yeah, that is what smart tools like bmxtools are doing.
>
> If XDCAM requires this amount of partitions then yeah, probably write
> the index tables twice. That way a smart reader should be able to
> figure out that it doesn't need to read more than the header, RIP and
> footer.

Sure, but this can be another patch.

Thanks,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] avformat/mxfenc: do not write index tables with the same InstanceUID

[Tomas Härdin]

mån 2022-03-14 klockan 21:44 +0100 skrev Marton Balint:
> 
> 
> On Mon, 14 Mar 2022, Tomas Härdin wrote:
> 
> > mån 2022-03-14 klockan 20:54 +0100 skrev Marton Balint:
> > > 
> > > 
> > > On Mon, 14 Mar 2022, Tomas Härdin wrote:
> > > 
> > > > mån 2022-03-14 klockan 19:49 +0100 skrev Marton Balint:
> > > > > Only index tables repeating previous index tables should use
> > > > > the
> > > > > same
> > > > > InstaceUID. Use the index start position when generating the
> > > > > InstanceUID to fix
> > > > > this.
> > > > > 
> > > > > Signed-off-by: Marton Balint <cus@passwd.hu>
> > > > > ---
> > > > >  libavformat/mxfenc.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> > > > > index ba8e7babfb..5b972eadaa 100644
> > > > > --- a/libavformat/mxfenc.c
> > > > > +++ b/libavformat/mxfenc.c
> > > > > @@ -1757,7 +1757,7 @@ static void
> > > > > mxf_write_index_table_segment(AVFormatContext *s)
> > > > >  
> > > > >      // instance id
> > > > >      mxf_write_local_tag(s, 16, 0x3C0A);
> > > > > -    mxf_write_uuid(pb, IndexTableSegment, 0);
> > > > > +    mxf_write_uuid(pb, IndexTableSegment, mxf-
> > > > > > last_indexed_edit_unit);
> > > > 
> > > > Two things: yes, it is good that this fixes the same
> > > > InstanceUID
> > > > being
> > > > reused. But more importantly, we should not be writing files
> > > > with
> > > > over
> > > > 65536 partitions!
> > > 
> > > last_indexed_edit_unit is frame based not partition based, so it
> > > can 
> > > overflow 65536 realtively easily, that is why I submitted patch
> > > 1.
> > 
> > Right. But we could use the partition number instead.
> 
> Well, we could use mxf->body_partitions_count but it is not trivial
> to see 
> that it will work for all cases.

I don't see why not. But upping to 32-bit is easy anyways.

> For simple indexes, we rewrite the index 
> table in the footer when writing the mxf header, opatom may follow
> another 
> layout, so it just felt less error-prone to use actually the start
> offset 
> of the index.

We only need to do this for frame wrapping. And yeah that can be a
separate patch.

/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-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] avformat/mxfenc: do not write index tables with the same InstanceUID

[Marton Balint]

On Wed, 16 Mar 2022, Tomas Härdin wrote:

> mån 2022-03-14 klockan 21:44 +0100 skrev Marton Balint:
>> 
>> 
>> On Mon, 14 Mar 2022, Tomas Härdin wrote:
>> 
>> > mån 2022-03-14 klockan 20:54 +0100 skrev Marton Balint:
>> > > 
>> > > 
>> > > On Mon, 14 Mar 2022, Tomas Härdin wrote:
>> > > 
>> > > > mån 2022-03-14 klockan 19:49 +0100 skrev Marton Balint:
>> > > > > Only index tables repeating previous index tables should use
>> > > > > the
>> > > > > same
>> > > > > InstaceUID. Use the index start position when generating the
>> > > > > InstanceUID to fix
>> > > > > this.
>> > > > > 
>> > > > > Signed-off-by: Marton Balint <cus@passwd.hu>
>> > > > > ---
>> > > > >  libavformat/mxfenc.c | 2 +-
>> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > > > > 
>> > > > > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
>> > > > > index ba8e7babfb..5b972eadaa 100644
>> > > > > --- a/libavformat/mxfenc.c
>> > > > > +++ b/libavformat/mxfenc.c
>> > > > > @@ -1757,7 +1757,7 @@ static void
>> > > > > mxf_write_index_table_segment(AVFormatContext *s)
>> > > > >  
>> > > > >      // instance id
>> > > > >      mxf_write_local_tag(s, 16, 0x3C0A);
>> > > > > -    mxf_write_uuid(pb, IndexTableSegment, 0);
>> > > > > +    mxf_write_uuid(pb, IndexTableSegment, mxf-
>> > > > > > last_indexed_edit_unit);
>> > > > 
>> > > > Two things: yes, it is good that this fixes the same
>> > > > InstanceUID
>> > > > being
>> > > > reused. But more importantly, we should not be writing files
>> > > > with
>> > > > over
>> > > > 65536 partitions!
>> > > 
>> > > last_indexed_edit_unit is frame based not partition based, so it
>> > > can 
>> > > overflow 65536 realtively easily, that is why I submitted patch
>> > > 1.
>> > 
>> > Right. But we could use the partition number instead.
>> 
>> Well, we could use mxf->body_partitions_count but it is not trivial
>> to see 
>> that it will work for all cases.
>
> I don't see why not. But upping to 32-bit is easy anyways.

I tried, but body partition count is the same for the last body partition 
and for the footer partition, both having different index tables...

So I still find it more starightforward to use index start position 
instead of some magic to find out the proper partition count, is it fine 
with you?

Thanks,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] avformat/mxfenc: do not write index tables with the same InstanceUID

[Tomas Härdin]

ons 2022-03-16 klockan 20:38 +0100 skrev Marton Balint:
> 
> 
> On Wed, 16 Mar 2022, Tomas Härdin wrote:
> 
> > mån 2022-03-14 klockan 21:44 +0100 skrev Marton Balint:
> > > 
> > > 
> > > On Mon, 14 Mar 2022, Tomas Härdin wrote:
> > > 
> > > > mån 2022-03-14 klockan 20:54 +0100 skrev Marton Balint:
> > > > > 
> > > > > 
> > > > > On Mon, 14 Mar 2022, Tomas Härdin wrote:
> > > > > 
> > > > > > mån 2022-03-14 klockan 19:49 +0100 skrev Marton Balint:
> > > > > > > Only index tables repeating previous index tables should
> > > > > > > use
> > > > > > > the
> > > > > > > same
> > > > > > > InstaceUID. Use the index start position when generating
> > > > > > > the
> > > > > > > InstanceUID to fix
> > > > > > > this.
> > > > > > > 
> > > > > > > Signed-off-by: Marton Balint <cus@passwd.hu>
> > > > > > > ---
> > > > > > >  libavformat/mxfenc.c | 2 +-
> > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> > > > > > > index ba8e7babfb..5b972eadaa 100644
> > > > > > > --- a/libavformat/mxfenc.c
> > > > > > > +++ b/libavformat/mxfenc.c
> > > > > > > @@ -1757,7 +1757,7 @@ static void
> > > > > > > mxf_write_index_table_segment(AVFormatContext *s)
> > > > > > >  
> > > > > > >      // instance id
> > > > > > >      mxf_write_local_tag(s, 16, 0x3C0A);
> > > > > > > -    mxf_write_uuid(pb, IndexTableSegment, 0);
> > > > > > > +    mxf_write_uuid(pb, IndexTableSegment, mxf-
> > > > > > > > last_indexed_edit_unit);
> > > > > > 
> > > > > > Two things: yes, it is good that this fixes the same
> > > > > > InstanceUID
> > > > > > being
> > > > > > reused. But more importantly, we should not be writing
> > > > > > files
> > > > > > with
> > > > > > over
> > > > > > 65536 partitions!
> > > > > 
> > > > > last_indexed_edit_unit is frame based not partition based, so
> > > > > it
> > > > > can 
> > > > > overflow 65536 realtively easily, that is why I submitted
> > > > > patch
> > > > > 1.
> > > > 
> > > > Right. But we could use the partition number instead.
> > > 
> > > Well, we could use mxf->body_partitions_count but it is not
> > > trivial
> > > to see 
> > > that it will work for all cases.
> > 
> > I don't see why not. But upping to 32-bit is easy anyways.
> 
> I tried, but body partition count is the same for the last body
> partition 
> and for the footer partition, both having different index tables...
> 
> So I still find it more starightforward to use index start position 
> instead of some magic to find out the proper partition count, is it
> fine 
> with you?

I mean it shouldn't matter so long as its unique. So sure

/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-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] avformat/mxfenc: do not write index tables with the same InstanceUID

[Marton Balint]

On Wed, 16 Mar 2022, Tomas Härdin wrote:

> ons 2022-03-16 klockan 20:38 +0100 skrev Marton Balint:
>> 
>> 
>> On Wed, 16 Mar 2022, Tomas Härdin wrote:
>> 
>> > mån 2022-03-14 klockan 21:44 +0100 skrev Marton Balint:
>> > > 
>> > > 
>> > > On Mon, 14 Mar 2022, Tomas Härdin wrote:
>> > > 
>> > > > mån 2022-03-14 klockan 20:54 +0100 skrev Marton Balint:
>> > > > > 
>> > > > > 
>> > > > > On Mon, 14 Mar 2022, Tomas Härdin wrote:
>> > > > > 
>> > > > > > mån 2022-03-14 klockan 19:49 +0100 skrev Marton Balint:
>> > > > > > > Only index tables repeating previous index tables should
>> > > > > > > use
>> > > > > > > the
>> > > > > > > same
>> > > > > > > InstaceUID. Use the index start position when generating
>> > > > > > > the
>> > > > > > > InstanceUID to fix
>> > > > > > > this.
>> > > > > > > 
>> > > > > > > Signed-off-by: Marton Balint <cus@passwd.hu>
>> > > > > > > ---
>> > > > > > >  libavformat/mxfenc.c | 2 +-
>> > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > > > > > > 
>> > > > > > > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
>> > > > > > > index ba8e7babfb..5b972eadaa 100644
>> > > > > > > --- a/libavformat/mxfenc.c
>> > > > > > > +++ b/libavformat/mxfenc.c
>> > > > > > > @@ -1757,7 +1757,7 @@ static void
>> > > > > > > mxf_write_index_table_segment(AVFormatContext *s)
>> > > > > > >  
>> > > > > > >      // instance id
>> > > > > > >      mxf_write_local_tag(s, 16, 0x3C0A);
>> > > > > > > -    mxf_write_uuid(pb, IndexTableSegment, 0);
>> > > > > > > +    mxf_write_uuid(pb, IndexTableSegment, mxf-
>> > > > > > > > last_indexed_edit_unit);
>> > > > > > 
>> > > > > > Two things: yes, it is good that this fixes the same
>> > > > > > InstanceUID
>> > > > > > being
>> > > > > > reused. But more importantly, we should not be writing
>> > > > > > files
>> > > > > > with
>> > > > > > over
>> > > > > > 65536 partitions!
>> > > > > 
>> > > > > last_indexed_edit_unit is frame based not partition based, so
>> > > > > it
>> > > > > can 
>> > > > > overflow 65536 realtively easily, that is why I submitted
>> > > > > patch
>> > > > > 1.
>> > > > 
>> > > > Right. But we could use the partition number instead.
>> > > 
>> > > Well, we could use mxf->body_partitions_count but it is not
>> > > trivial
>> > > to see 
>> > > that it will work for all cases.
>> > 
>> > I don't see why not. But upping to 32-bit is easy anyways.
>> 
>> I tried, but body partition count is the same for the last body
>> partition 
>> and for the footer partition, both having different index tables...
>> 
>> So I still find it more starightforward to use index start position 
>> instead of some magic to find out the proper partition count, is it
>> fine 
>> with you?
>
> I mean it shouldn't matter so long as its unique. So sure

Ok, applied the series then.

Thanks,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avformat/mxfenc: allow more bits for variable part in uuid generation

[Tomas Härdin]

mån 2022-03-14 klockan 19:49 +0100 skrev Marton Balint:
> Also make sure we do not change the product UID.
> 
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavformat/mxfenc.c                    | 9 +++++----
>  tests/ref/fate/copy-trac4914            | 2 +-
>  tests/ref/fate/mxf-d10-user-comments    | 6 +++---
>  tests/ref/fate/mxf-opatom-user-comments | 2 +-
>  tests/ref/fate/mxf-reel_name            | 2 +-
>  tests/ref/fate/mxf-user-comments        | 2 +-
>  tests/ref/fate/time_base                | 2 +-
>  tests/ref/lavf/mxf                      | 6 +++---
>  tests/ref/lavf/mxf_d10                  | 2 +-
>  tests/ref/lavf/mxf_dv25                 | 2 +-
>  tests/ref/lavf/mxf_dvcpro50             | 2 +-
>  tests/ref/lavf/mxf_opatom               | 2 +-
>  tests/ref/lavf/mxf_opatom_audio         | 2 +-
>  13 files changed, 21 insertions(+), 20 deletions(-)
> 
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index 1e87dc6111..ba8e7babfb 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -227,7 +227,8 @@ static const UID mxf_d10_container_uls[] = {
>      {
> 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x0D,0x01,0x03,0x01,0x02,0x01
> ,0x06,0x01 }, // D-10 525/50 NTSC 30mb/s
>  };
>  
> -static const uint8_t uuid_base[]            = {
> 0xAD,0xAB,0x44,0x24,0x2f,0x25,0x4d,0xc7,0x92,0xff,0x29,0xbd };
> +static const uint8_t product_uid[]          = {
> 0xAD,0xAB,0x44,0x24,0x2f,0x25,0x4d,0xc7,0x92,0xff,0x29,0xbd,0x00,0x0c
> ,0x00,0x02};

Maybe use Identification instead of 0x0C.

> +static const uint8_t uuid_base[]            = {
> 0xAD,0xAB,0x44,0x24,0x2f,0x25,0x4d,0xc7,0x92,0xff };
>  static const uint8_t umid_ul[]              = {
> 0x06,0x0A,0x2B,0x34,0x01,0x01,0x01,0x05,0x01,0x01,0x0D,0x00,0x13 };
>  
>  /**
> @@ -424,9 +425,9 @@ typedef struct MXFContext {
>  
>  static void mxf_write_uuid(AVIOContext *pb, enum MXFMetadataSetType
> type, int value)
>  {
> -    avio_write(pb, uuid_base, 12);
> +    avio_write(pb, uuid_base, 10);
>      avio_wb16(pb, type);
> -    avio_wb16(pb, value);
> +    avio_wb32(pb, value);
>  }
>  
>  static void mxf_write_umid(AVFormatContext *s, int type)
> @@ -797,7 +798,7 @@ static void
> mxf_write_identification(AVFormatContext *s)
>  
>      // write product uid
>      mxf_write_local_tag(s, 16, 0x3C05);
> -    mxf_write_uuid(pb, Identification, 2);
> +    avio_write(pb, product_uid, 16);

For those wondering, the purpose of this not using mxf_write_uuid() is
likely to keep ProductUID the same after this patch. This is of course
good if a bit ugly since the calls with 0 and 1 are still there. Oh
well.

/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-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avformat/mxfenc: allow more bits for variable part in uuid generation

[Marton Balint]

On Mon, 14 Mar 2022, Tomas Härdin wrote:

> mån 2022-03-14 klockan 19:49 +0100 skrev Marton Balint:
>> Also make sure we do not change the product UID.
>> 
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  libavformat/mxfenc.c                    | 9 +++++----
>>  tests/ref/fate/copy-trac4914            | 2 +-
>>  tests/ref/fate/mxf-d10-user-comments    | 6 +++---
>>  tests/ref/fate/mxf-opatom-user-comments | 2 +-
>>  tests/ref/fate/mxf-reel_name            | 2 +-
>>  tests/ref/fate/mxf-user-comments        | 2 +-
>>  tests/ref/fate/time_base                | 2 +-
>>  tests/ref/lavf/mxf                      | 6 +++---
>>  tests/ref/lavf/mxf_d10                  | 2 +-
>>  tests/ref/lavf/mxf_dv25                 | 2 +-
>>  tests/ref/lavf/mxf_dvcpro50             | 2 +-
>>  tests/ref/lavf/mxf_opatom               | 2 +-
>>  tests/ref/lavf/mxf_opatom_audio         | 2 +-
>>  13 files changed, 21 insertions(+), 20 deletions(-)
>> 
>> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
>> index 1e87dc6111..ba8e7babfb 100644
>> --- a/libavformat/mxfenc.c
>> +++ b/libavformat/mxfenc.c
>> @@ -227,7 +227,8 @@ static const UID mxf_d10_container_uls[] = {
>>      {
>> 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x0D,0x01,0x03,0x01,0x02,0x01
>> ,0x06,0x01 }, // D-10 525/50 NTSC 30mb/s
>>  };
>>  
>> -static const uint8_t uuid_base[]            = {
>> 0xAD,0xAB,0x44,0x24,0x2f,0x25,0x4d,0xc7,0x92,0xff,0x29,0xbd };
>> +static const uint8_t product_uid[]          = {
>> 0xAD,0xAB,0x44,0x24,0x2f,0x25,0x4d,0xc7,0x92,0xff,0x29,0xbd,0x00,0x0c
>> ,0x00,0x02};
>
> Maybe use Identification instead of 0x0C.

Actually I'd rather keep it 0x0C, Identification value might change (if 
MXFMetadataSetType enum is reordered in mxf.h), and we don't want 
ProductUID to change even then...

Thanks,
Marton

>
>> +static const uint8_t uuid_base[]            = {
>> 0xAD,0xAB,0x44,0x24,0x2f,0x25,0x4d,0xc7,0x92,0xff };
>>  static const uint8_t umid_ul[]              = {
>> 0x06,0x0A,0x2B,0x34,0x01,0x01,0x01,0x05,0x01,0x01,0x0D,0x00,0x13 };
>>  
>>  /**
>> @@ -424,9 +425,9 @@ typedef struct MXFContext {
>>  
>>  static void mxf_write_uuid(AVIOContext *pb, enum MXFMetadataSetType
>> type, int value)
>>  {
>> -    avio_write(pb, uuid_base, 12);
>> +    avio_write(pb, uuid_base, 10);
>>      avio_wb16(pb, type);
>> -    avio_wb16(pb, value);
>> +    avio_wb32(pb, value);
>>  }
>>  
>>  static void mxf_write_umid(AVFormatContext *s, int type)
>> @@ -797,7 +798,7 @@ static void
>> mxf_write_identification(AVFormatContext *s)
>>  
>>      // write product uid
>>      mxf_write_local_tag(s, 16, 0x3C05);
>> -    mxf_write_uuid(pb, Identification, 2);
>> +    avio_write(pb, product_uid, 16);
>
> For those wondering, the purpose of this not using mxf_write_uuid() is
> likely to keep ProductUID the same after this patch. This is of course
> good if a bit ugly since the calls with 0 and 1 are still there. Oh
> well.
>
> /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-request@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-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avformat/mxfenc: allow more bits for variable part in uuid generation

[Tomas Härdin]

mån 2022-03-14 klockan 20:57 +0100 skrev Marton Balint:
> 
> 
> On Mon, 14 Mar 2022, Tomas Härdin wrote:
> 
> > mån 2022-03-14 klockan 19:49 +0100 skrev Marton Balint:
> > > Also make sure we do not change the product UID.
> > > 
> > > Signed-off-by: Marton Balint <cus@passwd.hu>
> > > ---
> > >  libavformat/mxfenc.c                    | 9 +++++----
> > >  tests/ref/fate/copy-trac4914            | 2 +-
> > >  tests/ref/fate/mxf-d10-user-comments    | 6 +++---
> > >  tests/ref/fate/mxf-opatom-user-comments | 2 +-
> > >  tests/ref/fate/mxf-reel_name            | 2 +-
> > >  tests/ref/fate/mxf-user-comments        | 2 +-
> > >  tests/ref/fate/time_base                | 2 +-
> > >  tests/ref/lavf/mxf                      | 6 +++---
> > >  tests/ref/lavf/mxf_d10                  | 2 +-
> > >  tests/ref/lavf/mxf_dv25                 | 2 +-
> > >  tests/ref/lavf/mxf_dvcpro50             | 2 +-
> > >  tests/ref/lavf/mxf_opatom               | 2 +-
> > >  tests/ref/lavf/mxf_opatom_audio         | 2 +-
> > >  13 files changed, 21 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> > > index 1e87dc6111..ba8e7babfb 100644
> > > --- a/libavformat/mxfenc.c
> > > +++ b/libavformat/mxfenc.c
> > > @@ -227,7 +227,8 @@ static const UID mxf_d10_container_uls[] = {
> > >      {
> > > 0x06,0x0E,0x2B,0x34,0x04,0x01,0x01,0x01,0x0D,0x01,0x03,0x01,0x02,
> > > 0x01
> > > ,0x06,0x01 }, // D-10 525/50 NTSC 30mb/s
> > >  };
> > >  
> > > -static const uint8_t uuid_base[]            = {
> > > 0xAD,0xAB,0x44,0x24,0x2f,0x25,0x4d,0xc7,0x92,0xff,0x29,0xbd };
> > > +static const uint8_t product_uid[]          = {
> > > 0xAD,0xAB,0x44,0x24,0x2f,0x25,0x4d,0xc7,0x92,0xff,0x29,0xbd,0x00,
> > > 0x0c
> > > ,0x00,0x02};
> > 
> > Maybe use Identification instead of 0x0C.
> 
> Actually I'd rather keep it 0x0C, Identification value might change
> (if 
> MXFMetadataSetType enum is reordered in mxf.h), and we don't want 
> ProductUID to change even then...

Good point

/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-request@ffmpeg.org with subject "unsubscribe".