Re: [FFmpeg-devel] [PATCH] avfilter: add fillborders filter

2017-11-30 Thread Paul B Mahol 
On 11/30/17, Moritz Barsnick  wrote:
> On Wed, Nov 29, 2017 at 20:16:16 +0100, Paul B Mahol wrote:
>> +@item mode
>> +Set fill mode.
>> +
>> +It accepts the following values:
>> +@table @samp
>> +@item smear
>> +fill pixels using outermost pixels
>> +
>> +@item mirror
>> +fill pixels using mirroring
>> +
>> +@item fixed
>> +fill pixels with constant value
>> +@end table
>
> "Default value is @code{smear}."

Fixed.

>
>> +Set color for pixels in fixed mode. Defauls is @var{black}.
> Typo:  ^ Default

Fixed.

>
>
>> +s->yuv_color[Y] = RGB_TO_Y_CCIR(s->rgba_color[R], s->rgba_color[G],
>> s->rgba_color[B]);
>> +s->yuv_color[U] = RGB_TO_U_CCIR(s->rgba_color[R], s->rgba_color[G],
>> s->rgba_color[B], 0);
>> +s->yuv_color[V] = RGB_TO_V_CCIR(s->rgba_color[R], s->rgba_color[G],
>> s->rgba_color[B], 0);
>> +s->yuv_color[A] = s->rgba_color[A];
>
> This RGB to YUV calculation could go into the else block below, since
> it's only required/used there, right?:

It could but it does not hurt to have it where it is.

>
>> +if (desc->flags & AV_PIX_FMT_FLAG_RGB) {
>> +memcpy(s->fill, s->rgba_color, sizeof(s->rgba_color));
>> +} else {
>> +memcpy(s->fill, s->yuv_color, sizeof(s->yuv_color));
>> +}
>
> Moritz
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avfilter: add fillborders filter

2017-11-30 Thread Moritz Barsnick 
On Wed, Nov 29, 2017 at 20:16:16 +0100, Paul B Mahol wrote:
> +@item mode
> +Set fill mode.
> +
> +It accepts the following values:
> +@table @samp
> +@item smear
> +fill pixels using outermost pixels
> +
> +@item mirror
> +fill pixels using mirroring
> +
> +@item fixed
> +fill pixels with constant value
> +@end table

"Default value is @code{smear}."

> +Set color for pixels in fixed mode. Defauls is @var{black}.
Typo:  ^ Default


> +s->yuv_color[Y] = RGB_TO_Y_CCIR(s->rgba_color[R], s->rgba_color[G], 
> s->rgba_color[B]);
> +s->yuv_color[U] = RGB_TO_U_CCIR(s->rgba_color[R], s->rgba_color[G], 
> s->rgba_color[B], 0);
> +s->yuv_color[V] = RGB_TO_V_CCIR(s->rgba_color[R], s->rgba_color[G], 
> s->rgba_color[B], 0);
> +s->yuv_color[A] = s->rgba_color[A];

This RGB to YUV calculation could go into the else block below, since
it's only required/used there, right?:

> +if (desc->flags & AV_PIX_FMT_FLAG_RGB) {
> +memcpy(s->fill, s->rgba_color, sizeof(s->rgba_color));
> +} else {
> +memcpy(s->fill, s->yuv_color, sizeof(s->yuv_color));
> +}

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

Re: [FFmpeg-devel] [PATCH] avfilter: add fillborders filter

2017-11-29 Thread Nicolas George 
Paul B Mahol (2017-11-29):
> Nope.

Well, then learn how to write documentation.

Regards,

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

Re: [FFmpeg-devel] [PATCH] avfilter: add fillborders filter

2017-11-29 Thread Paul B Mahol 
On 11/29/17, Moritz Barsnick  wrote:
> On Wed, Nov 29, 2017 at 13:56:08 +0100, Paul B Mahol wrote:
>> +@section fillborders
>> +
>> +Fill borders of the input video.
>
> Nice, but isn't this something the drawbox filter could be enhanced to
> do?

Nope.

>
>> +fill blank pixels
>
> Does this mean "fill with blank pixels"? Does this imply transparency?
> (Sorry, I couldn't figure that out from the code at first glance.)

Fill pixels of fixed value.

>
>> +Set color for blank pixels. Defauls is @var{black}.
>^ Default
>
>>  OBJS-$(CONFIG_FIND_RECT_FILTER)  += vf_find_rect.o
>> lavfutils.o
>> +OBJS-$(CONFIG_FILLBORDERS_FILTER)+= vf_fillborders.o
>>  OBJS-$(CONFIG_FLOODFILL_FILTER)  += vf_floodfill.o
>
> Alphabetical order. ;-)

Fixed.

>
>>  REGISTER_FILTER(FIND_RECT,  find_rect,  vf);
>> +REGISTER_FILTER(FILLBORDERS,fillborders,vf);
>>  REGISTER_FILTER(FLOODFILL,  floodfill,  vf);
>
> Ditto.

Fixed.

>
>> +{ "left",   "set the left fill border",   OFFSET(left),
>> AV_OPT_TYPE_INT, {.i64=0}, 0, INT_MAX,FLAGS },
>> +{ "right",  "set the right fill border",  OFFSET(right),
>> AV_OPT_TYPE_INT, {.i64=0}, 0, INT_MAX,FLAGS },
>> +{ "top","set the top fill border",OFFSET(top),
>> AV_OPT_TYPE_INT, {.i64=0}, 0, INT_MAX,FLAGS },
>> +{ "bottom", "set the bottom fill border", OFFSET(bottom),
>> AV_OPT_TYPE_INT, {.i64=0}, 0, INT_MAX,FLAGS },
>
> Could make that "... fill border width" or something (which may be
> redundant - I'm not sure).

Better not.

>
>> +{ "mode",   "set the fill borders mode",  OFFSET(mode),
>> AV_OPT_TYPE_INT, {.i64=0}, 0, FM_NB_MODES-1, FLAGS, "mode" },
>> +{ "smear",  NULL, 0, AV_OPT_TYPE_CONST, {.i64=FM_SMEAR},  0, 0,
>> FLAGS, "mode" },
>> +{ "mirror", NULL, 0, AV_OPT_TYPE_CONST, {.i64=FM_MIRROR}, 0, 0,
>> FLAGS, "mode" },
>> +{ "blank",  NULL, 0, AV_OPT_TYPE_CONST, {.i64=FM_BLANK},  0, 0,
>> FLAGS, "mode" },
>
> The default mode ("{.i64=0}") should also be encoded with use of an
> explicit macro.

Fixed.

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

Re: [FFmpeg-devel] [PATCH] avfilter: add fillborders filter

2017-11-29 Thread Paul B Mahol 
On 11/29/17, Nicolas George  wrote:
> Paul B Mahol (2017-11-29):
>> +Fill borders of the input video.
>
> Am I missing something or are you proposing 277 lines of code for
> something that drawbox or crop+pad can already do?

By careful patch examination, one can find out that filter provides
additional functionality: filling by smearing and mirroring - no
crop/drawbox/pad filter combinations can archieve this at all or if
it can in case of just blanking it comes with extra cost of copying
video frame.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avfilter: add fillborders filter

2017-11-29 Thread Moritz Barsnick 
On Wed, Nov 29, 2017 at 13:56:08 +0100, Paul B Mahol wrote:
> +@section fillborders
> +
> +Fill borders of the input video.

Nice, but isn't this something the drawbox filter could be enhanced to
do?

> +fill blank pixels

Does this mean "fill with blank pixels"? Does this imply transparency?
(Sorry, I couldn't figure that out from the code at first glance.)

> +Set color for blank pixels. Defauls is @var{black}.
   ^ Default

>  OBJS-$(CONFIG_FIND_RECT_FILTER)  += vf_find_rect.o lavfutils.o
> +OBJS-$(CONFIG_FILLBORDERS_FILTER)+= vf_fillborders.o
>  OBJS-$(CONFIG_FLOODFILL_FILTER)  += vf_floodfill.o

Alphabetical order. ;-)

>  REGISTER_FILTER(FIND_RECT,  find_rect,  vf);
> +REGISTER_FILTER(FILLBORDERS,fillborders,vf);
>  REGISTER_FILTER(FLOODFILL,  floodfill,  vf);

Ditto.

> +{ "left",   "set the left fill border",   OFFSET(left),   
> AV_OPT_TYPE_INT, {.i64=0}, 0, INT_MAX,FLAGS },
> +{ "right",  "set the right fill border",  OFFSET(right),  
> AV_OPT_TYPE_INT, {.i64=0}, 0, INT_MAX,FLAGS },
> +{ "top","set the top fill border",OFFSET(top),
> AV_OPT_TYPE_INT, {.i64=0}, 0, INT_MAX,FLAGS },
> +{ "bottom", "set the bottom fill border", OFFSET(bottom), 
> AV_OPT_TYPE_INT, {.i64=0}, 0, INT_MAX,FLAGS },

Could make that "... fill border width" or something (which may be
redundant - I'm not sure).

> +{ "mode",   "set the fill borders mode",  OFFSET(mode),   
> AV_OPT_TYPE_INT, {.i64=0}, 0, FM_NB_MODES-1, FLAGS, "mode" },
> +{ "smear",  NULL, 0, AV_OPT_TYPE_CONST, {.i64=FM_SMEAR},  0, 0, 
> FLAGS, "mode" },
> +{ "mirror", NULL, 0, AV_OPT_TYPE_CONST, {.i64=FM_MIRROR}, 0, 0, 
> FLAGS, "mode" },
> +{ "blank",  NULL, 0, AV_OPT_TYPE_CONST, {.i64=FM_BLANK},  0, 0, 
> FLAGS, "mode" },

The default mode ("{.i64=0}") should also be encoded with use of an
explicit macro.

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

Re: [FFmpeg-devel] [PATCH] avfilter: add fillborders filter

2017-11-29 Thread Nicolas George 
Paul B Mahol (2017-11-29):
> +Fill borders of the input video.

Am I missing something or are you proposing 277 lines of code for
something that drawbox or crop+pad can already do?

Regards,

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

Re: [FFmpeg-devel] [PATCH] avfilter: add fillborders filter

2017-11-18 Thread Paul B Mahol 
On 11/18/17, Rostislav Pehlivanov  wrote:
> On 18 November 2017 at 18:53, Paul B Mahol  wrote:
>
>> Signed-off-by: Paul B Mahol 
>> ---
>>  doc/filters.texi |  31 ++
>>  libavfilter/Makefile |   1 +
>>  libavfilter/allfilters.c |   1 +
>>  libavfilter/vf_fillborders.c | 232 ++
>> +
>>  4 files changed, 265 insertions(+)
>>  create mode 100644 libavfilter/vf_fillborders.c
>>
>> diff --git a/doc/filters.texi b/doc/filters.texi
>> index 5d99437871..7a23d8de04 100644
>> --- a/doc/filters.texi
>> +++ b/doc/filters.texi
>> @@ -8607,6 +8607,37 @@ ffmpeg -i file.ts -vf
>> find_rect=newref.pgm,cover_rect=cover.jpg:mode=cover
>> new.m
>>  @end example
>>  @end itemize
>>
>> +@section fillborders
>> +
>> +Fill borders of the input video.
>> +
>> +It accepts the following options:
>> +
>> +@table @option
>> +@item left
>> +Number of pixels to fill from left border.
>> +
>> +@item right
>> +Number of pixels to fill from right border.
>> +
>> +@item top
>> +Number of pixels to fill from top border.
>> +
>> +@item bottom
>> +Number of pixels to fill from bottom border.
>> +
>> +@item mode
>> +Set fill mode.
>> +
>> +It accepts the following values:
>> +@table @samp
>> +@item smear
>> +fill pixels using outermost pixels
>> +@item mirror
>> +fill pixels using mirroring
>> +@end table
>> +@end table
>> +
>>  @section floodfill
>>
>>
> MpegvideoEncDSPContext->draw_edges seems to pretty much do that, and it has
> SIMD. Could you use it instead?

Its in wrong library, and is just smear mode.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avfilter: add fillborders filter

2017-11-18 Thread Rostislav Pehlivanov 
On 18 November 2017 at 18:53, Paul B Mahol  wrote:

> Signed-off-by: Paul B Mahol 
> ---
>  doc/filters.texi |  31 ++
>  libavfilter/Makefile |   1 +
>  libavfilter/allfilters.c |   1 +
>  libavfilter/vf_fillborders.c | 232 ++
> +
>  4 files changed, 265 insertions(+)
>  create mode 100644 libavfilter/vf_fillborders.c
>
> diff --git a/doc/filters.texi b/doc/filters.texi
> index 5d99437871..7a23d8de04 100644
> --- a/doc/filters.texi
> +++ b/doc/filters.texi
> @@ -8607,6 +8607,37 @@ ffmpeg -i file.ts -vf 
> find_rect=newref.pgm,cover_rect=cover.jpg:mode=cover
> new.m
>  @end example
>  @end itemize
>
> +@section fillborders
> +
> +Fill borders of the input video.
> +
> +It accepts the following options:
> +
> +@table @option
> +@item left
> +Number of pixels to fill from left border.
> +
> +@item right
> +Number of pixels to fill from right border.
> +
> +@item top
> +Number of pixels to fill from top border.
> +
> +@item bottom
> +Number of pixels to fill from bottom border.
> +
> +@item mode
> +Set fill mode.
> +
> +It accepts the following values:
> +@table @samp
> +@item smear
> +fill pixels using outermost pixels
> +@item mirror
> +fill pixels using mirroring
> +@end table
> +@end table
> +
>  @section floodfill
>
>
MpegvideoEncDSPContext->draw_edges seems to pretty much do that, and it has
SIMD. Could you use it instead?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel