[FFmpeg-devel] [PATCH] lavf/mxfenc: Use nb_components, not av_pix_fmt_count_planes()

[Tomas Härdin]

tor 2024-10-31 klockan 20:02 +0100 skrev Marton Balint:
> 
> 
> On Tue, 29 Oct 2024, Tomas Härdin wrote:
> 
> > tis 2024-10-29 klockan 12:21 -0300 skrev James Almer:
> > > > From ce4b1dfb97530b242f899e5d1686f98fa83a7698 Mon Sep 17 00:00:00
> > > > 2001
> > > > From: =?UTF-8?q?Tomas=20H=C3=A4rdin?= <git at haerdin.se>
> > > > Date: Tue, 29 Oct 2024 16:13:04 +0100
> > > > Subject: [PATCH] lavf/mxfenc: Use nb_components, not
> > > > av_pix_fmt_count_planes()
> > > > 
> > > > This fixed https://trac.ffmpeg.org/ticket/11267
> > > > ---
> > > >  libavformat/mxfenc.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> > > > index 57be9e6ef6..e66df0fba2 100644
> > > > --- a/libavformat/mxfenc.c
> > > > +++ b/libavformat/mxfenc.c
> > > > @@ -1488,7 +1488,7 @@ static void
> > > > mxf_write_jpeg2000_subdesc(AVFormatContext *s, AVStream *st)
> > > >      MXFStreamContext *sc = st->priv_data;
> > > >      AVIOContext *pb = s->pb;
> > > >      int64_t pos;
> > > > -    int component_count = av_pix_fmt_count_planes(st->codecpar-
> > > > > format);
> > > > +    int component_count = av_pix_fmt_desc_get(st->codecpar-
> > > > > format)->nb_components;
> > > 
> > > I don't think anything ensures that av_pix_fmt_desc_get() here will
> > > not
> > > return NULL, so maybe instead do:
> > > 
> > > > const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(st->codecpar-
> > > > > format);
> > > > int component_count;
> > > > 
> > > > if (!desc)
> > > >     return AVERROR(EINVAL);
> > > > 
> > > > component_count = desc->nb_components;
> > 
> > I can't really see how that would happen, but I suppose it doesn't
> > hurt.
> > 
> > I see elsewhere in the code an assert that the returned pointer is not
> > NULL (mxf_write_ffv1_desc()), and explicit checks for it
> 
> There is a similar check in your patch:
> 
> -    int component_count = av_pix_fmt_count_planes(st->codecpar->format);
> +    const AVPixFmtDescriptor *pix_desc = av_pix_fmt_desc_get(st->codecpar->format);
> +
> +    if (!pix_desc) {
> +        av_log(s, AV_LOG_ERROR, "Pixel format not set - not writing JPEG2000SubDescriptor\n");
> +        return;
> +    }
> 
> You should propagate back the error, not just silently ignore it. Or if 
> this cannot happen, make this an assert instead.

Not a bad idea. write_desc in MXFContainerEssenceEntry returns void,
changing it to return int shouldn't be hard. I'll do it in a separate
patch

/Tomas