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

[Tobias Rapp]

On 01.08.2017 17:33, Paul B Mahol wrote:

On 8/1/17, Tobias Rapp  wrote:

On 01.08.2017 15:31, Paul B Mahol wrote:

On 8/1/17, Tobias Rapp  wrote:

On 01.08.2017 13:03, Paul B Mahol wrote:

Signed-off-by: Paul B Mahol 
---
 doc/filters.texi |  13 ++
 libavfilter/Makefile |   1 +
 libavfilter/allfilters.c |   1 +
 libavfilter/vf_premultiply.c | 307
---
 4 files changed, 277 insertions(+), 45 deletions(-)

diff --git a/doc/filters.texi b/doc/filters.texi
index 4089135..a50696a 100644
--- a/doc/filters.texi
+++ b/doc/filters.texi
@@ -14532,6 +14532,19 @@ ffmpeg -i INPUT -vf trim=duration=1

 @end itemize

+@section unpremultiply
+Apply alpha unpremultiply effect to input video stream using first
plane
+of second stream as alpha.
+
+Both streams must have same dimensions and same pixel format.
+
+The filter accepts the following option:
+
+@table @option
+@item planes
+Set which planes will be processed, unprocessed planes will be copied.
+By default value 0xf, all planes will be processed.
+@end table


IMHO using a flags-like string "planes=rgb" would be more user-friendly
than a bitmask. At least the documentation should tell which bit refers
to what channel.


It is directly related to pixel format.


I'm just wondering whether I can apply the logic of
AVPixFmtDescriptor.comp to the planes bitmask or not:

 /**
  * Parameters that describe how pixels are packed.
  * If the format has 1 or 2 components, then luma is 0.
  * If the format has 3 or 4 components:
  *   if the RGB flag is set then 0 is red, 1 is green and 2 is blue;
  *   otherwise 0 is luma, 1 is chroma-U and 2 is chroma-V.
  *
  * If present, the Alpha channel is always the last component.
  */


I mainly did it bitmask way because of additional code needed to handle cases
when there is no r/g/b but y/u/v planes and user supplied r/g/b only.



Indeed a bitmask is more generic. I'm not against it but think that 
there should be some more details in the user documentation on how to 
map the bits to planes. For example (in case the code comment above 
applies) something like:


"""
If the format has 1 or 2 components, then luma is bit 0.
If the format has 3 or 4 components:
  for RGB formats bit 0 is red, bit 1 is green and bit 2 is blue;
  for YUV formats bit 0 is luma, bit 1 is chroma-U and bit 2 is chroma-V.
If present, the alpha channel is always the last bit.
"""

Regards,
Tobias

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

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

[Paul B Mahol]

On 8/1/17, Tobias Rapp  wrote:
> On 01.08.2017 15:31, Paul B Mahol wrote:
>> On 8/1/17, Tobias Rapp  wrote:
>>> On 01.08.2017 13:03, Paul B Mahol wrote:
 Signed-off-by: Paul B Mahol 
 ---
  doc/filters.texi |  13 ++
  libavfilter/Makefile |   1 +
  libavfilter/allfilters.c |   1 +
  libavfilter/vf_premultiply.c | 307
 ---
  4 files changed, 277 insertions(+), 45 deletions(-)

 diff --git a/doc/filters.texi b/doc/filters.texi
 index 4089135..a50696a 100644
 --- a/doc/filters.texi
 +++ b/doc/filters.texi
 @@ -14532,6 +14532,19 @@ ffmpeg -i INPUT -vf trim=duration=1

  @end itemize

 +@section unpremultiply
 +Apply alpha unpremultiply effect to input video stream using first
 plane
 +of second stream as alpha.
 +
 +Both streams must have same dimensions and same pixel format.
 +
 +The filter accepts the following option:
 +
 +@table @option
 +@item planes
 +Set which planes will be processed, unprocessed planes will be copied.
 +By default value 0xf, all planes will be processed.
 +@end table
>>>
>>> IMHO using a flags-like string "planes=rgb" would be more user-friendly
>>> than a bitmask. At least the documentation should tell which bit refers
>>> to what channel.
>>
>> It is directly related to pixel format.
>
> I'm just wondering whether I can apply the logic of
> AVPixFmtDescriptor.comp to the planes bitmask or not:
>
>  /**
>   * Parameters that describe how pixels are packed.
>   * If the format has 1 or 2 components, then luma is 0.
>   * If the format has 3 or 4 components:
>   *   if the RGB flag is set then 0 is red, 1 is green and 2 is blue;
>   *   otherwise 0 is luma, 1 is chroma-U and 2 is chroma-V.
>   *
>   * If present, the Alpha channel is always the last component.
>   */

I mainly did it bitmask way because of additional code needed to handle cases
when there is no r/g/b but y/u/v planes and user supplied r/g/b only.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Tobias Rapp]

On 01.08.2017 15:31, Paul B Mahol wrote:

On 8/1/17, Tobias Rapp  wrote:

On 01.08.2017 13:03, Paul B Mahol wrote:

Signed-off-by: Paul B Mahol 
---
 doc/filters.texi |  13 ++
 libavfilter/Makefile |   1 +
 libavfilter/allfilters.c |   1 +
 libavfilter/vf_premultiply.c | 307
---
 4 files changed, 277 insertions(+), 45 deletions(-)

diff --git a/doc/filters.texi b/doc/filters.texi
index 4089135..a50696a 100644
--- a/doc/filters.texi
+++ b/doc/filters.texi
@@ -14532,6 +14532,19 @@ ffmpeg -i INPUT -vf trim=duration=1

 @end itemize

+@section unpremultiply
+Apply alpha unpremultiply effect to input video stream using first plane
+of second stream as alpha.
+
+Both streams must have same dimensions and same pixel format.
+
+The filter accepts the following option:
+
+@table @option
+@item planes
+Set which planes will be processed, unprocessed planes will be copied.
+By default value 0xf, all planes will be processed.
+@end table


IMHO using a flags-like string "planes=rgb" would be more user-friendly
than a bitmask. At least the documentation should tell which bit refers
to what channel.


It is directly related to pixel format.


I'm just wondering whether I can apply the logic of 
AVPixFmtDescriptor.comp to the planes bitmask or not:


/**
 * Parameters that describe how pixels are packed.
 * If the format has 1 or 2 components, then luma is 0.
 * If the format has 3 or 4 components:
 *   if the RGB flag is set then 0 is red, 1 is green and 2 is blue;
 *   otherwise 0 is luma, 1 is chroma-U and 2 is chroma-V.
 *
 * If present, the Alpha channel is always the last component.
 */

Regards,
Tobias

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

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

[Paul B Mahol]

On 8/1/17, Tobias Rapp  wrote:
> On 01.08.2017 13:03, Paul B Mahol wrote:
>> Signed-off-by: Paul B Mahol 
>> ---
>>  doc/filters.texi |  13 ++
>>  libavfilter/Makefile |   1 +
>>  libavfilter/allfilters.c |   1 +
>>  libavfilter/vf_premultiply.c | 307
>> ---
>>  4 files changed, 277 insertions(+), 45 deletions(-)
>>
>> diff --git a/doc/filters.texi b/doc/filters.texi
>> index 4089135..a50696a 100644
>> --- a/doc/filters.texi
>> +++ b/doc/filters.texi
>> @@ -14532,6 +14532,19 @@ ffmpeg -i INPUT -vf trim=duration=1
>>
>>  @end itemize
>>
>> +@section unpremultiply
>> +Apply alpha unpremultiply effect to input video stream using first plane
>> +of second stream as alpha.
>> +
>> +Both streams must have same dimensions and same pixel format.
>> +
>> +The filter accepts the following option:
>> +
>> +@table @option
>> +@item planes
>> +Set which planes will be processed, unprocessed planes will be copied.
>> +By default value 0xf, all planes will be processed.
>> +@end table
>
> IMHO using a flags-like string "planes=rgb" would be more user-friendly
> than a bitmask. At least the documentation should tell which bit refers
> to what channel.

It is directly related to pixel format.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Tobias Rapp]

On 01.08.2017 13:03, Paul B Mahol wrote:

Signed-off-by: Paul B Mahol 
---
 doc/filters.texi |  13 ++
 libavfilter/Makefile |   1 +
 libavfilter/allfilters.c |   1 +
 libavfilter/vf_premultiply.c | 307 ---
 4 files changed, 277 insertions(+), 45 deletions(-)

diff --git a/doc/filters.texi b/doc/filters.texi
index 4089135..a50696a 100644
--- a/doc/filters.texi
+++ b/doc/filters.texi
@@ -14532,6 +14532,19 @@ ffmpeg -i INPUT -vf trim=duration=1

 @end itemize

+@section unpremultiply
+Apply alpha unpremultiply effect to input video stream using first plane
+of second stream as alpha.
+
+Both streams must have same dimensions and same pixel format.
+
+The filter accepts the following option:
+
+@table @option
+@item planes
+Set which planes will be processed, unprocessed planes will be copied.
+By default value 0xf, all planes will be processed.
+@end table


IMHO using a flags-like string "planes=rgb" would be more user-friendly 
than a bitmask. At least the documentation should tell which bit refers 
to what channel.



[...]



Some FATE test for the new filter would be welcome.

Regards,
Tobias

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

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

[Nicolas George]

Le quartidi 14 thermidor, an CCXXV, Paul B Mahol a écrit :
> Signed-off-by: Paul B Mahol 
> ---
>  doc/filters.texi |  13 ++
>  libavfilter/Makefile |   1 +
>  libavfilter/allfilters.c |   1 +
>  libavfilter/vf_premultiply.c | 307 
> ---
>  4 files changed, 277 insertions(+), 45 deletions(-)

Thanks for the change. It looks ok to me now.

Regards,

-- 
  Nicolas George


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

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

[Nicolas George]

Le quartidi 14 thermidor, an CCXXV, Paul B Mahol a écrit :
> Signed-off-by: Paul B Mahol 
> ---
>  doc/filters.texi   |  13 ++
>  libavfilter/Makefile   |   1 +
>  libavfilter/allfilters.c   |   1 +
>  libavfilter/vf_unpremultiply.c | 431 
> +
>  4 files changed, 446 insertions(+)
>  create mode 100644 libavfilter/vf_unpremultiply.c

It looks like a copy-paste of vf_premultiply.c with the callback
changed. I think it would be better to use the same code, either two
filters with the same function or just an option "inverse=1" to
premultiply itself.

> +s->unpremultiply[0] = limited ? unpremultiply8offset : 
> unpremultiply8;
> +s->unpremultiply[1] = limited ? unpremultiply8offset : 
> unpremultiply8;
> +s->unpremultiply[2] = limited ? unpremultiply8offset : 
> unpremultiply8;

That can be merged.

Regards,

-- 
  Nicolas George


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