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

[Michael Niedermayer <michael@niedermayer.cc>]

[-- Attachment #1.1: Type: text/plain, Size: 4541 bytes --]

On Sat, Apr 01, 2023 at 07:11:45PM +0200, Jerome Martinez wrote:
> On 01/04/2023 17:43, Michael Niedermayer wrote:
> > On Sat, Apr 01, 2023 at 05:20:50PM +0200, Jerome Martinez wrote:
> > > On 01/04/2023 16:37, Michael Niedermayer wrote:
> > > > On Tue, Mar 14, 2023 at 10:52:15AM +0100, Jerome Martinez wrote:
> > > > [...]
> > > > > +
> > > > > +    memset(state, 128, sizeof(state));
> > > > > +    if (st->codecpar->extradata) {
> > > > > +        ff_init_range_decoder(&c, st->codecpar->extradata, st->codecpar->extradata_size);
> > > > > +        ff_build_rac_states(&c, 0.05 * (1LL << 32), 256 - 8);
> > > > > +        v = get_ffv1_unsigned_symbol(&c, state);
> > > > > +        av_assert0(v >= 2);
> > > > > +        if (v > 4) {
> > > > > +            return 0;
> > > > > +        }
> > > > > +        sc->micro_version = get_ffv1_unsigned_symbol(&c, state);
> > > > > +    } else {
> > > > > +        uint8_t keystate = 128;
> > > > > +        ff_init_range_decoder(&c, pkt->data, pkt->size);
> > > > > +        ff_build_rac_states(&c, 0.05 * (1LL << 32), 256 - 8);
> > > > > +        get_rac(&c, &keystate); // keyframe
> > > > > +        v = get_ffv1_unsigned_symbol(&c, state);
> > > > > +        av_assert0(v < 2);
> > > > Are these asserts testing muxer input ?
> > > > if so what ensures that the values are within the asserted range ?
> > > 
> > > My understanding of the code and workflow is that the input is currently
> > > rejected (AV_LOG_ERROR, "invalid version %d in ver01 header\n" and
> > > AV_LOG_ERROR, "Invalid version in global header\n") in ffv1dec.c during the
> > > analysis of this input so before this piece of code is reached.
> > > Could be an AV_LOG_ERROR if preferred.
> > if the encoder writes 5 teh muxer crashes here
> > 
> > V 5me=    0 fps=0.0 q=-0.0 size=       0kB time=00:00:00.00 bitrate=N/A speed=   0x
> > Assertion v < 2 failed at libavformat/mxfenc.c:2505
> 
> 
> I hardcoded version 5 in the encoder based on version 4 (so with extra
> metadata), and I have:
> [mxf @ 0x7fffd52f3100] could not get ffv1 version
> 
> My understanding is that you hardcoded version 5 in the encoder based on
> version 0 or 1 (so without extra metadata), in that case I have the assert,
> and it was on purpose on my side because I understand from ffv1 encoder code
> that any version > 2 should have extra metadata, and if it changes in the
> future this line in MXF muxer should be changed at the same time as FFV1
> encoder code
> 
> But I didn't test and didn't realize that the FFV1 parsing is not done with
> -c:v copy, and if that case I get the assert with an (fake) input with
> version 5 and no extra metadata.
> 
> What about the attached patch? I reused the ffv1 decoder error messages +
> "ffv1 " tip.
> Note: I moved, so less coherency in the code, the error message in
> mxf_parse_ffv1_frame for more precise message depending on the presence or
> not of extra metadata, it could stay as is if we don't care about the
> precision.

>  mxfenc.c |   10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 2b3a2cc619fcaf2dd1da7aa4b566b390eced62a3  0001-avformat-mxfenc-replace-ffv1-version-related-assert-.patch
> From 65f6fba9434fa9ab4b5d2b91c3f930c1e3cb4bef Mon Sep 17 00:00:00 2001
> From: Jerome Martinez <jerome@mediaarea.net>
> Date: Sat, 1 Apr 2023 19:04:25 +0200
> Subject: [PATCH] avformat/mxfenc: replace ffv1 version related assert by error
>  message
> 
> ---
>  libavformat/mxfenc.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index 9eba208ffb..0a1ae5353c 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -2489,8 +2489,8 @@ static int mxf_parse_ffv1_frame(AVFormatContext *s, AVStream *st, AVPacket *pkt)
>          ff_init_range_decoder(&c, st->codecpar->extradata, st->codecpar->extradata_size);
>          ff_build_rac_states(&c, 0.05 * (1LL << 32), 256 - 8);
>          v = get_ffv1_unsigned_symbol(&c, state);
> -        av_assert0(v >= 2);
> -        if (v > 4) {
> +        if (v < 2) {
> +            av_log(s, AV_LOG_ERROR, "Invalid version in ffv1 global header\n");
>              return 0;
>          }

should this not also fail with v > 4 as before ?

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you think the mosad wants you dead since a long time then you are either
wrong or dead since a long time.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: 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".