[FFmpeg-devel] libavfilter: extending overlay filter

[Mark Himsley]

On 14/03/11 11:55, Stefano Sabatini wrote:
> On date Sunday 2011-03-13 20:54:09 +0000, Mark Himsley encoded:
>> On 13/03/11 19:28, Mark Himsley wrote:
>>> 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:
>>>> [...]
[...]
>> +enum { RED = 0, GREEN, BLUE, ALPHA };
>> +
>>   typedef struct {
>>       int x, y;                   ///<  position of overlayed picture
>>
>> +    int allow_packed_rgba;
>> +    uint8_t inout_is_packed_rgba;
>> +    uint8_t inout_is_alpha;
>> +    uint8_t inout_rgba_map[4];
>> +    uint8_t blend_is_packed_rgba;
>> +    uint8_t blend_rgba_map[4];
>
> just a minor remark, I wonder if "overlay" is more clear than "blend".

I agree. But in other parts of the current code the word blend is used, 
such as blend_pix_fmts[].

Using either "overlay" everywhere or "blend" everywhere would be a good 
change. Using both is confusing.

My preference would be to use "main" and "overlay" throughout.


>> +    over->inout_is_packed_rgba = 1;
>> +    over->inout_is_alpha = 0;
>> +    switch (inlink->format) {
>> +    case PIX_FMT_ARGB:  over->inout_is_alpha = 1; over->inout_rgba_map[ALPHA] = 0; over->inout_rgba_map[RED  ] = 1; over->inout_rgba_map[GREEN] = 2; over->inout_rgba_map[BLUE ] = 3; break;
>> +    case PIX_FMT_ABGR:  over->inout_is_alpha = 1; over->inout_rgba_map[ALPHA] = 0; over->inout_rgba_map[BLUE ] = 1; over->inout_rgba_map[GREEN] = 2; over->inout_rgba_map[RED  ] = 3; break;
>> +    case PIX_FMT_RGBA:  over->inout_is_alpha = 1;
>> +    case PIX_FMT_RGB24: over->inout_rgba_map[RED  ] = 0; over->inout_rgba_map[GREEN] = 1; over->inout_rgba_map[BLUE ] = 2; over->inout_rgba_map[ALPHA] = 3; break;
>> +    case PIX_FMT_BGRA:  over->inout_is_alpha = 1;
>> +    case PIX_FMT_BGR24: over->inout_rgba_map[BLUE ] = 0; over->inout_rgba_map[GREEN] = 1; over->inout_rgba_map[RED  ] = 2; over->inout_rgba_map[ALPHA] = 3; break;
>> +    default:
>> +        over->inout_is_packed_rgba = 0;
>> +    }
>
> This should be factored out in a separate function...

Yes.


>> +
>> +    if (over->inout_is_alpha)
>> +        av_log(ctx, AV_LOG_INFO,
>> +            "main has alpha\n");
>> +
>> +    if (over->inout_is_packed_rgba)
>> +        av_log(ctx, AV_LOG_INFO,
>
> AV_LOG_DEBUG

Ok.


>> +    switch (inlink->format) {
>> +    case PIX_FMT_ARGB:  over->blend_rgba_map[ALPHA] = 0; over->blend_rgba_map[RED  ] = 1; over->blend_rgba_map[GREEN] = 2; over->blend_rgba_map[BLUE ] = 3; break;
>> +    case PIX_FMT_ABGR:  over->blend_rgba_map[ALPHA] = 0; over->blend_rgba_map[BLUE ] = 1; over->blend_rgba_map[GREEN] = 2; over->blend_rgba_map[RED  ] = 3; break;
>> +    case PIX_FMT_RGBA:  over->blend_rgba_map[RED  ] = 0; over->blend_rgba_map[GREEN] = 1; over->blend_rgba_map[BLUE ] = 2; over->blend_rgba_map[ALPHA] = 3; break;
>> +    case PIX_FMT_BGRA:  over->blend_rgba_map[BLUE ] = 0; over->blend_rgba_map[GREEN] = 1; over->blend_rgba_map[RED  ] = 2; over->blend_rgba_map[ALPHA] = 3; break;
>> +    default:
>> +        over->blend_is_packed_rgba = 0;
>> +    }
>
> Code duplication.

Yes.


>> +    if (over->inout_is_packed_rgba)
>> +            av_log(ctx, AV_LOG_INFO,
>
> AV_LOG_DEBUG

Ok.


>> +    if (over->inout_is_packed_rgba != over->blend_is_packed_rgba) {
>> +        av_log(ctx, AV_LOG_ERROR,
>> +               "Main and overlay are not similar formats, cannot mix YUV and RGB.\nInsert a format filter to change a stream's format.\n");
>> +        return AVERROR(EINVAL);
>> +    }
>
> I believe this should be avoided/removed.

I agree, but within the current framework - what should happen when an 
experienced user who has set the extra command line parameter to enable 
RGB processing gets it wrong and feeds the overlay filter with a mixture 
of YUV and RGB?


>> +                // m is the blend multiplication of overlay over the main main
>
> can be removed

True.


>> +                if (over->inout_is_alpha)
>> +                {
>> +                    switch (blend)
>> +                    {
>
> Nit: the favored style is
> if (...) {
>      switch (...) {

Understood - K&R it is.


>> +                        case 0:
>> +                        case 0xff:
>> +                            break;
>> +                        default:
>> +                            // the un-premultiplied calculation is:
>> +                            // (255 * 255 * overlay_alpha) / ( 255 * (overlay_alpha + main_alpha) - (overlay_alpha * main_alpha) )
>> +                            blend =
>> +                            // the next line is a faster version of:  0xff * 0xff * blend
>> +                                ( (blend<<  16) - (blend<<  9) + blend )
>> +                                / (
>> +                            // the next line is a faster version of:  0xff * (blend + d[over->inout_rgba_map[ALPHA]])
>> +                                    ((blend + d[over->inout_rgba_map[ALPHA]])<<  8 ) - (blend + d[over->inout_rgba_map[ALPHA]])
>> +                                    - d[over->inout_rgba_map[ALPHA]] * blend
>> +                                );
>
> I can't understand this.
>
> When overlaying over the main input, the main alpha should remain
> unchanged, only the overlay alpha channel is used for blending over
> the main input.

Not when the background has alpha. When the background has alpha the 
proportion of the overlay video to the background video is proportional 
to the overlay alpha and inversely proportional to the background alpha.

This is how, for instance, Adobe After Effects does it when you are 
blending an overlay over an alpha. This code returns pixel values to 
within 1/255th of the values After Effects returns.


>> +                    d += 4;
>> +                } else {
>> +                    d += 3;
>> +                }
>>                   s += 4;
>
> d += dst_pixstep;
> s += src_pixstep;
>
> more clear and more robust

Ok.


-- 
Mark