| Message ID | 20220314184950.640-2-cus@passwd.hu |
|---|---|
| State | Accepted |
| Commit | ffff5bb740b09dafa75b880b7a1e85a793604623 |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avformat/mxfenc: allow more bits for variable part in uuid generation | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_aarch64_jetson | success | Make finished |
| andriy/make_fate_aarch64_jetson | success | Make fate finished |
| andriy/make_armv7_RPi4 | success | Make finished |
| andriy/make_fate_armv7_RPi4 | success | Make fate finished |
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
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
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
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
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
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
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
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
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);
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(-)