[FFmpeg-devel] [PATCH] lavf/mxfenc: Bump EDIT_UNITS_PER_BODY

[FFmpeg-devel] [PATCH] lavf/mxfenc: Bump EDIT_UNITS_PER_BODY

[Tomas Härdin]

[-- Attachment #1: Type: text/plain, Size: 141 bytes --]

Passes FATE because we don't have any tests that mux files with a
whopping 250 frames. Tested with a jpeg2000 Tears of Steel sample.

/Tomas

[-- Attachment #2: 0001-lavf-mxfenc-Bump-EDIT_UNITS_PER_BODY.patch --]
[-- Type: text/x-patch, Size: 931 bytes --]

From ad87019bf1ec7540a43e9a56acaf7adb32c917ee Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git@haerdin.se>
Date: Mon, 13 Feb 2023 10:55:06 +0100
Subject: [PATCH] lavf/mxfenc: Bump EDIT_UNITS_PER_BODY

250 is ridiculously low and leads to excessive partitions and allocations.
This change also makes muxed files smaller, and makes demuxing them faster.
---
 libavformat/mxfenc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index a29d678098..124b5a6b41 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -67,7 +67,7 @@ extern const FFOutputFormat ff_mxf_opatom_muxer;
 #define IS_D10(s)    ((s)->oformat == &ff_mxf_d10_muxer.p)
 #define IS_OPATOM(s) ((s)->oformat == &ff_mxf_opatom_muxer.p)
 
-#define EDIT_UNITS_PER_BODY 250
+#define EDIT_UNITS_PER_BODY (1 << 20)
 #define KAG_SIZE 512
 
 typedef struct MXFIndexEntry {
-- 
2.30.2


[-- Attachment #3: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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] lavf/mxfenc: Bump EDIT_UNITS_PER_BODY

[Marton Balint]

On Mon, 13 Feb 2023, Tomas Härdin wrote:

> Passes FATE because we don't have any tests that mux files with a
> whopping 250 frames. Tested with a jpeg2000 Tears of Steel sample.

Have you checked what BMX does? Because as far as I remember for 
RDD9 (XDCAM HD) this should not exceed 10 seconds.

I suggest adding an option for this with -1 default, and unless the user 
overrides it make it 238 in case of MPEG2 essence, and infinite (0) 
otherwise.

There is also code which allocates the index entries in 
EDIT_UNITS_PER_BODY increments, that probably should be replaced with 
av_dynarray2_add...

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] lavf/mxfenc: Bump EDIT_UNITS_PER_BODY

[Tomas Härdin]

mån 2023-02-13 klockan 20:33 +0100 skrev Marton Balint:
> 
> 
> On Mon, 13 Feb 2023, Tomas Härdin wrote:
> 
> > Passes FATE because we don't have any tests that mux files with a
> > whopping 250 frames. Tested with a jpeg2000 Tears of Steel sample.
> 
> Have you checked what BMX does? Because as far as I remember for 
> RDD9 (XDCAM HD) this should not exceed 10 seconds.

I have not checked BMX. Also such constraints should be explicitly
documented and there should be tests for them. An option like -xdcam
might be appropriate. Currently there is no justification at all in the
code for the choice of 250.

> 
> I suggest adding an option for this with -1 default, and unless the
> user 
> overrides it make it 238 in case of MPEG2 essence, and infinite (0) 
> otherwise.

I wouldn't be opposed to having it use a dynamic array. As for MPEG2,
that does stink a bit but perhaps it avoids causing grief for most
users. Here we again come to the problem that we don't know the
workflows in which mxf* are used, and I don't like that we're cargo
culting defaults.

> 
> There is also code which allocates the index entries in 
> EDIT_UNITS_PER_BODY increments, that probably should be replaced with
> av_dynarray2_add...

That (re)allocation happens at most twice for assuming GOP size <
EDIT_UNITS_PER_BODY

The reason I bring this all up is because opening MXF files muxed by
lavf over HTTP is slow as hell. This will be true for any other storage
where seeking incurs a non-trivial cost. HDDs come to mind. Perhaps
that is really an mxfdec problem, but then parsing becomes even hairier
than it already is. We'd have to binary search the file when seeking,
hoping to find the necessary index table segments on-the-fly..

/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] lavf/mxfenc: Bump EDIT_UNITS_PER_BODY

[Marton Balint]

On Mon, 13 Feb 2023, Tomas Härdin wrote:

> mån 2023-02-13 klockan 20:33 +0100 skrev Marton Balint:
>> 
>> 
>> On Mon, 13 Feb 2023, Tomas Härdin wrote:
>> 
>> > Passes FATE because we don't have any tests that mux files with a
>> > whopping 250 frames. Tested with a jpeg2000 Tears of Steel sample.
>> 
>> Have you checked what BMX does? Because as far as I remember for 
>> RDD9 (XDCAM HD) this should not exceed 10 seconds.
>
> I have not checked BMX. Also such constraints should be explicitly
> documented and there should be tests for them. An option like -xdcam
> might be appropriate. Currently there is no justification at all in the
> code for the choice of 250.

Intent of RDD9 compliance.

>> 
>> I suggest adding an option for this with -1 default, and unless the
>> user 
>> overrides it make it 238 in case of MPEG2 essence, and infinite (0) 
>> otherwise.
>
> I wouldn't be opposed to having it use a dynamic array. As for MPEG2,
> that does stink a bit but perhaps it avoids causing grief for most
> users. Here we again come to the problem that we don't know the
> workflows in which mxf* are used, and I don't like that we're cargo
> culting defaults.

I think we have a pretty good idea that MPEG2 in MXF usually means some 
broadcast realted use, therefore intent of RDD9 compliance by default is 
not insane at all for MPEG2 essence. Can we please keep it as default for 
MPEG2?

>
>> 
>> There is also code which allocates the index entries in 
>> EDIT_UNITS_PER_BODY increments, that probably should be replaced with
>> av_dynarray2_add...
>
> That (re)allocation happens at most twice for assuming GOP size <
> EDIT_UNITS_PER_BODY
>
> The reason I bring this all up is because opening MXF files muxed by
> lavf over HTTP is slow as hell. This will be true for any other storage
> where seeking incurs a non-trivial cost. HDDs come to mind. Perhaps
> that is really an mxfdec problem, but then parsing becomes even hairier
> than it already is. We'd have to binary search the file when seeking,
> hoping to find the necessary index table segments on-the-fly..

A sane muxer duplicates all index table segments in the footer, so a smart 
demuxer can read the whole index from there. Yes, mxfdec problem. I have a 
WIP patch somewhere fixing that.

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] lavf/mxfenc: Bump EDIT_UNITS_PER_BODY

[Tomas Härdin]

mån 2023-02-13 klockan 22:05 +0100 skrev Marton Balint:
> 
> I think we have a pretty good idea that MPEG2 in MXF usually means
> some 
> broadcast realted use, therefore intent of RDD9 compliance by default
> is 
> not insane at all for MPEG2 essence. Can we please keep it as default
> for 
> MPEG2?

If the intent is to follow RDD9 then more things need to be done, among
them erroring out if gop_size > 15. EDIT_UNITS_PER_BODY is also too big
for 24/1.001 Hz essence. Finally the logic around line 2988 is wrong
since it will easily and consistently writes index table segments > 10
seconds:

> if (!mxf->edit_unit_byte_count &&
>     (!mxf->edit_units_count || mxf->edit_units_count >
> EDIT_UNITS_PER_BODY) &&
>     !(ie.flags & 0x33)) { // I-frame, GOP start
> 

This will only work for NTSC because (250+15)/30*1.001 < 8.9s
assuming gop_size <= 15, and it will be > 10s for PAL and 24/1.001 Hz.
The reallocation logic is likely there to compensate for this
wrongness.

The simplest way to remain compliant should therefore be:

* check that gop_size <= 15
* change the above condition to mxf->edit_units_count > 239-gop_size
(maybe -1)

The simplest for the latter is just > 223. If ever the muxer gets a
series of packages that then exceeds the limits set out in RDD 9-2006
then it should complain loudly and terminate so that users don't
accidentally write non-compliant files.

For the allocation stuff, we should make room for 301 EditUnits. If
ever the muxer finds the need to insert a 302nd EditUnit when muxing
MPEG2 then it should error out.

Of course a lot of this could likely be avoided if we just used BMX
instead.

> 
> > 
> > > 
> > > There is also code which allocates the index entries in 
> > > EDIT_UNITS_PER_BODY increments, that probably should be replaced
> > > with
> > > av_dynarray2_add...
> > 
> > That (re)allocation happens at most twice for assuming GOP size <
> > EDIT_UNITS_PER_BODY
> > 
> > The reason I bring this all up is because opening MXF files muxed
> > by
> > lavf over HTTP is slow as hell. This will be true for any other
> > storage
> > where seeking incurs a non-trivial cost. HDDs come to mind. Perhaps
> > that is really an mxfdec problem, but then parsing becomes even
> > hairier
> > than it already is. We'd have to binary search the file when
> > seeking,
> > hoping to find the necessary index table segments on-the-fly..
> 
> A sane muxer duplicates all index table segments in the footer, so a
> smart 
> demuxer can read the whole index from there.

True, though that requires using a dynarray. It also requires smarts
that I don't think mxfdec currently has.

> Yes, mxfdec problem. I have a 
> WIP patch somewhere fixing that.

That would be lovely

/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".