1093214 - [Gallery] Refactor/Modularize carousel view

Status: RESOLVED INVALID

(Firefox OS Graveyard :: Gaia::Gallery, defect)

Description

[Justin D'Arcangelo [:justindarc]]

As part of the continued Gallery refactoring, the "carousel view" needs to be modularized to be reusable with multiple data sources. In the upcoming v2.2 refresh, the carousel view needs to be able to be populated with album photos/videos in addition to the current "all" carousel. Therefore, the view logic needs to be easily separable from the backing data.

Comment 1

[Justin D'Arcangelo [:justindarc]]

Going to block Bug 1064600 since I will base this patch on top of the initial refactoring work.

Comment 2

[Justin D'Arcangelo [:justindarc]]

Attachment: WIP pull-request (master)

This is VERY early, but wanted to get feedback before I go much further. I haven't looked yet at abstracting a single "controller" object for the "gallery-carousel-view", so I've simply hooked in to the pre-existing mechanisms for switching to the fullscreen view (e.g. when a thumbnail is clicked, previously `showFile(index);` was called -- as of right now, I'm simply invoking `setItems(files)` on the gallery-carousel-view web component... i still need to implement a `showItem(index)` to switch to a specific image in the carousel, but you'll get the idea). So, really the main focal point here is js/components/gallery-carousel-view.js. This is a Gallery-specific custom element that combines a <gaia-header>, <gaia-carousel> and a <footer> into a single, reusable component. To test, I've renamed the old #fullscreen-view in index.html and added a new <gallery-carousel-view id="fullscreen-view"> that will eventually take its place.

Again, this is VERY rough, but I was excited at the mere fact that I was able to populate the carousel and present this view as a custom web component. If this looks like we're heading down the right path, I'll keep going. If we have concerns, we can discuss those as well. Thanks!

Comment 3

[Diego Marcos [:dmarcos]]

Comment on attachment 8520914 [details] [diff] [review]
WIP pull-request (master)

I love those nested web components. I like how the code is laid out and how swaps the old carrousel with the new one with minimal impact on the rest of the code base

Comment 4

[David Flanagan [:djf]]

Comment on attachment 8520914 [details] [diff] [review]
WIP pull-request (master)

Overall, this looks really good. My comments are minor naming and code structuring stuff, mostly.

I am concerned about the integration of event handling between mediaframe and carousel, and I think there might be problems there... Repeating one of my comments from github:

note that you won't get perfect event handling this way. If the user is moving their finger quickly then there might be a touchmove that moves 50px. Maybe 10px of that is used for panning, and 40px is passed to the callback. At this point, the carousel should move by 40px. But with your system it won't move at all.

Maybe the user will never actually notice, but I think they might. If the user's finger is over a particular spot in the photo, then the photo should move with the finger so that the finger is always over the same spot (or close to it).

Comment 5

[Justin D'Arcangelo [:justindarc]]

(In reply to David Flanagan [:djf] from comment #4)
> Comment on attachment 8520914 [details] [diff] [review]
> WIP pull-request (master)
> 
> Overall, this looks really good. My comments are minor naming and code
> structuring stuff, mostly.
> 
> I am concerned about the integration of event handling between mediaframe
> and carousel, and I think there might be problems there... Repeating one of
> my comments from github:
> 
> note that you won't get perfect event handling this way. If the user is
> moving their finger quickly then there might be a touchmove that moves 50px.
> Maybe 10px of that is used for panning, and 40px is passed to the callback.
> At this point, the carousel should move by 40px. But with your system it
> won't move at all.
> 
> Maybe the user will never actually notice, but I think they might. If the
> user's finger is over a particular spot in the photo, then the photo should
> move with the finger so that the finger is always over the same spot (or
> close to it).

Thanks for the feedback! As far as the panning/scrolling bleed-over event handling issue goes, this is something I considered when trying to glue together MediaFrame with <gaia-carousel>. It would be fairly easy (I think) to implement some additional API on <gaia-carousel> to allow us to manually "drive" the overpan into the carousel. However, I actually prefer the behavior of the implementation I chose for a specific reason (which is debatable.. we can probably get UX input here). So, if you're zoomed in on an image and you pan around, when you reach the edge, the panning stops and the carousel doesn't "inherit" the overpan and begin transitioning to the next image. The reason I prefer this behavior is so that you don't accidentally pan too quickly and cause the carousel to flip over to the next image. When you're zoomed into an image and you're panning around, its very easy to get lost navigating your place in the image and I've found that it can be too easy to accidentally flip into the next image. When that happens, you completely lose your context even if you flip *back* to the image you were previously on because you're now zoomed all the way out. In my implementation, if you pan all the way to the edge of a zoomed-in image to where it stops panning, then release your finger and proceed to attempt to scroll past the edge to go to the next image, it works. I know this is hard to explain so my STR would be:

1. Zoom far in on an image
2. Pan the image by sliding your finger from right-to-left until you reach the edge
3. Release your finger, and now attempt to slide the carousel to the "next" image by swiping from right-to-left again
4. The carousel should interpret that final "right-to-left" swipe as an intent to go to the next image

Again, this is probably very subjective, so one person's opinion on the UX here will likely differ from the next. If you think we need to investigate further into what the "correct" behavior here is, then lets get UX's input.

Thanks again for the feedback! I'll ping you again once I revise the PR and go a little further with it.

Comment 6

[David Flanagan [:djf]]

Comment on attachment 8520914 [details] [diff] [review]
WIP pull-request (master)

I've now taken a look at the other part of this patch where the gaia-carousel is imported, and am concerned enough about that that I've changed my feedback+ to feedback-.  

My concerns are:

1) I think our build system will end up putting the entire component along with all of its tests and example application into Gallery's application.zip file.  If I'm right about that, then we need to either not use bower (and just manually import the component file that we need) or fix the build system

2) The component relies on horizontal scrolling of a container element that is wider than the screen. In the past wide elements like this would screw up the rendering pipeline and make everything slow. So I am very worried (but have not tested yet) that this basic implementation choice you made for the carousel component will make it impossible to get 60fps transitions between images.

So before we go any further, we need to test frame rates for the transitions between photos both with and without this patch.

Comment 7

[Justin D'Arcangelo [:justindarc]]

(In reply to David Flanagan [:djf] from comment #6)
> Comment on attachment 8520914 [details] [diff] [review]
> WIP pull-request (master)
> 
> I've now taken a look at the other part of this patch where the
> gaia-carousel is imported, and am concerned enough about that that I've
> changed my feedback+ to feedback-.  
> 
> My concerns are:
> 
> 1) I think our build system will end up putting the entire component along
> with all of its tests and example application into Gallery's application.zip
> file.  If I'm right about that, then we need to either not use bower (and
> just manually import the component file that we need) or fix the build system
> 

That's fine. If it makes you happy to manually copy/paste dependencies into the gallery folder, we can do that.

> 2) The component relies on horizontal scrolling of a container element that
> is wider than the screen. In the past wide elements like this would screw up
> the rendering pipeline and make everything slow. So I am very worried (but
> have not tested yet) that this basic implementation choice you made for the
> carousel component will make it impossible to get 60fps transitions between
> images.
> 
> So before we go any further, we need to test frame rates for the transitions
> between photos both with and without this patch.

I have already spent weeks optimizing the performance and framerate of gaia-carousel. I tested the scrolling 4 ways:

- CSS `left`/`top` properties
- CSS transform using `translate(x, y)`
- CSS transform using `translate3d(x, y, 0)`
- `scrollLeft`/`scrollTop` properties

Much to my surprise, the scroll method ended up with the best framerate. My request for feedback here was *not* for <gaia-carousel>, it was for the code pertaining to Gallery and using web components as a module solution. I understand you may have concerns with <gaia-carousel>, however this code has been thoroughly through several rounds of feedback and review with Wilson who is spending all of his time working with web components, so I trust his expertise when it comes to understanding the best practices for things like templating/styling/event handlers/etc.

The bottom line is, the implementation details of <gaia-carousel> has very little to do with the purpose of this PR. I was hoping after your first round of feedback, where you left several constructive comments, to go back to the PR, adjust as necessary and continue. If you have issues with implementation specifics of <gaia-carousel>, you can file bugs against it as you see fit, but that should not obstruct us from moving forward with refactoring.

Comment 8

[David Flanagan [:djf]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #7)
> (In reply to David Flanagan [:djf] from comment #6)
> > Comment on attachment 8520914 [details] [diff] [review]
> > WIP pull-request (master)
> > 
> > I've now taken a look at the other part of this patch where the
> > gaia-carousel is imported, and am concerned enough about that that I've
> > changed my feedback+ to feedback-.  
> > 
> > My concerns are:
> > 
> > 1) I think our build system will end up putting the entire component along
> > with all of its tests and example application into Gallery's application.zip
> > file.  If I'm right about that, then we need to either not use bower (and
> > just manually import the component file that we need) or fix the build system
> > 
> 
> That's fine. If it makes you happy to manually copy/paste dependencies into
> the gallery folder, we can do that.

Its not that it 'makes me happy', it that this patch changes the size of the application.zip that gets pushed to the phone from .5mb to 12mb. I think it should be clear that we can't land that.

> 
> > 2) The component relies on horizontal scrolling of a container element that
> > is wider than the screen. In the past wide elements like this would screw up
> > the rendering pipeline and make everything slow. So I am very worried (but
> > have not tested yet) that this basic implementation choice you made for the
> > carousel component will make it impossible to get 60fps transitions between
> > images.
> > 
> > So before we go any further, we need to test frame rates for the transitions
> > between photos both with and without this patch.
> 
> I have already spent weeks optimizing the performance and framerate of
> gaia-carousel. I tested the scrolling 4 ways:
> 
> - CSS `left`/`top` properties
> - CSS transform using `translate(x, y)`
> - CSS transform using `translate3d(x, y, 0)`
> - `scrollLeft`/`scrollTop` properties
> 
> Much to my surprise, the scroll method ended up with the best framerate. 

I was just testing this as well before I saw this comment, and I'm impressed with the framerate it achieves. I guess scrolling must be well optimized in gecko, though I'm surprised that it works for sideways scrolling.

I'm glad you tested this carefully yourself, and I apologize for leaving that comment without taking the time to try it out myself. I'm still nervous, though, because in the past there were serious problems with using containers wider than the screen. Perhaps the issue was only with transform:translate and not with scrolling, or perhaps the issues have long since been resolved.

My
> request for feedback here was *not* for <gaia-carousel>, it was for the code
> pertaining to Gallery and using web components as a module solution. 

But it would be silly to think that I'd approve landing this patch without even looking at the component. Just because it has been reviewed and landed in the gaia components repo doesn't mean it gets a free pass to land in the Gallery app without being examined for suitability. My concerns about scrolling may have been misplaced, but I've identified a few other issues (see github) that will have to be addressed if you're going to use this component without losing features of Gallery.

I
> understand you may have concerns with <gaia-carousel>, however this code has
> been thoroughly through several rounds of feedback and review with Wilson
> who is spending all of his time working with web components, so I trust his
> expertise when it comes to understanding the best practices for things like
> templating/styling/event handlers/etc.

I trust his expertise in those things as well. But I have expertise to offer as well. Such as the knowledge that animating with transform:translate() does not (or did not) work well when you have a really wide container like you do here. The existing gallery uses three independently positioned elements for that reason and this was a major headache in the past.

Sideways scrolling is not an approach I ever tried, and it may well be the best one. I'm particularly impresssed by the overdraw number: yours is usually right at 100, and mine is 200+.  Both of our approaches get frame rates in the 50s when just moving a finger left and right.  I think my approach might have a better frame rate for the transition that happens after the user lifts their finger, but it is harder to measure that one because the numbers change too quickly.  (Yours has to use a JS animation while mine can use a CSS transition, and I would expect that to make a difference.)

To be satisfied with this new horizontal scrolling approach, I'd want to talk to someone in graphics to see if they think it will typically be an optimized in gecko. And I'd like to test (if you haven't already) on a lower-spec device like unagi. I don't want to land this only to find out later that it has good performance on some devices and bad on others.

Other issues are noted in my github comments. They include the need for some kind of event that will allow the Gallery to free up decoded images in media frames that will not be used anymore, and the issue about making transition speed dependent on distance to travel and the velocity of the swipe. Now that I've tried the patch out I also notice that there are no margins between the photos. I don't know if that is a gaia-carousel issue or a gallery-carousel-view issue, but I imagine that it will need to be fixed or the patch won't pass ui-review.

> 
> The bottom line is, the implementation details of <gaia-carousel> has very
> little to do with the purpose of this PR. 

Those details may not have anything to do with the *purpose* of the PR, but they are *part of* the PR and they matter.

I was hoping after your first
> round of feedback, where you left several constructive comments, to go back
> to the PR, adjust as necessary and continue. If you have issues with
> implementation specifics of <gaia-carousel>, you can file bugs against it as
> you see fit, but that should not obstruct us from moving forward with
> refactoring.

We can file separate bugs, if you want, but they'll block this bug. We can't land any piece of the refactoring that causes regressions in features (like missing margins between photos) or performance (like the memory leaks I'm worried about by not clearing the MediaFrame objects properly).

Comment 9

[Justin D'Arcangelo [:justindarc]]

My whole point was that the implementation specifics of gaia-carousel are not relevant here. The purpose of this PR is to establish a pattern for refactoring the app so we can move forward. If you have issues with implementation specifics of gaia-carousel then you can file bugs for that component. Regardless of the changes in future iterations of gaia-carousel, they should not have any profound impact on the patterns we choose to follow in Gallery. Just the same, future changes to gaia-header and all the other gaia-* web components should not regularly introduce API-breaking changes that affect the apps that use them. Likewise, if you suspected potential performance issues in a platform-native element such as <img> or <video>, you would likely open up platform bugs accordingly. We need a clear yes or no to the approach taken by this PR to use or not to use document.registerElement() as a viable means for encapsulating "view" logic. Blocking on *possible* performance issues of a dependency when no perceivable performance penalty exists and due diligence was done to ensure the component was optimized before use is not constructive in helping establish the necessary design patterns for refactoring. I was hoping by requesting feedback at a much earlier-than-usual stage that we could get a clear answer here and not head off in ten directions at once. Time is rapidly running out for this refactor and it needs to happen before we can begin work on v2.2 features. Additionally, any proposed work you'd like to see on the gaia-carousel component could easily happen in parallel while the refactoring continues.

Comment 10

[Diego Marcos [:dmarcos]]

> To be satisfied with this new horizontal scrolling approach, I'd want to
> talk to someone in graphics to see if they think it will typically be an
> optimized in gecko. And I'd like to test (if you haven't already) on a
> lower-spec device like unagi. I don't want to land this only to find out
> later that it has good performance on some devices and bad on others.
> 

What is the ETA for you to test on a low end device and get feedback from graphics?

Comment 11

[Diego Marcos [:dmarcos]]

> We can file separate bugs, if you want, but they'll block this bug. We can't
> land any piece of the refactoring that causes regressions in features (like
> missing margins between photos) or performance (like the memory leaks I'm
> worried about by not clearing the MediaFrame objects properly).

We should not hold this back for too long. The cost of losing momentum is greater than having to deal with some defects later. We're not being sloppy by any means, the carrousel implementation has been discussed thoroughly, reviewed and tested. Bugs must not hit the user but should not be stigmatized either during the development phase. Dealing with those issues is the purpose of the stabilization sprints we introduced in our process. We can block 2.2 on those follow up bugs if necessary.

Comment 12

[David Flanagan [:djf]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #9)
> My whole point was that the implementation specifics of gaia-carousel are
> not relevant here. The purpose of this PR is to establish a pattern for
> refactoring the app so we can move forward. If you have issues with
> implementation specifics of gaia-carousel then you can file bugs for that
> component. 

I like the pattern you're establishing, and indicated that in comment #4.

But the fact that gaia-carousel does not replicate all of the functionality of the frames.js file it will be replacing is relevant here.

 We need a clear yes or
> no to the approach taken by this PR to use or not to use
> document.registerElement() as a viable means for encapsulating "view" logic.
> Blocking on *possible* performance issues of a dependency when no
> perceivable performance penalty exists 

I have never said that we're blocking on frame rate. I've said I have serious concerns about it. And I've said that in my own followup testing it looks good. And I've said that I'd like to consult someone who knows more about gecko's graphics than we do.

Meanwhile, things that *will* block this patch from landing include:

1) We need to either fix the build system or not use bower so that application.zip doesn't contain lots of unnecessary stuff

2) gaia-carousel needs a way to notify the client that its mediaframe objects won't be needed anymore so that it can release their image resources

3) gaia-carousel probably needs distance and velocity-sensitive transitions, or we need UX to sign off on dropping that feature.

4) gaia-carousel needs to be able to draw margins between its elements. We still want photos to take up the full width of the screen, but need to be able to see a black line between two photos when scrolling them.


