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

[Michael Niedermayer]

On Wed, Apr 05, 2023 at 02:31:16PM +0200, Michael Niedermayer wrote:
> 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

Changed my mind, ill push it splited in 2. Its more consistent with how
such changes have been done, even when its not needed here

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
-------------- 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/fdf2cc61/attachment.sig>