[FFmpeg-devel] [PATCH] swscale: add API to convert AVFrames directly
wm4
nfxjfg at googlemail.com
Tue Oct 1 19:00:11 CEST 2013
On Tue, 1 Oct 2013 13:34:28 +0200
Stefano Sabatini <stefasab at gmail.com> wrote:
> On date Tuesday 2013-10-01 01:10:59 +0200, wm4 encoded:
> [...]
> > New patch attached. (I didn't run any extensive tests though - because
> > it's a new API and nothing uses it.)
>
> What about using it in doc/examples or in ff*.c code?
OK, patch for scaling_video.c included. Ideally, I'd like to make
vf_scale.c use it, but that does weird things for supporting interlaced
scaling - not sure if it's even possible to use sws_scale_frame here.
> > From 0dfefdaf1ccb9687a112ab45bb3a0a2bfff3102a Mon Sep 17 00:00:00 2001
> > From: wm4 <nfxjfg at googlemail.com>
> > Date: Sun, 29 Sep 2013 19:53:09 +0200
> > Subject: [PATCH] swscale: add API to convert AVFrames directly
> >
> > 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.
> > ---
> > libswscale/swscale.h | 83 ++++++++++++++++
> > libswscale/swscale_internal.h | 10 ++
> > libswscale/utils.c | 213 +++++++++++++++++++++++++++++++++++++++++-
> > libswscale/version.h | 2 +-
> > 4 files changed, 306 insertions(+), 2 deletions(-)
>
> Note to commmitter: add missing APIchanges entry
Added something.
[...]
> > +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);
>
> nit: weird mixing between camelStyle and snake_style.
sws_init_context() does this already, and I followed the existing
conventions for consistency.
> > + * AVFrame *src = ...;
> > + * SwsContext *sws = sws_alloc_context();
> > + * AVFrame *dst = av_frame_alloc();
> > + * if (!dst) goto error;
> > + * dst->format = AV_PIX_FMT_YUV420P;
> > + * dst->width = src->width * 2;
> > + * dst->height = src->height * 2;
> > + * if (av_frame_copy_props(dst, src) < 0) goto error; // don't change anything else
> > + * if (av_frame_get_buffer(dst, 32) < 0) goto error; // allocate image
> > + * if (sws_scale_frame(sws, dst, src) < 0) goto error;
> > + * // dst now contains the scaled image data
>
> You should probably enclose this with @code ... @endcode.
OK. I forgot to address Clément's comments. He mentioned this too.
> > + * Warning: libswscale expects that dst is writable (see av_frame_is_writable()).
>
> duplicated??
Where?
> > + *
> > + * The SwsContext can be reused when converting the next frame. If the AVFrame
> > + * parameters are different, libswscale is automatically reinitialized.
> > + *
> > + * @return zero or positive value on success, a negative value on error
> > + */
> > +int sws_scale_frame(struct SwsContext *sws_context, struct AVFrame *dst,
>
> > + struct AVFrame *src);
>
> const?
Added.
> > +
> > +/**
> > * Free the swscaler context swsContext.
> > * If swsContext is NULL, then does nothing.
> > */
> > @@ -303,6 +385,7 @@ SwsFilter *sws_getDefaultFilter(float lumaGBlur, float chromaGBlur,
> > float lumaSharpen, float chromaSharpen,
> > float chromaHShift, float chromaVShift,
> > int verbose);
> > +SwsFilter *sws_cloneFilter(SwsFilter *filter);
>
> please docs
Well, there wasn't any documentation for any of the SwsFilter
related stuff... but added.
> > void sws_freeFilter(SwsFilter *filter);
> >
> > /**
> > diff --git a/libswscale/swscale_internal.h b/libswscale/swscale_internal.h
> > index 33fdfc2..23f0966 100644
> > --- a/libswscale/swscale_internal.h
> > +++ b/libswscale/swscale_internal.h
> > @@ -271,6 +271,16 @@ typedef struct SwsContext {
> > const AVClass *av_class;
> >
> > /**
> > + * All of these are used by sws_reinit_cached_context() only.
> > + */
> > + int force_reinit;
> > + struct SwsContext *previous_settings;
> > + SwsFilter *user_src_filter;
> > + SwsFilter *user_dst_filter;
>
> > + int user_srcRange;
> > + int user_dstRange;
>
> nit: self-inconsistent style
Well, I'm not sure what to do. Sometimes libswscale uses camel case
style, sometimes underscores. Changed user_src_filter to
user_srcFilter for now.
> > +
> > + /**
> > * Note that src, dst, srcStride, dstStride will be copied in the
> > * sws_scale() wrapper so they can be freely modified here.
> > */
> > diff --git a/libswscale/utils.c b/libswscale/utils.c
> > index a2e3ce1..9a1659b 100644
> > --- a/libswscale/utils.c
> > +++ b/libswscale/utils.c
> > @@ -46,6 +46,7 @@
> > #include "libavutil/mathematics.h"
> > #include "libavutil/opt.h"
> > #include "libavutil/pixdesc.h"
> > +#include "libavutil/frame.h"
> > #include "libavutil/ppc/cpu.h"
> > #include "libavutil/x86/asm.h"
> > #include "libavutil/x86/cpu.h"
> > @@ -72,6 +73,12 @@ const char *swscale_license(void)
> > return LICENSE_PREFIX FFMPEG_LICENSE + sizeof(LICENSE_PREFIX) - 1;
> > }
> >
> > +// Mark SwsContext in need for reinitialization.
> > +static void sws_flush_cache(struct SwsContext *sws_context)
> > +{
> > + sws_context->force_reinit = 1;
> > +}
>
> pointless wrapper??
OK, removed. The previous definition of this function was supposed to
make clear what the code achieves, but it has been changed since, and
now it's pretty self-explanatory.
> > +
> > #define RET 0xC3 // near return opcode for x86
> >
> > typedef struct FormatEntry {
> > @@ -964,6 +971,9 @@ int sws_setColorspaceDetails(struct SwsContext *c, const int inv_table[4],
> > {
> > const AVPixFmtDescriptor *desc_dst;
> > const AVPixFmtDescriptor *desc_src;
> > +
> > + sws_flush_cache(c);
> > +
> > memmove(c->srcColorspaceTable, inv_table, sizeof(int) * 4);
> > memmove(c->dstColorspaceTable, table, sizeof(int) * 4);
> >
> > @@ -1106,6 +1116,8 @@ av_cold int sws_init_context(SwsContext *c, SwsFilter *srcFilter,
> > const AVPixFmtDescriptor *desc_src;
> > const AVPixFmtDescriptor *desc_dst;
> >
> > + sws_flush_cache(c);
> > +
> > cpu_flags = av_get_cpu_flags();
> > flags = c->flags;
> > emms_c();
> > @@ -1964,7 +1976,30 @@ void sws_freeFilter(SwsFilter *filter)
> > av_free(filter);
> > }
> >
> > -void sws_freeContext(SwsContext *c)
> > +SwsFilter *sws_cloneFilter(SwsFilter *filter)
> > +{
> > + SwsFilter *new_filter;
> > + if (!filter)
> > + return NULL;
> > +
> > + new_filter = av_malloc(sizeof(SwsFilter));
> > + new_filter->lumH = sws_cloneVec(filter->lumH);
> > + new_filter->lumV = sws_cloneVec(filter->lumV);
> > + new_filter->chrH = sws_cloneVec(filter->chrH);
> > + new_filter->chrV = sws_cloneVec(filter->chrV);
> > +
>
> > + if (!new_filter->lumH || !new_filter->lumV ||
> > + !new_filter->chrH || !new_filter->chrV) {
>
> missing check on new_filter, will crash in the unlikely case where
> av_malloc() failed.
Fixed.
> > + sws_freeFilter(new_filter);
> > + return NULL;
> > + }
> > +
> > + return new_filter;
> > +}
> > +
> > +// Free and reset almost everything in the SwsContext, but do not wipe user
> > +// settings.
> > +static void sws_uninit_context(SwsContext *c)
> > {
> > int i;
> > if (!c)
> > @@ -2027,7 +2062,19 @@ void sws_freeContext(SwsContext *c)
> >
> > av_freep(&c->yuvTable);
> > av_freep(&c->formatConvBuffer);
> > +}
> >
> > +void sws_freeContext(SwsContext *c)
> > +{
> > + if (!c)
> > + return;
> > +
> > + sws_uninit_context(c);
> > + sws_freeFilter(c->user_src_filter);
> > + sws_freeFilter(c->user_dst_filter);
> > + c->user_src_filter = NULL;
> > + c->user_dst_filter = NULL;
> > + av_freep(&c->previous_settings);
> > av_free(c);
> > }
> >
> > @@ -2078,3 +2125,167 @@ struct SwsContext *sws_getCachedContext(struct SwsContext *context, int srcW,
> > }
> > return context;
> > }
> > +
>
> > +int sws_set_src_filter(struct SwsContext *sws_context, SwsFilter *srcFilter)
> > +{
> > + SwsFilter *new_filter = sws_cloneFilter(srcFilter);
> > + if (srcFilter && !new_filter)
> > + return AVERROR(ENOMEM);
> > +
> > + sws_freeFilter(sws_context->user_src_filter);
> > + sws_context->user_src_filter = new_filter;
> > +
> > + sws_flush_cache(sws_context);
> > + return 0;
> > +}
> > +
> > +int sws_set_dst_filter(struct SwsContext *sws_context, SwsFilter *dstFilter)
> > +{
> > + SwsFilter *new_filter = sws_cloneFilter(dstFilter);
> > + if (dstFilter && !new_filter)
> > + return AVERROR(ENOMEM);
> > +
> > + sws_freeFilter(sws_context->user_dst_filter);
> > + sws_context->user_dst_filter = new_filter;
> > +
> > + sws_flush_cache(sws_context);
> > + return 0;
> > +}
>
> not happy about code duplication, what about a macro or a common
> routine?
IMO a bit more awkward, but done.
> > +
>
> > +static int compare_csp_table(int table1[4], int table2[4])
>
> nit: csp -> colorspace for readability
OK.
> > +{
> > + int i;
> > + for (i = 0; i < 4; i++) {
> > + if (table1[i] != table2[i])
> > + return 0;
> > + }
> > + return 1;
> > +}
> > +
> > +// Return 1 if nothing changed between new and old, and 0 if it did.
> > +// This checks user settings only.
> > +static int can_reuse(struct SwsContext *new, struct SwsContext *old)
> > +{
> > + return new->flags == old->flags && // swscale_options
> > + new->srcW == old->srcW &&
> > + new->srcH == old->srcH &&
> > + new->dstW == old->dstW &&
> > + new->dstH == old->dstH &&
> > + new->srcFormat == old->srcFormat &&
> > + new->dstFormat == old->dstFormat &&
> > + new->srcRange == old->srcRange &&
> > + new->dstRange == old->dstRange &&
> > + new->param[0] == old->param[0] &&
> > + new->param[1] == old->param[1] &&
> > + new->src_v_chr_pos == old->src_v_chr_pos &&
> > + new->src_h_chr_pos == old->src_h_chr_pos &&
> > + new->dst_v_chr_pos == old->dst_v_chr_pos &&
> > + new->dst_h_chr_pos == old->dst_h_chr_pos &&
> > + new->dither == old->dither &&
> > + new->brightness == old->brightness && // sws_setColorspaceDetails
> > + new->contrast == old->contrast &&
> > + new->saturation == old->saturation &&
> > + compare_csp_table(new->srcColorspaceTable, old->srcColorspaceTable) &&
> > + compare_csp_table(new->dstColorspaceTable, old->dstColorspaceTable);
> > +}
> > +
> > +// Does a shallow copy of all fields that are user settings.
> > +static void copy_settings(struct SwsContext *dst, struct SwsContext *src)
> > +{
> > + dst->user_src_filter = src->user_src_filter;
> > + dst->user_dst_filter = src->user_dst_filter;
> > + dst->flags = src->flags;
> > + dst->srcW = src->srcW;
> > + dst->srcH = src->srcH;
> > + dst->dstW = src->dstW;
> > + dst->dstH = src->dstH;
> > + dst->srcFormat = src->srcFormat;
> > + dst->dstFormat = src->dstFormat;
> > + dst->srcRange = src->srcRange;
> > + dst->dstRange = src->dstRange;
> > + dst->param[0] = src->param[0];
> > + dst->param[1] = src->param[1];
> > + dst->src_v_chr_pos = src->src_v_chr_pos;
> > + dst->src_h_chr_pos = src->src_h_chr_pos;
> > + dst->dst_v_chr_pos = src->dst_v_chr_pos;
> > + dst->dst_h_chr_pos = src->dst_h_chr_pos;
> > + dst->dither = src->dither;
> > + dst->brightness = src->brightness;
> > + dst->contrast = src->contrast;
> > + dst->saturation = src->saturation;
> > + memcpy(dst->srcColorspaceTable, src->srcColorspaceTable, sizeof(int) * 4);
> > + memcpy(dst->dstColorspaceTable, src->dstColorspaceTable, sizeof(int) * 4);
> > +}
> > +
> > +int sws_reinit_cached_context(struct SwsContext *c)
> > +{
>
> > + int r = 0;
>
> nit+++: ret
OK.
> > + if (!c->previous_settings) {
> > + c->force_reinit = 1;
> > + if (!(c->previous_settings = av_malloc(sizeof(struct SwsContext))))
> > + return AVERROR(ENOMEM);
> > + }
> > + if (!c->contrast && !c->saturation) {
> > + c->contrast = 1 << 16;
> > + c->saturation = 1 << 16;
> > + }
> > + if (c->force_reinit || !can_reuse(c, c->previous_settings)) {
> > + struct SwsContext *previous_settings;
> > + previous_settings = c->previous_settings;
> > + *previous_settings = *c;
> > + sws_uninit_context(c);
> > + memset(c, 0, sizeof(*c));
> > + c->av_class = &sws_context_class;
> > + copy_settings(c, previous_settings);
> > + c->previous_settings = previous_settings;
> > + sws_setColorspaceDetails(c, c->srcColorspaceTable, c->srcRange,
> > + c->dstColorspaceTable, c->dstRange,
> > + c->brightness, c->contrast, c->saturation);
> > + r = sws_init_context(c, c->user_src_filter, c->user_dst_filter);
> > + if (r < 0)
> > + return r;
> > + c->force_reinit = 0;
> > + }
> > + return r;
> > +}
> > +
> > +static void sws_set_avframe_colorspace_table(int table[4],
> > + enum AVColorSpace colorspace)
> > +{
> > + int i;
>
> > + int swscsp = colorspace; // this is libavfilter/vf_scale.c's dirty secret
> > + const int *sws_table = sws_getCoefficients(swscsp);
>
> why the intermediary swscsp variable?
>
> Also I'd prefer to avoid the reference to scale.
The first line converts AVColorSpace to libswscale colorspace
constants, but they happen to use the same values - this is
undocumented, but vf_scale.c relies on it, so...
Anyway, replaced with a comment that doesn't mention vf_scale.c.
> > + for (i = 0; i < 4; i++)
> > + table[i] = sws_table[i];
> > +}
> > +
>
> > +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 == AVCOL_RANGE_JPEG;
> > + 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 == AVCOL_RANGE_JPEG;
> > + sws_set_avframe_colorspace_table(c->dstColorspaceTable, dst->colorspace);
> > +}
>
> Could be probably macroized/factored, and merged with sws_set_avframe_colorspace_table().
I'd rather not, because the result would be quite awkward. If the image
parameters weren't a loose bunch of variables, this would be easily
possible: e.g. libswscale would use AVFrame directly, or there would be
a type that summarizes image format information. But it doesn't, so
you would need a macro that appends "src" or "dst" to a variable name,
and any attempt to factor out this code would be awkward. I'd rather
duplicate 5 lines of code just one time.
> [...]
New patch attached.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-swscale-add-API-to-convert-AVFrames-directly.patch
Type: text/x-patch
Size: 15936 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131001/dc26f012/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-examples-scaling_video-work-with-AVFrame-use-sws_sca.patch
Type: text/x-patch
Size: 5074 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20131001/dc26f012/attachment-0001.bin>
More information about the ffmpeg-devel
mailing list