(In reply to Diego Marcos [:dmarcos] from comment #10)

> What is the ETA for you to test on a low end device and get feedback from
> graphics?

I don't know, but less time than it will take to fix the things above.


(In reply to Diego Marcos [:dmarcos] from comment #11)

> We should not hold this back for too long. The cost of losing momentum is
> greater than having to deal with some defects later. 

No. We're not going to land refactoring patches that cause known regressions. The stablilization period is for fixing unexpected bugs, and I'm sure we'll have plenty of those. We can't knowingly land bugs now and plan to fix them later.

We're not being sloppy
> by any means, the carrousel implementation has been discussed thoroughly,
> reviewed and tested. 

Yes, I'm confident it was thoroughly discussed and had been reviewd. But the author of the code it is intended to replace was not included in those discussions and reviews, which seems like a mistake. I've looked at it now and I've identified some missing features that it will need in order to replace the existing code. Furthermore, I found enough minor problems with the code (like the if/else statements that had the same code in each path, presumably the result of a mechanical change at some point) that I suspect that previous reviews were not actually thorough.

Comment 13

[David Flanagan [:djf]]

I've done some more testing of frame rate with the old (frames.js) and new (gaia-carousel) code. I tested both flicking through my set of photos as quickly as possible and just smoothly moving my finger left and right to pan back and forth between two photos without lifting my finger.  I tried this on Flame, Hamachi and Nexus 4, but recorded the most specific numbers on Flame.

Flame (tried 319mb and 1024mb with the same results)

frames.js:
  flicking: 45-60fps
  panning: 60fps

gaia-carousel:
  flicking: 25-35 fps
  panning: 50-55fps

Also on the flame, when using frames.js I would sometimes see a red square in the upper right corner in addition to the frame rate in the upper left. I'm not sure what that means. It never appeared for gaia-carousel.  The overdraw number (the 3rd in the frame rate display) was always lower with gaia-carousel than it was with frames.js.

On Nexus 4, flicking with frames.js gave me frame rates in the 30s, and flicking with gaia-carousel gave me frame rates in the 20s.  Panning was slower than I expected with both implementations. I'm guessing that the nexus 4 graphics pipeline isn't well optimized.

On my old Hamachi, with frames.js I'd get a frame rate in the 30s or 40s when flicking and a frame rate of 60 when panning, but after flicking around for a bit, it would often start using HWC and stop displaying frame rates at all. My understanding is that using HWC is the best possible thing, so that is a good result.

With gaia-carousel on the hamachi, I got frame rates in the 20s and never used HWC. I'm not a good judge of "buttery smooth" interactions, but on hamachi, I think I could feel a difference between the two implementations. 

I wonder if HWC on Hamachi is a function of the smaller screen size or has something to do with ICS versus KK?  And I also wonder what the red square means that I'm seeing on flame.

Comment 14

[David Flanagan [:djf]]

I asked in the #gfx channel and learned from Benoit that the red square means scrolling is happening but is not being handled by AZPC. I'm not sure why I'm seeing it with the frames.js implementation since there is no scrolling at all going on there...

Comment 15

[David Flanagan [:djf]]

Okay this is pretty amazing... If I zoom in and out with pinches and double taps, and pan up and down in a photo, I can sometimes get the red square to come on when I'm using the gaia-carousel implementation. And when the red square is on, flicks and pans go at full 60fps on the flame. So if there is some trick to forcing AZPC off for the carousel, that could be a big performance win. (With the frames.js implementation, the frame rate seems faster when the red square is on, too, though it comes and goes much more frequently in that case.)

Turning AZPC off in the Developer menu does not speed things up, however.

Comment 16

[David Flanagan [:djf]]

Benoit,

We need some advice from you (or anyone else on the graphics team who could help with this).

The background here is that we are trying to refactor the "carousel" code in the Gallery app that lets the user swipe left and right to view their photos.

The existing implementation is in the frames.js file in the Gallery app. It uses three absolutely positioned screen size elements to hold the photos. Normally one is on screen, one is off to the left and one is off to the right. As the user moves their finger back and forth we use CSS transforms to move the photos back and forth.  When the user lifts their finger, there is a CSS transition that completes the animation of the photo sliding into place.

The new implementation is the gaia-carousel web component. It uses three elements to hold three photos, but places these elements inside a container element and uses the scrollLeft property of the container to scroll horizontally and move them around. When the user lifts their finger, we can't do a CSS animation, so we animate with JavaScript and requestAnimationFrame().

I'm nervous about the new implementation because I remember long ago I tried an implementation that put the three photo elements into a single container and then uses CSS transforms to move that entire container back and forth. This had a low frame rate. Chris Jones (I think) explained at the time that my element was too wide for the animation to be efficient. This new implementation is different in that it uses a scrollable container for the content, but before we switch to it, I'd like an expert opinion.

The new approach is simpler, and we'd like to use it if we can be sure of getting good performance. So the primary question here is: will be get good frame rates with this approach of putting three screen-sized photos into a horizontally scrollable container and moving them back and forth by setting the scrollLeft property, including doing JS-based animations on scrollLeft?

I've done some testing myself and reported the results above in comment #13 and comment #15. Based on those results, I also have some secondary questions:

- are there hardware dependencies here? On the Hamachi it looked like I got HWC with the old approach but not with the new approach. But I didn't get HWC on Flame or Nexus 4. (Because of bigger screens maybe?)

- In comment #15 it looked like the horizontal scrolling approach was much faster when the red square was lit up. Is there something we can do with HTML or CSS to tell gecko to not us AZPC for the carousel?

Comment 17

[Justin D'Arcangelo [:justindarc]]

Attachment: WIP pull-request (master)

Added more commits to PR to address some concerns. Namely, HWC is re-enabled on gaia-carousel and the ability to specify item-padding was added to gaia-carousel. Also some code in gallery-carousel-view.js was cleaned up and comments were added to make it clearer.

Comment 18

[Diego Marcos [:dmarcos]]

> We can file separate bugs, if you want, but they'll block this bug. We can't
> land any piece of the refactoring that causes regressions in features (like
> missing margins between photos) 

This issue has been addressed in the PR as Justin mentioned

or performance (like the memory leaks I'm
> worried about by not clearing the MediaFrame objects properly).

Can you be more specific? The carousel creates only 3 Media Frames that get recycled.

https://github.com/justindarc/gaia/commit/d73669948602435198f394aa79423f15ee9428b4#diff-5e676579c1717f8368950547f8189284R181

Comment 19

[Diego Marcos [:dmarcos]]

Comment on attachment 8522560 [details] [review]
WIP pull-request (master)

Looks better than ever!

Comment 20

[David Flanagan [:djf]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #17)
> HWC is re-enabled
> on gaia-carousel 

Tell me more about this. What did you change? I'm not seeing any change in the frame rate numbers when I try the patch. But the overdraw numbers have now gone up to 200+. And since I'm seeing the numbers at all, this obviously isn't hitting the HWC.

Comment 21

[Justin D'Arcangelo [:justindarc]]

(In reply to David Flanagan [:djf] from comment #20)
> (In reply to Justin D'Arcangelo [:justindarc] from comment #17)
> > HWC is re-enabled
> > on gaia-carousel 
> 
> Tell me more about this. What did you change? I'm not seeing any change in
> the frame rate numbers when I try the patch. But the overdraw numbers have
> now gone up to 200+. And since I'm seeing the numbers at all, this obviously
> isn't hitting the HWC.

https://github.com/gaia-components/gaia-carousel/commit/d17f8b2829e3ac249bafd5c7abab4c8f955ecc17#diff-1bcc375adbc0fed0ffdfedefb464d06dL95

I'm hitting HWC on both Flame and Hamachi now. My overdraw number is ~160 if I disable HWC (otherwise I can't see the FPS counter to know what it is):

http://i.imgur.com/KT4yGRq.png

Also, I'm not sure about the accuracy of judging the FPS number on the "flick" animation because the animation is only 100ms and presumably to get a good reading you'd have to flick repeatedly through a series of images. However, as soon as you `touchstart` in the middle of an existing "flick" animation, the animation is interrupted immediately to allow the panning to take over again. If you increase `SNAP_ANIMATION_DURATION` in gaia-carousel.js to something much higher (>1000ms), you can see the animation play out longer and the FPS counter almost always reaches 60.

Comment 22

[David Flanagan [:djf]]

(In reply to Diego Marcos [:dmarcos] from comment #18)
> > We can file separate bugs, if you want, but they'll block this bug. We can't
> > land any piece of the refactoring that causes regressions in features (like
> > missing margins between photos) 
> 
> This issue has been addressed in the PR as Justin mentioned
> 
> or performance (like the memory leaks I'm
> > worried about by not clearing the MediaFrame objects properly).
> 
> Can you be more specific? The carousel creates only 3 Media Frames that get
> recycled.

I think Justin has addressed this in the latest commit by calling MediaFrame.clear() at the appropriate time.

Comment 23

[David Flanagan [:djf]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #21)
> (In reply to David Flanagan [:djf] from comment #20)
> > (In reply to Justin D'Arcangelo [:justindarc] from comment #17)
> > > HWC is re-enabled
> > > on gaia-carousel 
> > 
> > Tell me more about this. What did you change? I'm not seeing any change in
> > the frame rate numbers when I try the patch. But the overdraw numbers have
> > now gone up to 200+. And since I'm seeing the numbers at all, this obviously
> > isn't hitting the HWC.
> 
> https://github.com/gaia-components/gaia-carousel/commit/
> d17f8b2829e3ac249bafd5c7abab4c8f955ecc17#diff-
> 1bcc375adbc0fed0ffdfedefb464d06dL95

That's what I figured. Why does that even do anything, given that nothing in your code modifies a transform? Do you understand what is going on?

> 
> I'm hitting HWC on both Flame and Hamachi now. My overdraw number is ~160 if
> I disable HWC (otherwise I can't see the FPS counter to know what it is):
> 

I'm using flame-kk nightly build from two days ago, shallow-flashed on top of v188. What is different about your hardware that you're seeing HWC and I'm not, I wonder?
> http://i.imgur.com/KT4yGRq.png
> 
> Also, I'm not sure about the accuracy of judging the FPS number on the
> "flick" animation because the animation is only 100ms and presumably to get
> a good reading you'd have to flick repeatedly through a series of images.
> However, as soon as you `touchstart` in the middle of an existing "flick"
> animation, the animation is interrupted immediately to allow the panning to
> take over again. If you increase `SNAP_ANIMATION_DURATION` in
> gaia-carousel.js to something much higher (>1000ms), you can see the
> animation play out longer and the FPS counter almost always reaches 60.

Comment 24

[Justin D'Arcangelo [:justindarc]]

will-transform puts the element on its own layer. I *think* this should help prevent a single large layer (the scrolling layer) from being created to break things up a bit. I'm assuming that's the case since HWC is now enabled.

I'm running latest nightly master on v188. Maybe the size of the images you have in MediaFrame?

Comment 25

[David Flanagan [:djf]]

Comment on attachment 8522560 [details] [review]
WIP pull-request (master)

Feedback+: this fixes my concern about not calling clear() on unused media frames and it adds in margins between the photos.

I can't verify the FPS improvement on my Flame.

A few nits left on github.

Things that I think still need to be fixed before this can land:

- unnecessary stuff in application.zip

- no distance and velocity sensitive transitions

- better sharing of gestures motion between panning within a zoomed image and transitioning between images. Right now a zoomed photo "sticks" when you reach its edge and keep moving your finger.  And if you double-tap to zoom and then pan slowly to the left or right you'll see a jump where the carousel starts moving images and then stops

Comment 26

[Justin D'Arcangelo [:justindarc]]

(In reply to David Flanagan [:djf] from comment #25)
> Comment on attachment 8522560 [details] [review]
> WIP pull-request (master)
> 
> Feedback+: this fixes my concern about not calling clear() on unused media
> frames and it adds in margins between the photos.
> 
> I can't verify the FPS improvement on my Flame.
> 
> A few nits left on github.
> 
> Things that I think still need to be fixed before this can land:
> 
> - unnecessary stuff in application.zip
> 
> - no distance and velocity sensitive transitions
> 
> - better sharing of gestures motion between panning within a zoomed image
> and transitioning between images. Right now a zoomed photo "sticks" when you
> reach its edge and keep moving your finger.  And if you double-tap to zoom
> and then pan slowly to the left or right you'll see a jump where the
> carousel starts moving images and then stops

Thanks for giving us the go-ahead to continue. I will wait and hear from GFX to see if we need any follow-up bugs for optimizing gaia-carousel. However, in my testing during the design of the component, the native scrolling yielded the highest FPS (maybe an anomaly with web components). Regardless, those specifics shouldn't affect this patch. Also, I can follow-up with adding the necessary formula for handling velocity and distance tweaks.

As for lightening the size of the app bundle from the inclusion of Bower components, we should be able to strip out the unnecessary parts (tests, examples) in the build script. I can look around at the other Gaia apps to see how we've dealt with this before.

I'd also like to speak to UX about expectations for handling the overpan/zoom scenario. If they have other ideas they'd like to see that differ from Gallery's existing behaviors, I'd rather take that into consideration now rather than trying to duplicate existing behaviors verbatim.

I'll let you know when I'm ready for round 2 for feedback.

Comment 27

[David Flanagan [:djf]]

Justin and I noticed that with his latest patch, landscape mode photos animate on HWC but portrait mode photos do not. (That's why he was seeing one thing and I was seeing something else). That's if we hold the phone in portrait. But if I turn the phone to landscape, then it is the portrait mode photos that can animate on the HWC and the landscape mode photos that don't.

So when the image to be displayed is smaller it can go on HWC and when it is bigger it cannot. 

I don't know if this has something to do with the size the image is being displayed at, or the size the image is being decoded at. I tried a bunch of screenshots, where the images are screen-sized and can't be downsampled with #moz-samplesize, and I got the same results: no HWC in portrait, but we get it if the phone is rotated and the images to be displayed are smaller. So this is probably not related to decode size but to display size.

Comment 28

[David Flanagan [:djf]]

Justin: if you can't figure out a way to get us to use HWC even for the bigger images, then we probably need to optimize for good framerate when we can't use the HWC.

Comment 29

[Benoit Girard (:BenWa)]

We discussed this on IRC:
- Short term if you're having slow down with the current design we should get some profiles. We can run at 60 FPS with rAF so we can fix the problems we're hitting.
- In the long run we want to use APZ and smooth scrolling.
- Implementing proper APZ would need a bit of pluming to enable the browser style zooming within the gallery but the code is there.
- We discussed ideal time to do decode and loading thumbnails more efficiently.

Comment 30

[Justin D'Arcangelo [:justindarc]]

Closing this as invalid since this never happened and any future refactoring will be using the NGA.