962439 - transitions between tracks are visually abrupt, should be smoother

Status: RESOLVED DUPLICATE of bug 1126466

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

Description

[Dietrich Ayala (:dietrich)]

Looks like maybe it goes to a full black screen before fading in the next track's album art.

In the case where the album is the same, we should not do anything.

In the case where the album is different, we should do a fancy transition, and not start from black background.

Comment 1

[Dietrich Ayala (:dietrich)]

this is especially hurty on slow devices like tarako.

Comment 2

[Dominic Kuo [:dkuo]]

Dietrich,

this is what we planned to fix for music in 1.5, or I should say the new version of music app. Please let me know if we are going to fix this on tarako, thanks.

Comment 3

[Dietrich Ayala (:dietrich)]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16274

Expected behavior:

* Same album art between tracks: no change at all!

* Different album art, same transition as before

Works great on Tarako, and should be safe for master (though I might need to make a new patch)

Comment 4

[Dietrich Ayala (:dietrich)]

Even better fix would be to take a shortcut earlier without calling getThumbnailURL at all.

Example: maybe make a method for seeing if two fileinfo or metadata objects would have same thumbnail URL. 

metadata.isSameCoverImage(fileInfoA, fileInfoB)

And inside create the string key and compare, without ever hitting IndexedDB. That would save a messaging roundtrip since no IndexedDB interaction required.

That improvement does not have to block this simple fix. It can be made in a separate bug.

Comment 5

[Dominic Kuo [:dkuo]]

Comment on attachment 8376022 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16274

Hey Dietrich, sorry about the late reply, I got many blockers and now I have a chance to see your patch, I think checking the album art is the same or not to reduce the transitions between tracks is a good idea for tarako, also to remove the transition entirely is even better as you mentioned in comment 4, and if we do want to remove the transition not just for tarako, let's discuss with ux before we write some patches, let me know if you need any help, thanks!

Comment 6

[James Ho]

Dietrich, could you provide an ETA ready for review?

Comment 7

[Dietrich Ayala (:dietrich)]

This patch is ready for review.

Dominic can you r+ this patch if you do not have any additional changes?

I agree this is Tarako only. I'll file another bug based on this one for fixing the UX and making better perf on master.

Comment 8

[Dietrich Ayala (:dietrich)]

Filed bug 981653 for fixing on master.

Comment 9

[bhavana bajaj [:bajaj]]

Blocking, lets make sure to test/verify this by QA once this lands.

Comment 10

[bhavana bajaj [:bajaj]]

Setting the assignee to :dietrich as he's helping here.

Comment 11

[Dominic Kuo [:dkuo]]

(In reply to Dietrich Ayala (:dietrich) from comment #7)
> This patch is ready for review.
> 
> Dominic can you r+ this patch if you do not have any additional changes?
> 
> I agree this is Tarako only. I'll file another bug based on this one for
> fixing the UX and making better perf on master.

Hey Dietrich, actually I did reviewed it and commented on github in https://github.com/mozilla-b2g/gaia/pull/16274#discussion-diff-10291377, I think the patch needs some adjustments because it might only solve the case that the tracks use the default 10 album arts, for those tracks that do have covers in the same album, they should still change the album art because blob urls are different from every song files. So I think if we don't want to see the black cover between tracks, probably not to reset the album art to black can fix this.

Let me write a quick patch and see if it works.

Comment 12

[Dominic Kuo [:dkuo]]

Attachment: patch for 1.3t

Dietrich, I have quickly wrote a patch base on my idea in comment 11, would you please test it on tarako and see if it feels better after the patch is applied? after that let's see we need to find someone(maybe you can!) to review it or we need another approach to fix this, thanks!

Comment 13

[Dietrich Ayala (:dietrich)]

Dominic, is that pull request missing some changes?

Regardless, yes you are right that a better check is probably if album+artist are the same!

Comment 14

[Dietrich Ayala (:dietrich)]

This seems like a nice to have.

Doesn't really meet any criteria for blocking: https://wiki.mozilla.org/B2G/Triage#Issues_that_Should_Block

Comment 15

[bhavana bajaj [:bajaj]]

(In reply to Dietrich Ayala (:dietrich) from comment #14)
> This seems like a nice to have.
> 
> Doesn't really meet any criteria for blocking:
> https://wiki.mozilla.org/B2G/Triage#Issues_that_Should_Block

Correct, I think the only reason this was blocked on as this close to being done and seemed like a win keeping comment #1 in mind. Given the timeline and making sure we only have true blockers on the list, good time to re-evaluate and get this out.

Comment 16

[Dominic Kuo [:dkuo]]

(In reply to Dietrich Ayala (:dietrich) from comment #13)
> Dominic, is that pull request missing some changes?
> 
> Regardless, yes you are right that a better check is probably if
> album+artist are the same!

No, I just removed the transition entirely so the user won't see the black screen before the fading in, so that we don't have to worry about the next image is loaded or check album+artist to reduce the number of changing the src.

Comment 17

[Dietrich Ayala (:dietrich)]

Comment on attachment 8389925 [details] [review]
patch for 1.3t

Ok, thanks Dominic. That sounds like a fine resolution for Tarako. We can fix transitions on trunk in the bug I cloned from this one.

You should get r+ on this patch from a module peer still.

Comment 18

[Dietrich Ayala (:dietrich)]

Assigned to Dominic, since his latest patch is the route we're taking in this bug.

Comment 19

[Jim Porter (:squib)]

Fixed in bug 1126466.