[FFmpeg-devel] libavfilter: extending overlay filter
Mark Himsley
mark
Sun Mar 13 20:28:53 CET 2011
On 13/03/11 18:38, Stefano Sabatini wrote:
> On date Sunday 2011-03-13 15:18:04 +0000, Mark Himsley encoded:
>> On 13/03/11 14:26, Stefano Sabatini wrote:
>>> On date Sunday 2011-03-13 14:18:42 +0000, Mark Himsley encoded:
> [...]
>> --- ffmpeg/libavfilter/vf_overlay.c 2011-02-19 15:20:16.000000000 +0000
>> +++ FFmbc-0.6-rc3/libavfilter/vf_overlay.c 2011-03-13 15:11:23.000000000 +0000
>
> Please provide a patch created with git format-patch.
Thanks Stefano for looking at the patch.
>> @@ -60,6 +60,19 @@
>> typedef struct {
>> int x, y; ///< position of overlayed picture
>>
>> + int inout_is_rgb;
>> + int inout_is_alpha;
>> + int inout_r_location;
>> + int inout_g_location;
>> + int inout_b_location;
>> + int inout_a_location;
>> +
>> + int blend_is_rgb;
>> + int blend_r_location;
>> + int blend_g_location;
>> + int blend_b_location;
>> + int blend_a_location;
>
> This is best addressed by using an rgb_map.
Yep.
>> +
>> AVFilterBufferRef *overpicref;
>
>
>>
>> int max_plane_step[4]; ///< steps per pixel for each plane
>> @@ -91,8 +104,25 @@
>>
>> static int query_formats(AVFilterContext *ctx)
>> {
>> - const enum PixelFormat inout_pix_fmts[] = { PIX_FMT_YUV420P, PIX_FMT_NONE };
>> - const enum PixelFormat blend_pix_fmts[] = { PIX_FMT_YUVA420P, PIX_FMT_NONE };
>> + const enum PixelFormat inout_pix_fmts[] = {
>> + PIX_FMT_YUV420P,
>> + PIX_FMT_BGR24,
>> + PIX_FMT_RGB24,
>> + PIX_FMT_ARGB,
>> + PIX_FMT_RGBA,
>> + PIX_FMT_ABGR,
>> + PIX_FMT_BGRA,
>> + PIX_FMT_NONE
>> + };
>> + const enum PixelFormat blend_pix_fmts[] = {
>> + PIX_FMT_YUVA420P,
>> + PIX_FMT_ARGB,
>> + PIX_FMT_RGBA,
>> + PIX_FMT_ABGR,
>> + PIX_FMT_BGRA,
>> + PIX_FMT_NONE
>> + };
>> +
>> AVFilterFormats *inout_formats = avfilter_make_format_list(inout_pix_fmts);
>> AVFilterFormats *blend_formats = avfilter_make_format_list(blend_pix_fmts);
>
> Ideally we should be able to define the overlay pad accepted formats
> when the main pad formats have been already negotiated. Unfortunately
> I don't think this is currently possible.
This *is* my issue. I need to define the accepted overlay format after
the main format has been negotiated.
> Alternatively we could setup a parameter during configuration to set
> the colorspace class to use (rgb/yuv).
Interesting point, more on this later.
>> @@ -105,13 +135,68 @@
>>
>> static int config_input_main(AVFilterLink *inlink)
>> {
>> - OverlayContext *over = inlink->dst->priv;
>> + AVFilterContext *ctx = inlink->dst;
>> + OverlayContext *over = inlink->dst->priv;
>> const AVPixFmtDescriptor *pix_desc =&av_pix_fmt_descriptors[inlink->format];
>>
>> av_image_fill_max_pixsteps(over->max_plane_step, NULL, pix_desc);
>> over->hsub = pix_desc->log2_chroma_w;
>> over->vsub = pix_desc->log2_chroma_h;
>>
>> + over->inout_is_rgb = 0;
>> + switch (inlink->format)
>> + {
>> + case PIX_FMT_RGB24:
>> + case PIX_FMT_BGR24:
>> + case PIX_FMT_RGBA:
>> + case PIX_FMT_BGRA:
>> + case PIX_FMT_ARGB:
>> + case PIX_FMT_ABGR:
>> + over->inout_is_rgb = 1;
>> +
>> + over->inout_is_alpha = 0;
>> + switch (inlink->format)
>> + {
>> + case PIX_FMT_RGBA:
>> + case PIX_FMT_BGRA:
>> + case PIX_FMT_ARGB:
>> + case PIX_FMT_ABGR:
>> + over->inout_is_alpha = 1;
>> + av_log(ctx, AV_LOG_INFO,
>> + "main has alpha\n");
>> + break;
>> + }
>> +
>> + over->inout_r_location = 0;
>> + over->inout_g_location = 1;
>> + over->inout_b_location = 2;
>> + over->inout_a_location = 3;
>> + switch (inlink->format)
>> + {
>> + case PIX_FMT_BGR24:
>> + case PIX_FMT_BGRA:
>> + over->inout_r_location = 2;
>> + over->inout_b_location = 0;
>> + break;
>> + case PIX_FMT_ARGB:
>> + over->inout_r_location = 1;
>> + over->inout_g_location = 2;
>> + over->inout_b_location = 3;
>> + over->inout_a_location = 0;
>> + break;
>> + case PIX_FMT_ABGR:
>> + over->inout_r_location = 3;
>> + over->inout_g_location = 2;
>> + over->inout_b_location = 1;
>> + over->inout_a_location = 0;
>> + }
>> + av_log(ctx, AV_LOG_INFO,
>> + "main pixel locations R:%d G:%d B:%d A:%d\n",
>> + over->inout_r_location, over->inout_g_location,
>> + over->inout_b_location, over->inout_a_location);
>> + break;
>> + }
>> +
>>
>> return 0;
>> }
>>
>> @@ -147,7 +232,45 @@
>> NULL, NULL, NULL, NULL, NULL, 0, ctx))< 0)
>> goto fail;
>> over->x = res;
>> -
>> +
>> + over->blend_is_rgb = 0;
>> + switch (inlink->format)
>> + {
>> + case PIX_FMT_RGBA:
>> + case PIX_FMT_BGRA:
>> + case PIX_FMT_ARGB:
>> + case PIX_FMT_ABGR:
>> + over->blend_is_rgb = 1;
>> +
>> + over->blend_r_location = 0;
>> + over->blend_g_location = 1;
>> + over->blend_b_location = 2;
>> + over->blend_a_location = 3;
>> + switch (inlink->format)
>> + {
>> + case PIX_FMT_BGRA:
>> + over->blend_r_location = 2;
>> + over->blend_b_location = 0;
>> + break;
>> + case PIX_FMT_ARGB:
>> + over->blend_r_location = 1;
>> + over->blend_g_location = 2;
>> + over->blend_b_location = 3;
>> + over->blend_a_location = 0;
>> + break;
>> + case PIX_FMT_ABGR:
>> + over->blend_r_location = 3;
>> + over->blend_g_location = 2;
>> + over->blend_b_location = 1;
>> + over->blend_a_location = 0;
>> + }
>> + av_log(ctx, AV_LOG_INFO,
>> + "overlay pixel locations R:%d G:%d B:%d A:%d\n",
>> + over->blend_r_location, over->blend_g_location,
>> + over->blend_b_location, over->blend_a_location);
>> + break;
>> + }
>
> Already noted, this can be greatly simplified.
Agreed.
>> +
>> av_log(ctx, AV_LOG_INFO,
>> "main w:%d h:%d fmt:%s overlay x:%d y:%d w:%d h:%d fmt:%s\n",
>> ctx->inputs[MAIN]->w, ctx->inputs[MAIN]->h,
>> @@ -167,6 +290,12 @@
>> (int)var_values[VAR_MAIN_W], (int)var_values[VAR_MAIN_H]);
>> return AVERROR(EINVAL);
>> }
>> +
>> + if (over->inout_is_rgb != over->blend_is_rgb) {
>> + av_log(ctx, AV_LOG_ERROR,
>> + "Main and overlay are not similar formats, cannot mix YUV and RGB\n");
>> + return AVERROR(EINVAL);
>> + }
>> return 0;
>
> Possibly I would like to avoid this kind of configuration failures, or
> we're going to have lots of reports from users failing to interpret
> the error message.
Exactly. Which is why I said "I don't feel that I can submit [this
patch] while it breaks users current filter chains".
> A better approach is defining a configuration
> parameter for setting the colorspace class,
By this I assume you mean a filter configuration parameter.
I suppose we could set the default PixelFormats back to only allow YUV
and add an 'allow_RGB' switch to overlay.
> a yet better solution
> would be to extend the framework to manage efficiently such
> situations.
True.
>> fail:
>> @@ -187,7 +316,7 @@
>> av_gcd((int64_t)tb1.num * tb2.den,
>> (int64_t)tb2.num * tb1.den),
>> (int64_t)tb1.den * tb2.den, INT_MAX);
>> - av_log(ctx, AV_LOG_INFO,
>> + av_log(ctx, AV_LOG_DEBUG,
>
> spurios hunk
True. Oops.
>> "main_tb:%d/%d overlay_tb:%d/%d -> tb:%d/%d exact:%d\n",
>> tb1.num, tb1.den, tb2.num, tb2.den, tb->num, tb->den, exact);
>> if (!exact)
>> @@ -250,26 +379,77 @@
>> int overlay_end_y = y+h;
>> int slice_end_y = slice_y+slice_h;
>> int end_y, start_y;
>> -
>> + uint8_t m;
>
> Uhm, please meaningful name.
Good point, well made.
Thanks again.
--
Mark
More information about the ffmpeg-devel
mailing list