transitions between tracks are visually abrupt, should be smoother

RESOLVED DUPLICATE of bug 1126466

Status

RESOLVED DUPLICATE of bug 1126466
5 years ago
3 years ago

People

(Reporter: dietrich, Assigned: dkuo)

Tracking

unspecified
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:-)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Blocks: 949551
(Reporter)

Comment 1

5 years ago
this is especially hurty on slow devices like tarako.
Whiteboard: [tarako]
(Assignee)

Comment 2

5 years ago
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.
(Reporter)

Comment 3

5 years ago
Created attachment 8376022 [details] [review]
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)
Attachment #8376022 - Flags: feedback?(dkuo)
(Reporter)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
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!
Attachment #8376022 - Flags: feedback?(dkuo) → feedback+

Comment 6

5 years ago
Dietrich, could you provide an ETA ready for review?
Flags: needinfo?(dietrich)
blocking-b2g: --- → 1.3T?
Whiteboard: [tarako]
(Reporter)

Comment 7

5 years ago
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.
Flags: needinfo?(dietrich) → needinfo?(dkuo)
(Reporter)

Updated

5 years ago
Blocks: 981653
(Reporter)

Comment 8

5 years ago
Filed bug 981653 for fixing on master.
Blocking, lets make sure to test/verify this by QA once this lands.
blocking-b2g: 1.3T? → 1.3T+

Updated

5 years ago
Assignee: nobody → dietrich
Setting the assignee to :dietrich as he's helping here.
(Assignee)

Comment 11

5 years ago
(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.
Flags: needinfo?(dkuo)
(Assignee)

Comment 12

5 years ago
Created attachment 8389925 [details] [review]
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!
Attachment #8389925 - Flags: feedback?(dietrich)
(Reporter)

Comment 13

5 years ago
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!
Flags: needinfo?(dkuo)
(Reporter)

Comment 14

5 years ago
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
blocking-b2g: 1.3T+ → 1.3T?
(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.
blocking-b2g: 1.3T? → -
(Assignee)

Comment 16

5 years ago
(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.
Flags: needinfo?(dkuo)
(Reporter)

Comment 17

5 years ago
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.
Attachment #8389925 - Flags: feedback?(dietrich) → feedback+
(Reporter)

Comment 18

5 years ago
Assigned to Dominic, since his latest patch is the route we're taking in this bug.
Assignee: dietrich → dkuo

Comment 19

3 years ago
Fixed in bug 1126466.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1126466
You need to log in before you can comment on or make changes to this bug.