Closed Bug 962439 Opened 6 years ago Closed 5 years ago

transitions between tracks are visually abrupt, should be smoother

Categories

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

x86
macOS
defect
Not set

Tracking

(blocking-b2g:-)

RESOLVED DUPLICATE of bug 1126466
blocking-b2g -

People

(Reporter: dietrich, Assigned: dkuo)

References

Details

Attachments

(2 files)

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.
this is especially hurty on slow devices like tarako.
Whiteboard: [tarako]
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.
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)
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 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+
Dietrich, could you provide an ETA ready for review?
Flags: needinfo?(dietrich)
blocking-b2g: --- → 1.3T?
Whiteboard: [tarako]
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)
Blocks: 981653
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+
Assignee: nobody → dietrich
Setting the assignee to :dietrich as he's helping here.
(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)
Attached file 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)
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)
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? → -
(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)
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+
Assigned to Dominic, since his latest patch is the route we're taking in this bug.
Assignee: dietrich → dkuo
Fixed in bug 1126466.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1126466
You need to log in before you can comment on or make changes to this bug.