[FFmpeg-devel] [PATCH] Make the scale filter only declare the supported formats

[Michael Niedermayer]

On Tue, Jan 05, 2010 at 01:44:18AM +0100, Stefano Sabatini wrote:
> On date Tuesday 2010-01-05 00:18:50 +0100, Michael Niedermayer encoded:
> > On Mon, Jan 04, 2010 at 01:54:24AM +0100, Stefano Sabatini wrote:
> > > Hi, as in subject.
> > > 
> > > Depends on the already posted patch:
> > > "Make lsws expose supported formats".
> > > 
> > > Regards.
> > > -- 
> > > FFmpeg = Freak & Frenzy Meaningful Problematic Exuberant Gadget
> > 
> > >  vf_scale.c |   36 +++++++++++++++++++++++++++++++-----
> > >  1 file changed, 31 insertions(+), 5 deletions(-)
> > > 0394116cc822d6289f45e4e8cef28d7aa9dc6d43  make-scale-filter-declare-managed-fmts.patch
> > > Index: libavfilter-soc/ffmpeg/libavfilter/vf_scale.c
> > > ===================================================================
> > > --- libavfilter-soc.orig/ffmpeg/libavfilter/vf_scale.c	2010-01-04 01:32:21.000000000 +0100
> > > +++ libavfilter-soc/ffmpeg/libavfilter/vf_scale.c	2010-01-04 01:50:42.000000000 +0100
> > > @@ -68,18 +68,44 @@
> > >  
> > >  static int query_formats(AVFilterContext *ctx)
> > >  {
> > > -    AVFilterFormats *formats;
> > > +    AVFilterFormats *in_formats  = NULL;
> > > +    AVFilterFormats *out_formats = NULL;
> > > +    enum PixelFormat pix_fmt;
> > > +    int ret;
> > >  
> > 
> > >      if (ctx->inputs[0]) {
> > 
> > why do we need this check?
> 
> I wonder the same.
> 
> query_format() is supposed to be called when all the links have been
> connected to the filter, in this case the check shouldn't be necessary
> otherwise the application behavior is supposed to be undefined (->
> crash), anyway this needs to be documented better.
> 
> > > -        formats = avfilter_all_colorspaces();
> > > -        avfilter_formats_ref(formats, &ctx->inputs[0]->out_formats);
> > 
> > 
> > > +        if (!(in_formats  = av_mallocz(sizeof(AVFilterFormats)))) {
> > > +            ret = AVERROR(ENOMEM);
> > > +            goto fail;
> > > +        }
> > > +
> > > +        for (pix_fmt = 0; pix_fmt < PIX_FMT_NB; pix_fmt++)
> > > +            if (sws_isSupportedInput(pix_fmt))
> > > +                if ((ret = avfilter_add_colorspace(in_formats, pix_fmt)) < 0)
> > > +                    goto fail;
> > 
> > this design is inconvenient, the first avfilter_add_colorspace() could
> > allocate in_formats. Well heres your ** back :)
> 
> Yes I had the very same idea and was already working on a patch for
> that :).
> 
> > > +        avfilter_formats_ref(in_formats, &ctx->inputs[0]->out_formats);
> > >      }
> > >      if (ctx->outputs[0]) {
> > > -        formats = avfilter_all_colorspaces();
> > > -        avfilter_formats_ref(formats, &ctx->outputs[0]->in_formats);
> > > +        if (!(out_formats  = av_mallocz(sizeof(AVFilterFormats)))) {
> > > +            ret = AVERROR(ENOMEM);
> > > +            goto fail;
> > > +        }
> > > +
> > > +        for (pix_fmt = 0; pix_fmt < PIX_FMT_NB; pix_fmt++)
> > > +            if (sws_isSupportedOutput(pix_fmt))
> > > +                if ((ret = avfilter_add_colorspace(out_formats, pix_fmt)) < 0)
> > > +                    goto fail;
> > > +        avfilter_formats_ref(out_formats, &ctx->outputs[0]->in_formats);
> > >      }
> > >  
> > >      return 0;
> > > +
> > > +fail:
> > 
> > > +    if (in_formats)
> > > +        avfilter_formats_unref(&in_formats);
> > > +    if (out_formats)
> > > +        avfilter_formats_unref(&out_formats);
> > 
> > do we need this freeing here?
> > it seems more logic to let the common calling code do the cleanup
> 
> Mhh no strong opinion but I find slightly more readable to have common
> cleanup code being the line count almost the same.
> 
> Check attached patches.
> 
> BTW, many functions in formats.c miss av_malloc checks, should we add
> them?
> 
> Regards.
> -- 
> FFmpeg = Free and Forgiving Magnificient Patchable Extensive Goblin

>  avfilter.h |    6 ++++--
>  formats.c  |   20 ++++++++++++--------
>  2 files changed, 16 insertions(+), 10 deletions(-)
> 152e59c50f74417eb9c02472d63c94e7d3f3d344  change-add-colorspace.patch

ok


[...]
>  vf_scale.c |   26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 3bf17f2fbe4a08f0024ef102c57b4ebce0425996  make-scale-filter-declare-managed-fmts.patch

ok, i still thoigh think the cleanup should be moved to common code

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

Observe your enemies, for they first find out your faults. -- Antisthenes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100105/9ac6b19c/attachment.pgp>