[FFmpeg-devel] [PATCH] swscale: add API to convert AVFrames directly

wm4 nfxjfg at googlemail.com
Sun Sep 29 20:21:35 CEST 2013


On Sun, 29 Sep 2013 17:50:52 +0200
Michael Niedermayer <michaelni at gmx.at> wrote:

> On Sun, Sep 29, 2013 at 04:53:02PM +0200, wm4 wrote:
> > See sws_scale_frame() and the example in its doxygen comment.
> > 
> > This makes the libswscale API considerably easier to use. Various
> > settings are automatically initialized from AVFrame settings, such as
> > format, size, color space and color range parameters.
> > 
> > To make the API easier to use, also don't require setting the sws_flags
> > AVOptions, and default to SWS_BILINEAR scaling.
> > ---
> >  libswscale/options.c          |   2 +-
> >  libswscale/swscale.h          |  78 +++++++++++++++++++
> >  libswscale/swscale_internal.h |   7 ++
> >  libswscale/utils.c            | 174 +++++++++++++++++++++++++++++++++++++++++-
> >  libswscale/version.h          |   2 +-
> >  5 files changed, 260 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libswscale/options.c b/libswscale/options.c
> > index 8985e6b..0f7955a 100644
> > --- a/libswscale/options.c
> > +++ b/libswscale/options.c
> 
> > @@ -34,7 +34,7 @@ static const char *sws_context_to_name(void *ptr)
> >  #define VE AV_OPT_FLAG_VIDEO_PARAM | AV_OPT_FLAG_ENCODING_PARAM
> >  
> >  static const AVOption swscale_options[] = {
> > -    { "sws_flags",       "scaler flags",                  OFFSET(flags),     AV_OPT_TYPE_FLAGS,  { .i64 = DEFAULT            }, 0,       UINT_MAX,       VE, "sws_flags" },
> > +    { "sws_flags",       "scaler flags",                  OFFSET(flags),     AV_OPT_TYPE_FLAGS,  { .i64 = SWS_BILINEAR            }, 0,       UINT_MAX,       VE, "sws_flags" },
> >      { "fast_bilinear",   "fast bilinear",                 0,                 AV_OPT_TYPE_CONST,  { .i64  = SWS_FAST_BILINEAR  }, INT_MIN, INT_MAX,        VE, "sws_flags" },
> >      { "bilinear",        "bilinear",                      0,                 AV_OPT_TYPE_CONST,  { .i64  = SWS_BILINEAR       }, INT_MIN, INT_MAX,        VE, "sws_flags" },
> >      { "bicubic",         "bicubic",                       0,                 AV_OPT_TYPE_CONST,  { .i64  = SWS_BICUBIC        }, INT_MIN, INT_MAX,        VE, "sws_flags" },
> 
> should be a seperate patch and should maintain vertical alignment

OK.

> also the patch seems to fail fate:
> 
> Test bmp-4bit failed. Look at tests/data/fate/bmp-4bit.err for details.
> make: *** [fate-bmp-4bit] Error 136
> Test cdxl-pal8 failed. Look at tests/data/fate/cdxl-pal8.err for details.
> Test bmp-rle4 failed. Look at tests/data/fate/bmp-rle4.err for details.
> Test bmp-32bit failed. Look at tests/data/fate/bmp-32bit.err for details.
> Test bmp-1bit failed. Look at tests/data/fate/bmp-1bit.err for details.
> make: *** [fate-cdxl-pal8] Error 136
> make: *** [fate-bmp-rle4] Error 136
> make: *** [fate-bmp-32bit] Error 136
> make: *** [fate-bmp-1bit] Error 136
> Test bmp-32bit-mask failed. Look at tests/data/fate/bmp-32bit-mask.err for details.
> Test bmp-8bit-os2 failed. Look at tests/data/fate/bmp-8bit-os2.err for details.
> make: *** [fate-bmp-32bit-mask] Error 136
> make: *** [fate-bmp-8bit-os2] Error 136

This happened because I initialized contrast and saturation to 1.0
(1 << 16), because something will crash if that isn't done.

The regression was then caused by this chunk in sws_init_context():

    if (!c->contrast && !c->saturation && !c->dstFormatBpp)
        sws_setColorspaceDetails(c, ff_yuv2rgb_coeffs[SWS_CS_DEFAULT], c->srcRange,
                                 ff_yuv2rgb_coeffs[SWS_CS_DEFAULT],
                                 c->dstRange, 0, 1 << 16, 1 << 16);

This probably ensures that sws_setColorspaceDetails is called at all
if the user doesn't.

I fixed it by applying the default values in sws_reinit_cached_context,
so the semantics of sws_init_context don't change.

> 
> 
> > diff --git a/libswscale/swscale.h b/libswscale/swscale.h
> > index 42702b7..60733f0 100644
> > --- a/libswscale/swscale.h
> > +++ b/libswscale/swscale.h
> > @@ -39,6 +39,8 @@
> >  #include "libavutil/pixfmt.h"
> >  #include "version.h"
> >  
> > +struct AVFrame;
> > +
> >  /**
> >   * Return the LIBSWSCALE_VERSION_INT constant.
> >   */
> > @@ -169,6 +171,81 @@ struct SwsContext *sws_alloc_context(void);
> >  int sws_init_context(struct SwsContext *sws_context, SwsFilter *srcFilter, SwsFilter *dstFilter);
> >  
> >  /**
> > + * Set source filter. This works only with sws_reinit_cached_context() and
> > + * sws_scale_frame().
> > + *
> > + * @return zero or positive value on success, a negative value on
> > + * error
> > + */
> > +int sws_set_src_filter(struct SwsContext *sws_context, SwsFilter *srcFilter);
> > +
> > +/**
> > + * Set destination filter. This works only with sws_reinit_cached_context() and
> > + * sws_scale_frame().
> > + *
> > + * @return zero or positive value on success, a negative value on
> > + * error
> > + */
> > +int sws_set_dst_filter(struct SwsContext *sws_context, SwsFilter *dstFilter);
> 
> would it make sense to allow seting the filter via AVOption as a
> char* ?
> iam wondering because this maybe would avoid these 2 functions

SwsFilter is a struct with 4 SwsVector members, so I'm not sure how
that would work. Also, I need to know if the filter changes. These
functions basically set a flag that they changed, and if they use the
AVOption API, I'd have to compare the SwsFilters with the old state to
detect a change.

> 
> [...]
> > +static void sws_set_src_frame_options(struct SwsContext *c, struct AVFrame *src)
> > +{
> > +    c->srcFormat = src->format;
> > +    c->srcW      = src->width;
> > +    c->srcH      = src->height;
> > +    c->srcRange  = src->color_range;
> > +    sws_set_avframe_colorspace_table(c->srcColorspaceTable, src->colorspace);
> > +}
> > +
> > +static void sws_set_dst_frame_options(struct SwsContext *c, struct AVFrame *dst)
> > +{
> > +    c->dstFormat = dst->format;
> > +    c->dstW      = dst->width;
> > +    c->dstH      = dst->height;
> > +    c->dstRange  = dst->color_range;
> > +    sws_set_avframe_colorspace_table(c->dstColorspaceTable, dst->colorspace);
> > +}
> 
> static functions dont need sws_ prefixes, if you want to shorten them

I removed it from some functions. I left it for others, because maybe
they should actually be public.

By the way, I'm not all that sure whether my patch is correct.
libswscale is very scary, and there could be some things I missed. In
particular, there's the question whether sws_uninit_context followed
sws_init_context really does full reinitialization, or if there are
additional members that must be reset in sws_uninit_context. (Members
which are set only by some init code paths, but are read by everything
would belong into this category.)

Another oversight of my patch is that brightness/contrast/saturation
can't be changed without using sws_setColorspaceDetails, even though my
patch tries to work towards eliminating the need for this function.
These 3 parameters should perhaps be AVOptions, but then they wouldn't
work with vanilla sws_init_context, only with sws_reinit_cached_context.

The question is: do you (all devs/users) think this patch makes for for
a good API addition?

> 
> > +
> > +int sws_scale_frame(struct SwsContext *sws_context, struct AVFrame *dst,
> > +                    struct AVFrame *src)
> > +{
> > +    int r;
> > +    sws_set_src_frame_options(sws_context, src);
> > +    sws_set_dst_frame_options(sws_context, dst);
> > +    if ((r = sws_reinit_cached_context(sws_context)) < 0)
> > +        return r;
> > +    r = sws_scale(sws_context, (const uint8_t *const *)src->data, src->linesize,
> > +                  0, src->height, dst->data, dst->linesize);
> > +    return r;
> > +}
> > diff --git a/libswscale/version.h b/libswscale/version.h
> > index 06f119a..72c6074 100644
> > --- a/libswscale/version.h
> > +++ b/libswscale/version.h
> > @@ -27,7 +27,7 @@
> >  #include "libavutil/avutil.h"
> >  
> >  #define LIBSWSCALE_VERSION_MAJOR 2
> > -#define LIBSWSCALE_VERSION_MINOR 5
> > +#define LIBSWSCALE_VERSION_MINOR 6
> >  #define LIBSWSCALE_VERSION_MICRO 100
> >  
> >  #define LIBSWSCALE_VERSION_INT  AV_VERSION_INT(LIBSWSCALE_VERSION_MAJOR, \
> > -- 
> > 1.8.4.rc3
> > 
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel at ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-swscale-make-bilinear-scaling-the-default.patch
Type: text/x-patch
Size: 1644 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130929/c68b5ffe/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-swscale-add-API-to-convert-AVFrames-directly.patch
Type: text/x-patch
Size: 14308 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130929/c68b5ffe/attachment-0001.bin>


More information about the ffmpeg-devel mailing list