Re: [FFmpeg-devel] [PATCHv3] avfilter/vf_chromakey: Add chromakey video filter

[Paul B Mahol]

On 9/21/15, Timo Rothenpieler  wrote:
>> Benefit would be to save 2 branching and 2 div (the div can already be
>> replaced by a shift here though - which would have a benefit since x & y
>> are signed) and keep the code generic.
>
> Changed it to a shift and moved the if() out of the loop.
> No observable performance benefit though.
>
> +frame->data[3][frame->linesize[3] * y + x] =
> do_chromakey_pixel(ctx, u, v);

 You might want to check if saving a bunch of dereferencing in the inner
 loop helps performance.
>>>
>>> You mean getting frame->data[3] and frame->linesize[3] before the loop?
>>
>> yes
>>
>>> Shouldn't this be something the compiler optimises for me?
>>
>> it should, but i've observe performance enhancement in similar
>> situations.
>> You might want to try.
>
> Did some testing. My most aggressively hand-optimized version was
> significantly slower than just letting gcc optimize the original code.
> Looking at the assembly, it even seems to automatically optimize out the
> multiplications by y, but the assembly is quite complex and I'm not sure
> if I'm reading it right.
> None of the lighter changes made any difference in speed.
>
> I guess the greatest possible speedups could be made by converting the
> actual chromakey algorithm to integer math, which is something i plan on
> doing, but i'd like to get a working version in first.
>
>

still lgtm
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCHv3] avfilter/vf_chromakey: Add chromakey video filter

[Timo Rothenpieler]

still lgtm


If nobody objects, i'll push later today.


Timo




signature.asc
Description: OpenPGP digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCHv3] avfilter/vf_chromakey: Add chromakey video filter

[Timo Rothenpieler]

>> +if (frame->format == AV_PIX_FMT_YUVA420P || frame->format == 
>> AV_PIX_FMT_YUVA422P)
>> +x /= 2;
>> +
>> +if (frame->format == AV_PIX_FMT_YUVA420P)
>> +y /= 2;
> 
> Why not use the usual subsampling mechanism? (using vsub/hsub etc)

That seemed more complex to me, as this filter just supports those 3
pixel formats anyway.
It would involve getting the pixel format description in some init
function, calculating and storing the h/vsub values. Is there any
benefit to that? Otherwise I think I'd prefer this solution.

>> +frame->data[3][frame->linesize[3] * y + x] = 
>> do_chromakey_pixel(ctx, u, v);
> 
> You might want to check if saving a bunch of dereferencing in the inner
> loop helps performance.

You mean getting frame->data[3] and frame->linesize[3] before the loop?
Shouldn't this be something the compiler optimises for me?
Anyway, can easily be changed.

>> +if (res = av_frame_make_writable(frame))
>> +return res;
> 
> You have a .needs_writable attribute somewhere for that purpose

Didn't know about that attribute.

>> +#define FIXNUM(x) ((int) ((x) * (1 << 10) + 0.5))
> 
> lrint()?

These defines were taken from the old colorspace.h, where they got
removed from in 7944b0ce94.
No idea if lrint optimizes in the same way, but as this is used only
once during startup, it shouldn't matter here.



signature.asc
Description: OpenPGP digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCHv3] avfilter/vf_chromakey: Add chromakey video filter

[Clément Bœsch]

On Mon, Sep 21, 2015 at 03:51:37PM +0200, Timo Rothenpieler wrote:
> >> +if (frame->format == AV_PIX_FMT_YUVA420P || frame->format == 
> >> AV_PIX_FMT_YUVA422P)
> >> +x /= 2;
> >> +
> >> +if (frame->format == AV_PIX_FMT_YUVA420P)
> >> +y /= 2;
> > 
> > Why not use the usual subsampling mechanism? (using vsub/hsub etc)
> 
> That seemed more complex to me, as this filter just supports those 3
> pixel formats anyway.
> It would involve getting the pixel format description in some init
> function, calculating and storing the h/vsub values. Is there any
> benefit to that? Otherwise I think I'd prefer this solution.
> 

Benefit would be to save 2 branching and 2 div (the div can already be
replaced by a shift here though - which would have a benefit since x & y
are signed) and keep the code generic.

> >> +frame->data[3][frame->linesize[3] * y + x] = 
> >> do_chromakey_pixel(ctx, u, v);
> > 
> > You might want to check if saving a bunch of dereferencing in the inner
> > loop helps performance.
> 
> You mean getting frame->data[3] and frame->linesize[3] before the loop?

yes

> Shouldn't this be something the compiler optimises for me?

it should, but i've observe performance enhancement in similar situations.
You might want to try.

> Anyway, can easily be changed.
> 
> >> +if (res = av_frame_make_writable(frame))
> >> +return res;
> > 
> > You have a .needs_writable attribute somewhere for that purpose
> 
> Didn't know about that attribute.
> 
> >> +#define FIXNUM(x) ((int) ((x) * (1 << 10) + 0.5))
> > 
> > lrint()?
> 
> These defines were taken from the old colorspace.h, where they got
> removed from in 7944b0ce94.
> No idea if lrint optimizes in the same way, but as this is used only
> once during startup, it shouldn't matter here.
> 

OK

-- 
Clément B.


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCHv3] avfilter/vf_chromakey: Add chromakey video filter

[Clément Bœsch]

On Fri, Sep 18, 2015 at 04:27:54PM +0200, Timo Rothenpieler wrote:
> ---
>  Changelog  |   1 +
>  MAINTAINERS|   1 +
>  doc/filters.texi   |  45 +++
>  libavfilter/Makefile   |   1 +
>  libavfilter/allfilters.c   |   1 +
>  libavfilter/version.h  |   2 +-
>  libavfilter/vf_chromakey.c | 198 
> +
>  7 files changed, 248 insertions(+), 1 deletion(-)
>  create mode 100644 libavfilter/vf_chromakey.c
> 
[...]
> +static void get_pixel_uv(AVFrame *frame, int x, int y, uint8_t *u, uint8_t 
> *v)
> +{
> +if (x < 0 || x >= frame->width || y < 0 || y >= frame->height) {
> +return;
> +}
> +

> +if (frame->format == AV_PIX_FMT_YUVA420P || frame->format == 
> AV_PIX_FMT_YUVA422P)
> +x /= 2;
> +
> +if (frame->format == AV_PIX_FMT_YUVA420P)
> +y /= 2;

Why not use the usual subsampling mechanism? (using vsub/hsub etc)

> +
> +*u = frame->data[1][frame->linesize[1] * y + x];
> +*v = frame->data[2][frame->linesize[2] * y + x];
> +}
> +
> +static int do_chromakey_slice(AVFilterContext *avctx, void *arg, int jobnr, 
> int nb_jobs)
> +{
> +AVFrame *frame = arg;
> +
> +const int slice_start = (frame->height * jobnr) / nb_jobs;
> +const int slice_end = (frame->height * (jobnr + 1)) / nb_jobs;
> +
> +ChromakeyContext *ctx = avctx->priv;
> +
> +int x, y, xo, yo;
> +uint8_t u[9], v[9];
> +
> +memset(u, ctx->chromakey_uv[0], sizeof(u));
> +memset(v, ctx->chromakey_uv[1], sizeof(v));
> +

> +for (y = slice_start; y < slice_end; ++y) {
> +for (x = 0; x < frame->width; ++x) {
> +for (yo = 0; yo < 3; ++yo) {
> +for (xo = 0; xo < 3; ++xo) {

nit: pre increment is not the prefered style (this is not c++, there is no
need for a more confusing syntax)

> +get_pixel_uv(frame, x + xo - 1, y + yo - 1, [yo * 3 + 
> xo], [yo * 3 + xo]);
> +}
> +}
> +

> +frame->data[3][frame->linesize[3] * y + x] = 
> do_chromakey_pixel(ctx, u, v);

You might want to check if saving a bunch of dereferencing in the inner
loop helps performance.

> +}
> +}
> +
> +return 0;
> +}
> +
> +static int filter_frame(AVFilterLink *link, AVFrame *frame)
> +{
> +AVFilterContext *avctx = link->dst;
> +int res;
> +

> +if (res = av_frame_make_writable(frame))
> +return res;

You have a .needs_writable attribute somewhere for that purpose

> +
> +if (res = avctx->internal->execute(avctx, do_chromakey_slice, frame, 
> NULL, FFMIN(frame->height, avctx->graph->nb_threads)))
> +return res;
> +
> +return ff_filter_frame(avctx->outputs[0], frame);
> +}
> +

> +#define FIXNUM(x) ((int) ((x) * (1 << 10) + 0.5))

lrint()?

> +#define RGB_TO_U(rgb) (((- FIXNUM(0.16874) * rgb[0] - FIXNUM(0.33126) * 
> rgb[1] + FIXNUM(0.5) * rgb[2] + (1 << 9) - 1) >> 10) + 128)
> +#define RGB_TO_V(rgb) (((  FIXNUM(0.5) * rgb[0] - FIXNUM(0.41869) * 
> rgb[1] - FIXNUM(0.08131) * rgb[2] + (1 << 9) - 1) >> 10) + 128)
> +
[...]

-- 
Clément B.


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCHv3] avfilter/vf_chromakey: Add chromakey video filter

[Lou Logan]

Hi,

I know the docs is similar to colorkey, but I have a few short
suggestions.

On Fri, 18 Sep 2015 16:27:54 +0200, Timo Rothenpieler wrote:

[...]
> diff --git a/doc/filters.texi b/doc/filters.texi
> index 4a55e59..0446204 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -3556,6 +3556,51 @@ 
> boxblur=luma_radius=min(h\,w)/10:luma_power=1:chroma_radius=min(cw\,ch)/10:chrom
>  @end example
>  @end itemize
>  
> +@section chromakey
> +YUV colorspace color/chroma keying.
> +
> +The filter accepts the following options:
> +
> +@table @option
> +@item color
> +The color which will be replaced with transparency.

For the general syntax of this option, check the "Color" section in the
ffmpeg-utils manual. Default is @code{black}.

> +@item similarity
> +Similarity percentage with the key color.
> +
> +0.01 matches only the exact key color, while 1.0 matches everything.

Default is 0.01.

> +@item blend
> +Blend percentage.
> +
> +0.0 makes pixels either fully transparent, or not transparent at all.

Range is 0.0 to 1.0. Default is 0.0.

> +Higher values result in semi-transparent pixels, with a higher transparency
> +the more similar the pixels color is to the key color.
> +
> +@item yuv
> +Signals that the color passed is already in YUV instead of RGB.

Default is disabled.

> +
> +Litteral colors like "green" or "red" don't make sense with this enabled 
> anymore.

s/Litteral/Literal

The "anymore" can be removed.

What happens if a literal color is used when the yuv option is enabled?

[...]

> +static const AVOption chromakey_options[] = {
> +{ "color", "set the chromakey key color", OFFSET(chromakey_rgba), 
> AV_OPT_TYPE_COLOR, { .str = "black" }, CHAR_MIN, CHAR_MAX, FLAGS },
> +{ "similarity", "set the chromakey similarity value", 
> OFFSET(similarity), AV_OPT_TYPE_FLOAT, { .dbl = 0.01 }, 0.01, 1.0, FLAGS },
> +{ "blend", "set the chromakey key blend value", OFFSET(blend), 
> AV_OPT_TYPE_FLOAT, { .dbl = 0.0 }, 0.0, 1.0, FLAGS },
> +{ "yuv", "color parameter is in yuv instead of rgb", OFFSET(is_yuv), 
> AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, FLAGS },

"indicate that the color parameter is in yuv instead of rgb"
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCHv3] avfilter/vf_chromakey: Add chromakey video filter

[Paul B Mahol]

On 9/18/15, Timo Rothenpieler  wrote:
> ---
>  Changelog  |   1 +
>  MAINTAINERS|   1 +
>  doc/filters.texi   |  45 +++
>  libavfilter/Makefile   |   1 +
>  libavfilter/allfilters.c   |   1 +
>  libavfilter/version.h  |   2 +-
>  libavfilter/vf_chromakey.c | 198
> +
>  7 files changed, 248 insertions(+), 1 deletion(-)
>  create mode 100644 libavfilter/vf_chromakey.c
>


lgtm
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel