Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alpha channel heuristic when not using ImageWorker doesn't deal with uppercase file extensions #493

Closed
pascalatfetch opened this issue Jun 21, 2023 · 6 comments · Fixed by #497 or #507
Assignees
Labels

Comments

@pascalatfetch
Copy link

I tried loading an externally supplied file with .PNG extension, and the alpha channel wasn't clear.
It seems that the heuristic is to just look at the file path, whereas the ImageWorker detects mime types.
That said, there is a flag hasAlpha which the ImageWorker subsequently ignores.

Should the non-worker based code path use a FileReader and do mime type detection as well, or is that too much of a performance hit? At the very least the heuristic string comparison should ignore case.

Looking into this, the code responsible here is this line:

https://github.com/rdkcentral/Lightning/blob/master/src/platforms/browser/WebPlatform.mjs#L270

image

wouterlucas added a commit to wouterlucas/Lightning that referenced this issue Jun 28, 2023
This fix solves an issue when the image worker is not being used, so Lightning processes images on the main thread. There is a simple check for `.png` to toggle whether or not the image has an alpha channel. This however gives an issue when a `.PNG` is used. Introduced a simple lowerCase cast to be able to work with both `.png` and `.PNG`.

Additionally added some (quick and dirty) vitest cases to validate the above changes.
Resolves: rdkcentral#493
@wouterlucas
Copy link
Contributor

Hi @pascalatfetch ! Thanks for reporting, I made a tiny fix to straighten this out. Hope to get this into the next release!

@pascalatfetch
Copy link
Author

@wouterlucas Great work.

Do you think the image worker ignoring the hasAlpha flag is a potential issue?

@wouterlucas
Copy link
Contributor

During my tests it didn’t seem to ignore the hasAlpha flag on the function itself.

This might be higher up the chain if that is the case, I can look into it but suggest we move this along for the next release.

@pascalatfetch
Copy link
Author

pascalatfetch commented Jul 2, 2023

I don't think it's super important. Just saw this as a discrepancy.
Maybe I didn't explain what I meant correctly:

When providing the hasAlpha flag to the actual component, it is overriding the heuristic outcome when not running in a worker, but ignored when the worker finishes.

Instead, on worker execution, the hasAlphaChannel result is the only value that is acknowledged.

A fix for that might be to process hasAlpha || hasAlphaChannel when the worker finishes?

image

@wouterlucas
Copy link
Contributor

ah yes, thanks for clarifying. It indeed seems to be ignored on the image worker because it relies on the mimeType:
https://github.com/rdkcentral/Lightning/blob/master/src/platforms/browser/ImageWorker.mjs#L306

Although I agree that doesn't make sense and it should probably be included when evaluating the alpha also on the image worker. I do think the mimeType itself is solid way to check for the alpha channel, not sure if having an override is necessary at this point.

@elsassph
Copy link
Contributor

It is certainly awkward when image URLs don't include .png somewhere... so we add #.png to the URL in places where we know we want the image to be transparent...

@uguraslan uguraslan added this to the July 2023 release milestone Jul 12, 2023
@uguraslan uguraslan mentioned this issue Jul 12, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment