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

webp format support (Android Only) for GLContext.takeSnapshot function #7490

Merged
merged 2 commits into from May 17, 2020

Conversation

pacoelayudante
Copy link
Contributor

Why

At some point (API 14), Android added native WebP support (Bitmap.CompressFormat.WEBP), so adding the option to export the render to WebP is trivial.
Creating a new module just for converting from PNG to WebP is silly since you can get the GL render as WebP directly. Also converting from PNG to WebP could create quality loss.

How

In GLContext.java, I extended the block that specifies using png (instead of jpeg, the default) to recognize webp as another posible format.

Test Plan

Sadly, a combination of living in Windows OS and lack of experience in some aspects of software development, I was not able to test this change using the proper methods (unit test, and that). But I did tried the change in the App I'm building and it worked fine

Thanks for your attention, sorry for any mistake in the PR, this is the second time I do a PR

@pacoelayudante
Copy link
Contributor Author

@brentvatne ping like this?

@brentvatne brentvatne requested a review from nikki93 March 27, 2020 22:40
@brentvatne
Copy link
Member

@nikki93 will review this next time he's around!

@nikki93
Copy link
Contributor

nikki93 commented Mar 30, 2020

@pacoelayudante thanks for the PR! I just tested it and the code does work on Android. We would also need an accompanying documentation change in https://github.com/expo/expo/blob/master/docs/pages/versions/unversioned/sdk/gl-view.md#glviewtakesnapshotasyncgl-options explaining the option, and clearly explaining that it isn't supported on Android. Also I think a corresponding change should be added on iOS with a proper error message explaining that the format isn't available on iOS. I think these changes should be included before it makes it into the main library.

@pacoelayudante
Copy link
Contributor Author

So, I have no Mac, and I have to make the programming skillcheck with a -2 penalty since I don't have any skill point on Objective-C. So well... I'll just give it a go and hope for the best.
Also, I think is better to display a warning and exporting the image as png, sounds reasonable to me.
And finally, I will try a way to not clutter the docs, tell me what you think about it

@pacoelayudante
Copy link
Contributor Author

OK! so check it out @nikki93

Copy link
Member

@tsapeta tsapeta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @pacoelayudante! 👍

@tsapeta tsapeta merged commit 093704f into expo:master May 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants