[FFmpeg-devel] avformat/mxfenc: SMPTE RDD 48:2018 Amd 1:2022 (FFV1 in MXF) support

[Michael Niedermayer]

On Tue, Apr 04, 2023 at 11:37:54PM +0200, Jerome Martinez wrote:
> On 04/04/2023 19:32, Michael Niedermayer wrote:
> > On Tue, Apr 04, 2023 at 04:57:03PM +0200, Jerome Martinez wrote:
> > > On 04/04/2023 16:43, Michael Niedermayer wrote:
> > > > On Mon, Apr 03, 2023 at 12:07:06AM +0200, Jerome Martinez wrote:
> > > > > On 02/04/2023 22:07, Michael Niedermayer wrote:
> > > > > 
> > > > > +        if (f->version == 4 && f->micro_version > 2) {
> > > > > +            av_log(f->avctx, AV_LOG_ERROR, "unsupported version 4 micro_version %d\n",
> > > > > +                f->micro_version);
> > > > > +            return AVERROR_PATCHWELCOME;
> > > > > +        }
> > > > >        }
> > > > >        f->ac = get_symbol(c, state, 0);
> > > > you do not know if the decoder will have any problem with these files
> > > 
> > > But you don't don't if the decoder will have no problem, it seems safer to
> > > me to reject something we are not sure to support.
> > "each new micro-version after this first stable variant is compatible with the previous micro-version: decoders **SHOULD NOT** reject FFV1 bitstreams due to an unknown micro-version equal or above the micro-version considered as stable."
> 
> This sentence is about a stable version, and version 4 is not stable, so
> FFV1 version 4 micro_version 3 is not necessarily compatible with FFV1
> version 4 micro_version 2, and so on, until we freeze the micro-version
> considered as stable. It is actually same for FFV1 version 3 e.g. "if
> (f->version == 3 && f->micro_version > 1 || f->version > 3) get_rac(&fs->c,
> (uint8_t[]) { 129 });" seems to say that version 3 micro_version 2 is not
> compatible with version 3 micro_version 1 (and acceptable, only version 3
> micro_version 5 and above must be compatible with version 3 micro_version 4
> because micro_version 4 is the one flagged as stable).
> Reason I suggested this micro_version check for version 4 (I didn't add it
> for version 3 because version 3 has a micro-version considered as stable).
> Anyway, it is not important to me, just apply the patch you prefer.

Theres a practical problem with rejecting increased micro versions.
That problem is that we would not be able to introduce compatible changes
as even if they are compatible the micro_version itself would break all
decoders


> 
> 
> > 
> > 
> > [...]
> > >   libavcodec/ffv1dec.c |    5 +++++
> > >   libavformat/mxfenc.c |    4 ++++
> > >   2 files changed, 9 insertions(+)
> > > 9b094eb0bd0888725a4a3fac925ef1fa733a48c3  0001-avcodec-ffv1dec-reject-unsupported-ffv1-versions.patch
> > >  From dc0382709e548ef2514198bc866028066134d33e Mon Sep 17 00:00:00 2001
> > > From: Jerome Martinez <jerome at mediaarea.net>
> > > Date: Mon, 3 Apr 2023 00:04:53 +0200
> > > Subject: [PATCH] avcodec/ffv1dec: reject unsupported ffv1 versions
> > > 
> > > And add similar check in libavformat/mxfenc
> > > ---
> > >   libavcodec/ffv1dec.c | 5 +++++
> > >   libavformat/mxfenc.c | 4 ++++
> > >   2 files changed, 9 insertions(+)
> > the patch is mostly ok
> > iam a bit undecided if a decoder change and a muxer bugfix belong in the
> > same patch, so do as you prefer
> 
> I think that both parts are about the same thing (and in the future both
> should be changed at the same time) but it is really subjective, let me know
> if you prefer that I send 2 separated patches and I'll do it, else please
> apply one of the already proposed patch.

The problem is "same" "time"
In the world of libavcodec, "time" is specified in libavcodec/version.h
In the world of libavformat, "time" is specified in libavformat/version.h

Now if neither version is increased then you certainly can not assume
anything and any mixture can occurr. You can have before and after
libs mixed in any way on a system. For many changes this is ok
but sometimes the libavformat change needs the libavcodec feature.
So doing it in the same commit can be done here but it cannot be the
default (future) way to do it

If you bump the minor versions of both libs with a change then you can have
libavcodec  libavformat
before      before
after       before
after       after

If you bump the major versions of both libs with a change then you can have
before      before
after       after

the last variant is hard as major isnt bumped often.

so the way changes are introduced to 2 libs is to do it
first to libavcodec (this tests the after before case)
then subsequently to libavformat (this tests the after after case)
And with such seperate changes you just tested all legal combinations
without any need to think about it :)

OTOH if a commit does both libs together it requires an actual review
of what happens if not both libs are upgraded together on the user side.

The idea is simple, split it in 2 and things get naturally tested
and no need to spend any time on odd lib mixes as the only case is naturally
tested between the 2 commits.

Ill apply one of the patches as its ok here, but it might be "not ok" for
future changes to ffv1 in both libs, it depends on the exact changes

Thanks

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- 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/20230405/08c2732d/attachment.sig>