Skip to content
This repository has been archived by the owner. It is now read-only.

Use CSS for cropper rotation #1467

Closed
zaach opened this issue Jul 29, 2014 · 27 comments
Closed

Use CSS for cropper rotation #1467

zaach opened this issue Jul 29, 2014 · 27 comments

Comments

@zaach
Copy link
Contributor

@zaach zaach commented Jul 29, 2014

The cropper rotates the image using canvas, which is really slow on mobile. I'm talking "use a Nexus 5 but still wait 15 seconds to rotate" slow. Let's rotate the image with CSS and do the canvas rotation at export like decent human beings.

@shane-tomlinson
Copy link
Member

@shane-tomlinson shane-tomlinson commented Jul 30, 2014

Use 3d transforms and offload that work to the GPU!

@pdehaan pdehaan added this to the train-20 (Aug 25) milestone Jul 31, 2014
@ckarlof
Copy link
Contributor

@ckarlof ckarlof commented Jul 31, 2014

@zaach says it might need his love, but @johngruen is going to take a look.

@ckarlof ckarlof modified the milestones: train-20 (Aug 25), train-21 (Sep 8) Aug 25, 2014
@ckarlof ckarlof modified the milestones: train-21 (Sep 8), train-22 (Sep 22) Sep 4, 2014
@zaach zaach assigned zaach and unassigned johngruen Sep 10, 2014
@ckarlof ckarlof removed this from the train-22 (Sep 22) milestone Sep 22, 2014
@zaach zaach added backlog and removed waffle:ready labels Oct 2, 2014
@edwindotcom
Copy link

@edwindotcom edwindotcom commented Jun 3, 2015

@pdehaan @billmaggs asking QA/Prod if this is a blocker for soft launch?

@pdehaan
Copy link
Contributor

@pdehaan pdehaan commented Jun 3, 2015

I'm not sure of the % distribution between desktop and mobile, but I'd say that ~10-15seconds is probably a blocker.

@billmaggs
Copy link

@billmaggs billmaggs commented Jun 3, 2015

Yes, I would say that's too slow for a reasonable user to stay with the activity so its a blocker.

@johngruen
Copy link
Contributor

@johngruen johngruen commented Jun 3, 2015

@billmaggs @edmoz @pdehaan @zaach: a few things:

  1. the ability to change the avatar appears to be preffed off completely in iOS. I suspect it's preffed off on Android as well. Can we confirm this? Preffing off the feature on mobile would make this non-blocking
  2. datadog isn't segmenting OSes very well RN, but android pings to the settings page appear to be very very low.

TL;DR IDK what 'soft launch' means exactly in this context, but I don't think this needs to be blocking.

Also, @zaach and I will be working together next week and can figure out a way to do this with CSS if we have time.

@pdehaan
Copy link
Contributor

@pdehaan pdehaan commented Jun 3, 2015

Screenshot from my iPhone 4S.

img_1960


I can fire up my Nexus 4/7 and confirm there as well.

@johngruen
Copy link
Contributor

@johngruen johngruen commented Jun 3, 2015

@pdehaan well, f me

@johngruen
Copy link
Contributor

@johngruen johngruen commented Jun 3, 2015

@pdehaan @edmoz: it seems i'm wrong about the current state of avatars on mobile, but it might be a lot simpler to simply use ABLE to disable the feature on mobile than to implement the CSS rotator (considering this bug has languished for a year)

@pdehaan
Copy link
Contributor

@pdehaan pdehaan commented Jun 3, 2015

Here's a screenshot from my Nexus 7. It doesn't look like my Nexus 4 wants to power up today.

screenshot_2015-06-03-13-54-02

@pdehaan
Copy link
Contributor

@pdehaan pdehaan commented Jun 3, 2015

FirefoxOS 2.1.0

2015-06-03-12-59-18

@zaach zaach added this to the train 43 milestone Jul 14, 2015
@eoger
Copy link
Contributor

@eoger eoger commented Jul 17, 2015

Pictures around ~500x500px works pretty well on my Nexus 4 (1 sec rotation).
However, big pictures (like this 5000x2000px yummy ramen photo) still take a lot of time to rotate, because of #1916.

screenshot_2015-07-17-15-46-28

@pdehaan
Copy link
Contributor

@pdehaan pdehaan commented Jul 18, 2015

@eoger does this happen for other types of food, or is it a ramen specific bug?

@TDA
Copy link
Contributor

@TDA TDA commented Jul 18, 2015

@pdehaan That was highly relevant I assume? :P

@eoger
Copy link
Contributor

@eoger eoger commented Jul 18, 2015

@pdehaan I'm not sure, I'll try to investigate with Hot pot next time, see if I can reproduce the bug.

@zaach
Copy link
Contributor Author

@zaach zaach commented Jul 23, 2015

How does this fare given #2776 has landed? Perhaps we can remove the blocker label if performance is within 1-3 seconds.

@zaach zaach modified the milestones: train 44, train 43 Jul 27, 2015
@rfk rfk modified the milestones: avatars-fx41, train 44 Aug 2, 2015
@edwindotcom
Copy link

@edwindotcom edwindotcom commented Aug 3, 2015

per irc - pdehaan is OK to remove blocker tag.

@pdehaan
Copy link
Contributor

@pdehaan pdehaan commented Aug 3, 2015

I tested using latest.dev.lcip.org and a 8MP 2448x3264px image (1.5MB) on my Nexus 4 and the rotation speeds were pretty good (~2.5s).

@ckarlof
Copy link
Contributor

@ckarlof ckarlof commented Aug 12, 2015

De-prioritizing for now and removing from avatars-41 milestone.

@ckarlof ckarlof removed the Fx41 label Aug 12, 2015
@ckarlof ckarlof removed this from the avatars-fx41 milestone Aug 12, 2015
@shane-tomlinson
Copy link
Member

@shane-tomlinson shane-tomlinson commented Oct 12, 2015

@zaach - is the cropper using CSS or JS now? If CSS, can you close this down?

@zaach
Copy link
Contributor Author

@zaach zaach commented Oct 12, 2015

Still JS 😩

@shane-tomlinson
Copy link
Member

@shane-tomlinson shane-tomlinson commented Oct 26, 2015

Performant enough for now. Closing until we hear of more problems.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
10 participants