Page MenuHomePhabricator

evas filter: Implement grayscale filter in pure GL
ClosedPublic

Authored by kimcinoo on Sep 6 2019, 4:01 AM.

Details

Summary

Initial version implementing grayscale filter in pure GL.
This patch needs a logt of love as 5bce712 did.

Grasyscale formula:
https://www.tutorialspoint.com/dip/grayscale_to_rgb_conversion.htm

Test Plan
  1. Create filter_example with following .

efl_gfx_filter_program_set(image, "grayscale ()", "grayscale");

  1. Run.

ELM_ACCEL=gl ./filter_example

Diff Detail

Repository
rEFL core/efl
Branch
efl.gfx.filter_add.grayscale.filter
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13150
Build 9343: arc lint + arc unit
kimcinoo created this revision.Sep 6 2019, 4:01 AM
kimcinoo requested review of this revision.Sep 6 2019, 4:01 AM
kimcinoo edited the test plan for this revision. (Show Details)Sep 6 2019, 4:05 AM
Hermet requested changes to this revision.Wed, Sep 18, 11:54 PM

Overall assembled code looks fine to me some unnecessary debugging prints just bothers other developers to checking logging. (except it occurs performance drop);

+two question.
This grayscale filter (r, r, r, a) is generic one? this logic ignores the brightness since it drop other color channels, Were did you get this method?

+will you go work for sw backend?

src/lib/evas/filters/evas_filter.c
1610
in = _filter_buffer_get(ctx, inbuf);
EINA_SAFETY_ON_NULL_RETURN_VAL(in, NULL);

out = _filter_buffer_get(ctx, outbuf);
EINA_SAFETY_ON_NULL_RETURN_VAL(out, NULL);
src/modules/evas/engines/gl_generic/filters/gl_filter_grayscale.c
6

no debugging.

23

if it doesn't necessary i'd like to suggest remove it.

This revision now requires changes to proceed.Wed, Sep 18, 11:54 PM
kimcinoo marked an inline comment as done.Wed, Sep 25, 12:38 AM

+two question.
This grayscale filter (r, r, r, a) is generic one? this logic ignores the brightness since it drop other color channels, Were did you get this method?

Yes you are right. I will update it based on https://www.tutorialspoint.com/dip/grayscale_to_rgb_conversion.htm

+will you go work for sw backend?

Sure thing.

src/lib/evas/filters/evas_filter.c
1610

This one is just following other lines coding style of evas_filter.c so I would like to keep current status.

src/modules/evas/engines/gl_generic/filters/gl_filter_grayscale.c
23

This is just following other files in gl_generic/filters so I would like to keep current status.

kimcinoo updated this revision to Diff 25555.Wed, Sep 25, 12:45 AM

Update based on review opinion

kimcinoo edited the summary of this revision. (Show Details)Wed, Sep 25, 12:48 AM
kimcinoo updated this revision to Diff 25557.Wed, Sep 25, 12:56 AM

Use correct grayscale formula

kimcinoo updated this revision to Diff 25558.Wed, Sep 25, 1:02 AM

Use correct grayscale formula

A small suggestion to check D9490
If colors are save as bytes there are optimized techniques to avoid float point calculation. (I do not know if this is useful here :) )

Also you can use more accurate values for RGB to Y conversion from here
https://en.wikipedia.org/wiki/YUV (0.299, 0.587, 0.114)

A small suggestion to check D9490
If colors are save as bytes there are optimized techniques to avoid float point calculation. (I do not know if this is useful here :) )

Thank you for the suggestion. It seems that D9490 is for correct calculation. You might mean " >> 16". Here c.rgb = 0.3 * c.r + 0.59 * c.g + 0.11 * c.b; is enough because it is normalized already.

Also you can use more accurate values for RGB to Y conversion from here
https://en.wikipedia.org/wiki/YUV (0.299, 0.587, 0.114)

Thank you for information. I did not know Y′ stands for the luma component (the brightness).

Don't just copy & paste.
If something wrong or unnecessary, you don't need to keep the consistency.

src/lib/evas/filters/evas_filter.c
1610

That's bad one. you should not follow the wrong version.

src/modules/evas/engines/gl_generic/filters/gl_filter_grayscale.c
23

Don't add more wrong stuff.

kimcinoo updated this revision to Diff 25737.Thu, Sep 26, 3:06 PM

Remove unnecessary call

kimcinoo marked an inline comment as done.Thu, Sep 26, 3:13 PM
kimcinoo added inline comments.
src/lib/evas/filters/evas_filter.c
1610

Reasonable

src/modules/evas/engines/gl_generic/filters/gl_filter_grayscale.c
23

This was useful for me and someone could need this as well.

Hermet requested changes to this revision.Thu, Sep 26, 9:51 PM
Hermet added inline comments.
src/modules/evas/engines/gl_generic/filters/gl_filter_grayscale.c
23

Firsty, leaves to someone. They can decide by themselves. they would add necessary debugging stuff when it's really necessary.
debugging is debugging,
If you don't need to debugging anymore, then it's useless. remove is better.

This revision now requires changes to proceed.Thu, Sep 26, 9:51 PM
Hermet resigned from this revision.Thu, Sep 26, 9:57 PM
Hermet added inline comments.
src/lib/evas/filters/evas_filter.c
1610

A)
a = calloc();
//DO SOMETHING

if (!a) EXCEPTION!

B)
a = calloc();
if (!a) EXCEPTON!
//DO SOMETHING!

I think this is same example for this.
If you think A is correct and B is reasonable, then nothing to discuss here anymore.

kimcinoo marked an inline comment as done.Thu, Sep 26, 10:01 PM
kimcinoo added inline comments.
src/lib/evas/filters/evas_filter.c
1610

See carefully this part is changed

kimcinoo added inline comments.Thu, Sep 26, 10:02 PM
src/lib/evas/filters/evas_filter.c
1610

I said that your opinion is reasonable not mine

kimcinoo updated this revision to Diff 25746.Thu, Sep 26, 11:28 PM

Remove lines for debug information

This revision was not accepted when it landed; it landed in state Needs Review.Thu, Sep 26, 11:34 PM
This revision was automatically updated to reflect the committed changes.