[FFmpeg-devel] [PATCH 1/3] avformat/mxfdec: SMPTE RDD 48:2018 support

[Michael Niedermayer]

On Tue, Jul 19, 2022 at 03:48:59PM +0200, Tomas Härdin wrote:
> mån 2022-07-11 klockan 23:44 +0200 skrev Michael Niedermayer:
> > 
> > +    { {
> > 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09
> > ,0x01,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V0 */
> > +    { {
> > 0x06,0x0e,0x2b,0x34,0x04,0x01,0x01,0x0D,0x04,0x01,0x02,0x02,0x03,0x09
> > ,0x02,0x00 }, 15,       AV_CODEC_ID_FFV1 }, /*FFV1 V1 */
> > +    { {
> 
> Double-checked, these are correct
> 
> > +typedef struct MXFFFV1SubDescriptor {
> > +    MXFMetadataSet meta;
> > +    uint8_t *extradata;
> > +    int extradata_size;
> 
> Is FFV1 extradata size bounded? It so we could avoid an allocation.
> Either way the local set syntax limits this to 64k, see below.

the extradata is extensible so future versions can be bigger.
For the current version there should be a maximum. As the extradata uses
an adaptive range coder it is not trivial to give a tight limit. It would
be easy to give some non tight limit. But iam not sure this has any point
as future versions can be bigger


> 
> > +static const uint8_t mxf_ffv1_extradata[]                  = {
> > 0x06,0x0e,0x2b,0x34,0x01,0x01,0x01,0x0e,0x04,0x01,0x06,0x0c,0x01,0x00
> > ,0x00,0x00 };
> 
> Maybe add a comment // FFV1InitializationMetadata

done


> 
> > +static int mxf_read_ffv1_sub_descriptor(void *arg, AVIOContext *pb,
> > int tag, int size, UID uid, int64_t klv_offset)
> > +{
> > +    MXFFFV1SubDescriptor *ffv1_sub_descriptor = arg;
> > +
> > +    if (IS_KLV_KEY(uid, mxf_ffv1_extradata)) {
> > +        if (ffv1_sub_descriptor->extradata)
> > +            av_log(NULL, AV_LOG_WARNING, "Duplicate
> > ffv1_extradata\n");
> 
> Perhaps we should pass AVFormatContext* along with these functions to
> enable proper logging. Doesn't need to hold up this patch though.
> 
> > +        av_free(ffv1_sub_descriptor->extradata);
> > +        ffv1_sub_descriptor->extradata_size = 0;
> > +        ffv1_sub_descriptor->extradata = av_malloc(size);
> > +        if (!ffv1_sub_descriptor->extradata)
> > +            return AVERROR(ENOMEM);
> > +        ffv1_sub_descriptor->extradata_size = size;
> 
> This could be simplified with av_fast_malloc(), or no allocation at all
> if we use a static sized array.

this was missing AV_INPUT_BUFFER_PADDING_SIZE, added that.
I dont think av_fast_malloc() is a good idea, its a once per stream
allocation, i also dont think a static array is a good idea, there is
no size limit unless you want to limit to a specific version and
compute a worst case bound on a adaptive coder. And then
that worst case would be orders of magnitude bigger than real extradata
because real extradata compresses quite well. While the worst case would
be the case that is biggest and compresses worst. So a static array
would waste space


> 
> > +        avio_read(pb, ffv1_sub_descriptor->extradata, size);
> > +    }
> 
> I presume the other items (GOP size, version number etc) are of no
> interest and can be probed by the decoder.

They are of no interrest to me ATM. Maybe there is some use case for
parsing some mroe values, i do not really know. I would say we add them
when we understand what to do with them.
I presume theres at least one person who saw a use in them being stored
there so i presume theres someone in some standard committee who saw some
use in it. It seems that usecase did not make it into writing in the
part of the specification that i did read


[...]

> 
> > +    { {
> > 0x06,0x0e,0x2b,0x34,0x02,0x53,0x01,0x01,0x0d,0x01,0x01,0x01,0x01,0x01
> > ,0x81,0x03 }, mxf_read_ffv1_sub_descriptor,
> > sizeof(MXFFFV1SubDescriptor), FFV1SubDescriptor },
> 
> The spec says 0x7F not 0x53. 0x53 is used in groups with 2-byte tags

If i put 0x7F with no other change there, it will break demuxing the files i have
I guess i must have copied this from the files without noticing it mismatches
the spec


> rather than full KLVs. The intent here seems to be to use local tags,
> which fortuitously limits extradata_size to 64k. This makes me think
> Amendment 1:2022 is wrong or that 0x7F is just to signal private use
> until it gets rolled into the next version of RDD 48.
> 
> Tables 18 and 23 in S377m-1-2009 say that 0x7F corresponds to "Abstract
> Groups" which are "never encoded as Metadata Sets".
> 
> Reading S336m-2007 it seems one can actually use various lengths and
> tag sizes. 0x53 corresponds to 2 byte length and 2 byte tag. S377m says
> that in addition to this, 0x13 is allowed in MXF which uses ASN.1 BER
> encoded lengths. Don't know if any files in the wild use that. Probably
> not.

couldnt they make this more complex and bizarr?

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20220729/ebab4136/attachment.sig>