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

[ENHANCE] Preview Quality Prop #10

Merged

Conversation

anthlasserre
Copy link
Contributor

GIF stills in a lower quality in preview mode.
So the goal of this PR is to let user to choose its own media type for preview with the props giphyPreviewQuality & tenorPreviewQuality.

See the example below.
Left with preview_gif media type and right with fixed_width media type from Giphy.
You can see a better quality on the right side.

@Thanasis1101 say me what do you think about it ?

Capture d’écran 2021-04-06 à 13 52 04

@Thanasis1101
Copy link
Owner

Thanasis1101 commented Apr 6, 2021

@anthlasserre yes, I was aware of this. I made it like this so that when using the gif search app through cellular data, the user would not consume more data due to bigger gif sizes that will be downloaded. From what I checked it's not an insignificant difference so thats why I chose this quality.

However, I had in mind to give the opportunity for better quality in the future, so yes, it would be better to let the developer decide about this. They could also check if data or wifi is being used and change the quality accordingly.

I was thinking what would be best. To have a more general property like quality with values high, medium, low and then set for giphy and tenor automatically ? Or give the properties as you suggested ? The first one is clearer and easier as api. The second one gives more choices to the developer and could be confusing and not intuitive, although we could overcome this with a good explanation in the README.md.

What do you think about this?

@anthlasserre
Copy link
Contributor Author

@Thanasis1101 Thanks for you quick reply. I agree with your high-medium-low quality solution. You are right. Like I suggest firstly in this PR is a little bit confusing regarding various/differents formats from Giphy & Tenor.
I will update my PR with your suggestion but I need to clarify something with you.

I suggest you to check what Giphy recommand for mobile use.
https://developers.giphy.com/docs/optional-settings/#rendition-guide

Can you confirm me that we can choose this new formats via previewQuality and sentQuality props.

Giphy

  • low: preview_gif
  • medium: fixed_width
  • high: downsized_large

Tenor

  • low: tinygif
  • medium: mediumgif
  • high: gif

@Thanasis1101
Copy link
Owner

@anthlasserre Thanks for the Giphy link, that's useful.

Regarding the suggested low-medium-high values, I saw some examples and compared their byte size. I agree with you on the Giphy ones, but on the Tenor values I would prefer nanogif instead of tinygif for low, in order to be more similar to low of Giphy and have smaller size for better data optimizaton. Also, I noticed that mediumgif and gif have no significant visual difference, they only differ in byte size. So we could use tinygif as our medium and leave mediumgif out. This way, I think, giphy and tenor respective quality values will be more similar. What do you think? You could also try these and see the differences live, so that you match them visually.

Regarding the property names, I would prefer previewGifQuality and selectedGifQuality. The 2nd property will also be for onLongPress I guess, right?

@anthlasserre
Copy link
Contributor Author

anthlasserre commented Apr 8, 2021

@Thanasis1101 Okey perfect! So to recap:

Giphy

  • low: preview_gif
  • medium: fixed_width
  • high: downsized_large

Tenor

  • low: nanogif
  • medium: tinygif
  • high: mediumgif

Props

  • previewGifQuality to enable to change preview gif format (values: 'low' | 'medium' | 'high')
  • selectedGifQuality to enable to change selected gif format (values: 'low' | 'medium' | 'high')

What do you think about it?

@Thanasis1101
Copy link
Owner

@anthlasserre About Tenor high I am not so sure. I had suggested for it to be gif instead of mediumgif. I think that since the developer selects it, there should be no issue with data consumption, so we could provide the best quality. Only issue could be slow loading, which would better be tested. So, I suggest that you try this with Tenor high: gif, and see if there is big difference in loading time between giphy and tenor gifs for high. If there is, then we should use mediumgif.

I am fine with all the rest !

@anthlasserre
Copy link
Contributor Author

@Thanasis1101 I just requested a Tenor API Key to test it.
Let's check a little test I made on first 5 GIFs between mediumgif and gif Tenor media formats.

image

Size

It makes a big difference. Generally mediumgif divide by 2 the gif size.

Quality

medium GIF format is fixed at 640px max in width and 640px max in height.
On all the media objects I've tried generally GIFs original formats never reach this 640px limit.
A GIF with a gif format of 250x250, its mediumgif equals to 640x640.
And it doesn't impact the size.

Recap

mediumgif is 2 times lighter than gif
mediumgif keep the same quality of gif
mediumgif is px dimensions stills generally bigger than gif

🏆 And the winner is: mediumgif

Can you confirm me that we stay on the previous Tenor config with mediumgif ?

Cheers mate

@Thanasis1101
Copy link
Owner

@anthlasserre great research !
I was aware of the size thing, but not so sure about the quality on mobile (if there was visual difference). Considering your data, I agree on mediumgif.
So yes, use the values as you suggested in your previous comment !

@anthlasserre
Copy link
Contributor Author

anthlasserre commented Apr 9, 2021

@Thanasis1101 Thanks for your quick reply.

What default values I set for previewGifQuality & selectedGifQualityprops ?

previewGifQuality: low
selectedGifQuality: medium

Are you okey with that ?

@anthlasserre
Copy link
Contributor Author

@Thanasis1101 I let's you check my updates.

@Thanasis1101
Copy link
Owner

@anthlasserre yes I think the defaults are fine ! Nice, when I have some time I will review, accept and publish the update !

@anthlasserre
Copy link
Contributor Author

@Thanasis1101 Thanks mate. Have a good week end 🍻

@Thanasis1101 Thanasis1101 merged commit e6dd12a into Thanasis1101:master Apr 11, 2021
@Thanasis1101
Copy link
Owner

@anthlasserre thank you very much !
I saw and tested your changes and updated the README.md.
Version 1.3.1 is now published ! Great job !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants