Bug 1264901 (fennec-media-control)

Implement the remote media-control front-end

RESOLVED FIXED in Firefox 49

Status

()

Firefox for Android
General
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: scottwu, Assigned: sebastian)

Tracking

(Depends on: 5 bugs, Blocks: 1 bug)

unspecified
Firefox 50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed, firefox50 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
As a continuation of Bug 1240423, the front-end UI for remote media-control is implemented. We can control media playback from the drop-down notification and lockscreen.

It should display a play/pause button along with the page title, URL, and favicon of the page currently playing the media. If the user stops the media or when playback ends, the remote-control should be disappear.
(Reporter)

Updated

2 years ago
Assignee: nobody → scwwu
(Reporter)

Comment 1

2 years ago
Created attachment 8741711 [details]
MozReview Request: Bug 1264901 - implement the remote media-control front-end

Review commit: https://reviewboard.mozilla.org/r/46703/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46703/
(Reporter)

Comment 2

2 years ago
Comment on attachment 8741711 [details]
MozReview Request: Bug 1264901 - implement the remote media-control front-end

Hello Margaret :)

This is Scott from Taipei. I have been working with Alastor to finish the media-control front-end. I ran into an issue with the notification view, and wonder if you know how to get tackle it?

In compact view the remote-control looks just fine, but in expanded big view the icon becomes pixelated. So I wonder if there's a way to disable the expanded view?

I'm very new to Android, so apologies in advance for any noob questions I may have for you in the future :P

Thanks!
Attachment #8741711 - Flags: feedback?(margaret.leibovic)
(Reporter)

Comment 3

2 years ago
Created attachment 8741725 [details]
Notification expanded big view with pixelated favicon

Comment 4

2 years ago
(In reply to Scott Wu [:scottwu] from comment #2)
> Comment on attachment 8741711 [details]
> MozReview Request: Bug 1264901 - implement the remote media-control front-end
> 
> Hello Margaret :)
> 
> This is Scott from Taipei. I have been working with Alastor to finish the
> media-control front-end. I ran into an issue with the notification view, and
> wonder if you know how to get tackle it?
> 
> In compact view the remote-control looks just fine, but in expanded big view
> the icon becomes pixelated. So I wonder if there's a way to disable the
> expanded view?

I'm not immediately sure of an answer here. Chenxia has spent time working on our notifications, I'm going to redirect this question to her.

> I'm very new to Android, so apologies in advance for any noob questions I
> may have for you in the future :P

No worries, keep the questions coming! Also feel free to ask questions in #mobile on irc.mozilla.org, that's where the Android team hangs out (mostly in North America/Europe time zones).
Flags: needinfo?(liuche)

Comment 5

2 years ago
https://reviewboard.mozilla.org/r/46703/#review43805

::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java:195
(Diff revision 1)
> -    private String getActionTittle(String action) {
> +    private Bitmap getFavicon() {
> +        return mTab == null ? Favicons.defaultFavicon : mTab.getFavicon();
> +    }
> +
> +    private String getTitle() {
> +        return mTab == null ? "Media Title" : mTab.getTitle();

You'll need to localize this string by adding an entity to strings.xml.in and android_strings.dtd. You can search for other entity names for examples of how to do this.

Updated

2 years ago
Attachment #8741711 - Flags: feedback?(margaret.leibovic) → feedback?(liuche)
(Assignee)

Comment 6

2 years ago
(In reply to Scott Wu [:scottwu] from comment #2)
> In compact view the remote-control looks just fine, but in expanded big view
> the icon becomes pixelated. So I wonder if there's a way to disable the
> expanded view?

In your code you are setting the notification style:
> .setStyle(getMediaStyle())

This style comes with a bigContentView (the expanded view). You can either not set any style, or if you need it then try to extend it and prevent it from setting the bigContentView:
https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/app/Notification.java#4372
Or go ahead and implement your own style.
(Reporter)

Comment 7

2 years ago
Thanks for the tip Sebastian. Sorry but I'm still unable to piece it together due to my unfamiliarity with how Android works (and Java in general).

I tried not setting any style but the default look doesn't have the play button on the same line as compact view does. Or should I be creating a custom layout for this task? I've spend many hours trying to figure this one out but getting no where.

Wonder if you could help me a little bit more? or I can pass this patch off to someone who's more suitable for finishing this.

Thanks a lot!
Flags: needinfo?(s.kaspari)
(Assignee)

Comment 8

2 years ago
Yeah, if you want to have the same look & feel like the compact version of the media style then you might need to create your own layout/style.

I could help finishing this: Do we have screens how this should look like? Or should I loop in antlam? I tried to import the patch in bug 1240423 and this here but it seems like they do not apply cleanly on fx-team currently.
Flags: needinfo?(s.kaspari)
(Reporter)

Comment 9

2 years ago
Created attachment 8744399 [details]
Notification media control

Thank you Sebastian! Here's a screenshot of the media remote control in notification. It's a default style in compact view. Wonder if we can create a custom layout base on one of these: https://android.googlesource.com/platform/frameworks/base/+/master/core/res/res/layout/
(Reporter)

Updated

2 years ago
Flags: needinfo?(s.kaspari)
(Assignee)

Comment 10

2 years ago
Can you rebase the patches for me? There are several errors when importing the patch in bug 1240423.

Comment 11

2 years ago
Hi, Sebastian,
Now I'm going to finish some dependency back-end side bugs, I would help you to rebase the patch after landing them. (because some code flow might be changed during review process)
Thanks for your help!
(Assignee)

Comment 12

2 years ago
Thinking a bit more about it I think we shouldn't create our own style. Instead we should make sure to create a bitmap that matches what the notification style expects. Favicons.java will return whatever is available - this can be a tiny favicon, like here. If this is the case then we should center this Favicon inside a larger Bitmap.

There are two things happening in parallel that will improve the situation here too (meta bug 1265712): Using touch icons in the UI (bug 1265710 and bug 1265708) - This will improve the icon for YouTube in particular, and better fallback icons for low-quality icons (bug 1265497).

(In reply to Alastor Wu [:alwu] from comment #11)
> Now I'm going to finish some dependency back-end side bugs, I would help you
> to rebase the patch after landing them. (because some code flow might be
> changed during review process)

Okay! Just re-flag me as soon as I can help here.
Flags: needinfo?(s.kaspari)

Comment 13

2 years ago
Created attachment 8750084 [details] [diff] [review]
Rebase patch

Rebase for Scott's patch.
(Reporter)

Comment 14

2 years ago
Hi Sebastian,

Alastor helped me rebased the patch, but still has 2 dependencies waiting to be reviewed and check-in, so we'll need to apply those 2 patches before applying this one:

1. https://reviewboard.mozilla.org/r/45363/ (bug 1257738)
2. https://reviewboard.mozilla.org/r/36809/ (bug 1240423)
3. https://bugzilla.mozilla.org/attachment.cgi?id=8750084 (this bug)

You mentioned the favicon issues and possible solutions. If we do that then maybe the bigContentView wouldn't be that big of a problem anymore.

Again, thank you so much for the help :)
Flags: needinfo?(s.kaspari)

Comment 15

2 years ago
Hi, Anthony,
I have a UI question about Fennec's media-control, could you help me that?

The question is "what kind of small icon we should display during the media playing?"
The small icon is the tiny icon on the status bar, eg. battery/wifi icon [1].

Very appreciate your help :)

[1] https://goo.gl/photos/xPPrG51djrFTaCk4A
Flags: needinfo?(alam)
(In reply to Alastor Wu [:alwu] from comment #15)
> Hi, Anthony,
> I have a UI question about Fennec's media-control, could you help me that?
> 
> The question is "what kind of small icon we should display during the media
> playing?"
> The small icon is the tiny icon on the status bar, eg. battery/wifi icon [1].
> 
> Very appreciate your help :)
> 
> [1] https://goo.gl/photos/xPPrG51djrFTaCk4A

Hey Alastor,

I'm having trouble following this thread from the original bug (bug 1240423). 

Can you post a screenshot of what the current implementation looks like after we addressed our issues from the original bug?
Flags: needinfo?(alam) → needinfo?(alwu)

Comment 17

2 years ago
Hi, Anthony,
The current implementation is like the picture in comment3, but the final goal we want it looks like is in the comment9. (And that is what Scott is working on) 

That kind of control interface would appear in both notification tray and lock-screen. My question in comment15 is about the top small icon in notification bar.

Except the small icon, we have other problems like 
(1) The favicon resolution is not enough
(2) Do we need to change the icon of the play/pause button? (now we use android's default icon)
(3) The color of media control interface

Thanks!
Flags: needinfo?(alwu)
(In reply to Alastor Wu [:alwu] from comment #17)
> Hi, Anthony,
> The current implementation is like the picture in comment3, but the final
> goal we want it looks like is in the comment 9. (And that is what Scott is
> working on) 

Are we just imitating the Chrome implementation/functionality here?

If so, can we use our media control icons from bug 1250741 for the 'play' and 'pause' icons?

> That kind of control interface would appear in both notification tray and
> lock-screen. My question in comment15 is about the top small icon in
> notification bar.

I see Chrome is using the 'audio' icon. We could use the single tone Firefox icon that we use in our other notifications.

Let me know if that doesn't make sense.
Flags: needinfo?(alwu)

Comment 19

2 years ago
It sounds good! Thanks :)
Flags: needinfo?(alwu)
(In reply to Alastor Wu [:alwu] from comment #19)
> It sounds good! Thanks :)

Thanks Alastor!

would you mind showing me a screenshot when you're done? :)
Flags: needinfo?(alwu)

Comment 21

2 years ago
Sure! 
Scott is working on this bug now, I think he would post the picture when he done it :)
Flags: needinfo?(alwu)
(Reporter)

Comment 22

2 years ago
Created attachment 8755376 [details] [diff] [review]
Rebase patch

(Rebased patch, fixed notification icon and strings)

These 2 patches should come before this one:
1. https://reviewboard.mozilla.org/r/45363/ (bug 1257738)
2. https://reviewboard.mozilla.org/r/36809/ (bug 1240423)


Hi Sebastian,

Having spent some more time trying to make it work, I afraid I'm still unable to piece together a solution. I referred to the existing implementation on TwoLinePageRow[1] for the use of FaviconView[2], but in this case the MediaControlService is extended from Service, and I'm not sure what changes I'd have to make for it to work here. 

Again, thanks for your help. I'm still in the process of following through Android/Java materials to get a better technical understanding, but I worry this is causing unnecessary delay to shipping this feature.

Please let me know if you or anyone who's familiar with Fennec would be willing to take a look at this bug.

Thanks a lot!

[1]: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java#109
[2]: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/widget/FaviconView.java
Attachment #8741711 - Attachment is obsolete: true
Attachment #8750084 - Attachment is obsolete: true
Attachment #8741711 - Flags: feedback?(liuche)
(Assignee)

Comment 23

2 years ago
Thank you for rebasing! I'll have a look now. :)
Flags: needinfo?(s.kaspari)
I am releasing this bug for other to grab; it's no longer productive for Scott (who is new to Android) to finish the patch.
Assignee: scwwu → nobody
(Assignee)

Comment 25

2 years ago
(In reply to Sebastian Kaspari (:sebastian) from comment #23)
> Thank you for rebasing! I'll have a look now. :)

I still want to see if I can help but bug 1275016 is blocking my (full) builds currently.
(Assignee)

Updated

2 years ago
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Sebastian,
Thank you to help on this bug!
Created attachment 8759306 [details]
img_album_default.zip

Let's just use the Firefox icon for the notification bar for now.

Meanwhile, I've also attached a default album art image for us.
(Assignee)

Comment 28

2 years ago
Created attachment 8759651 [details]
Bug 1264901 - Implement media-control front-end.

Review commit: https://reviewboard.mozilla.org/r/57576/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57576/
Attachment #8759651 - Flags: review?(ahunt)
(Assignee)

Comment 29

2 years ago
There's some cleanup I want to do in MediaControlService. But I'll file a follow-up bug for that.
(Assignee)

Updated

2 years ago
Attachment #8755376 - Attachment is obsolete: true

Comment 30

2 years ago
Comment on attachment 8759651 [details]
Bug 1264901 - Implement media-control front-end.

https://reviewboard.mozilla.org/r/57576/#review54468

::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java:268
(Diff revision 1)
> -        // TODO : use website name, content and favicon in bug1264901.
> -        return new Notification.Builder(this)
> -            .setSmallIcon(android.R.drawable.ic_media_play)
> -            .setContentTitle("Media Title")
> -            .setContentText("Media Artist")
> -            .setDeleteIntent(getDeletePendingIntent())
> +        ThreadUtils.assertNotOnUiThread();
> +
> +        Notification.MediaStyle style = new Notification.MediaStyle();
> +        style.setShowActionsInCompactView(0);
> +
> +        final Tab tab = mTabReference.get();

This seems potentially dangerous - what if the tab disappears between our check in notifyControlInterfaceChanged, and running this on the background thread?

We could probably either check that (tab != null) in updateNotification(), or assign
final Tab tab = mTabReference.get();
in notifyControlInterfaceChanged() and pass that into updateNotification(). (The first option seems a lot simpler to me.)

::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java:317
(Diff revision 1)
>  
> -    private PendingIntent getClickPendingIntent() {
> -        Intent intent = new Intent(getApplicationContext(), BrowserApp.class);
> -        return PendingIntent.getActivity(getApplicationContext(), 0, intent, 0);
> +    private Bitmap generateCoverArt(Tab tab) {
> +        Bitmap favicon = tab.getFavicon();
> +
> +        // If we do not have a favicon or if it's smaller than 72 pixels then just use the default icon.
> +        if (favicon == null || favicon.getWidth() < 72 || favicon.getHeight() < 72) {

Is it worth making this density dependent? E.g. 64x64 might still be acceptable on older devices e.g. the N4 (I'm not quite sure how low to go though).
Attachment #8759651 - Flags: review?(ahunt) → review+

Comment 31

2 years ago
Hi, Anthony, Sebastian,
Very appreciate your help!

I have a question about the notification UI style, do we still want the collapsed notification UI style for control interface [1]? After applying Sebastian's patch, the control interface is a big view, instead of small view.

Thanks!

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1240423#c10
Flags: needinfo?(alam)
(Assignee)

Comment 32

2 years ago
(In reply to Alastor Wu [:alwu] from comment #31)
> I have a question about the notification UI style, do we still want the
> collapsed notification UI style for control interface [1]? After applying
> Sebastian's patch, the control interface is a big view, instead of small
> view.

It is both. If it's the only one or the most important one then Android will show the big view. In all other cases it will show the small, collapsed view. This is just following what the MediaStyle notification does by default.
(Assignee)

Comment 33

2 years ago
https://reviewboard.mozilla.org/r/57576/#review54468

> This seems potentially dangerous - what if the tab disappears between our check in notifyControlInterfaceChanged, and running this on the background thread?
> 
> We could probably either check that (tab != null) in updateNotification(), or assign
> final Tab tab = mTabReference.get();
> in notifyControlInterfaceChanged() and pass that into updateNotification(). (The first option seems a lot simpler to me.)

I'll pass the tab from notifyControlInterfaceChanged() to updateNotification() if it's not null.

> Is it worth making this density dependent? E.g. 64x64 might still be acceptable on older devices e.g. the N4 (I'm not quite sure how low to go though).

I started with the exactly needed value in dp. Basically no favicon was big enough for the big notification view (min. 64x64dp). I tried various values to find one that still looks good on a high density device and that is available in the wild. 72px seemed to be the sweet spot.
Additionally, right now we are just grabbing the favicon from the Tab. However the Tab will only load a favicon with size "browserToolbarFaviconSize" (= 21.33dp) at max.
This is more or less a band-aid fix so that we show some favicons at all. I'll file a follow-up bug to investigate more options.

Comment 35

2 years ago
(In reply to Sebastian Kaspari (:sebastian) from comment #32)
> It is both. If it's the only one or the most important one then Android will
> show the big view. In all other cases it will show the small, collapsed
> view. This is just following what the MediaStyle notification does by
> default.

Hi, Sebastian,
Since Chrome uses the small view even when it's the only notification, so my question is whether we need/want to use the "MediaStyle"? Or we should use our custom small view with the play/pause button?
Thanks!
(Assignee)

Comment 36

2 years ago
Created attachment 8760168 [details]
chrome-media-notification.png

(In reply to Alastor Wu [:alwu] from comment #35)
> Since Chrome uses the small view even when it's the only notification, so my
> question is whether we need/want to use the "MediaStyle"? Or we should use
> our custom small view with the play/pause button?

I see this big notification when using Chrome (52). Can you expand the notification with two fingers or is it always a small notification on your device?

Using the system style is usually more future-proof as the system changes in the future. So I'm a bit hesitant to write a custom style (and it's of course more work and needs to be maintained).

With a custom notification we could adapt better to our current icon situation (There are a bunch of bugs filed to improve the icon quality). But we can still do this in a follow-up bug, I guess?

Comment 37

2 years ago
(In reply to Sebastian Kaspari (:sebastian) from comment #36)
> I see this big notification when using Chrome (52). Can you expand the
> notification with two fingers or is it always a small notification on your
> device?

It seems that Chrome modifies its display style, and this photo [1] is what I see in Chrome (50).

[1] https://photos.google.com/share/AF1QipOGCSP4FLETbKmkotvJ25mrVCTfh4xjGYazuH8L6er-5c0j7lfIXfutAJaOxa7t5w?key=bmZKNzM1SVdZd0U4ajZqVW5kSzI2YThLLTJFWXBR

> Using the system style is usually more future-proof as the system changes in
> the future. So I'm a bit hesitant to write a custom style (and it's of
> course more work and needs to be maintained).
> 
> With a custom notification we could adapt better to our current icon
> situation (There are a bunch of bugs filed to improve the icon quality). But
> we can still do this in a follow-up bug, I guess?

Thanks for your explanation!
I just want to know what kind of style we would see in the future :)
(Assignee)

Comment 38

2 years ago
Comment on attachment 8759651 [details]
Bug 1264901 - Implement media-control front-end.

Aw, this missed merge day.

Approval Request Comment

[Feature/regressing bug #]: This patch implements the media control frontend on top of bug 1257738 and bug 1240423 (They are already in Aurora).

[User impact if declined]: The patches in bug 1257738 and 1240423 landed with an unfinished media notification. In some cases this notification can be triggered and is shown to the user.

[Describe test coverage new/current, TreeHerder]: Local testing with websites that play audio (YouTube, Soundcloud, ..)

[Risks and why]: Medium - This patch contains changes to the new MediaControlService. So there's a risk of breaking something. However this patch is needed to transform the unfinished notification into what it should look like. So the only other option is to back everything else out again on Aurora.

[String/UUID change made/needed]: -
Attachment #8759651 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

2 years ago
Depends on: 1278255

Comment 39

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/11010bdd45ac
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
(Assignee)

Updated

2 years ago
Depends on: 1278624
(Assignee)

Updated

2 years ago
Depends on: 1278626

Updated

2 years ago
Flags: needinfo?(alam)
(Assignee)

Updated

2 years ago
Depends on: 1279137
(Assignee)

Updated

2 years ago
Blocks: 1279714
Comment on attachment 8759651 [details]
Bug 1264901 - Implement media-control front-end.

Media control work that missed the merge, we'd like audio to work smoothly, let's uplift to aurora along with bug 1278255.
Attachment #8759651 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 41

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/81216130eacc
status-firefox49: --- → fixed

Updated

2 years ago
Blocks: 1282391
I am not getting media controls on Firefox 49 using a Nexus 5x and Android N however on Firefox 50 they appear. 

Media controls do not appear on Firefox 49 or 50 using an Android One Spice Android v4.4, HTC Evo Android 4.1.1 or a Galaxy S3 GT-I9300 Android 4.3.

Comment 44

2 years ago
The media controls is only supported for android 5.0 and above.
See Also: → bug 1286433

Updated

2 years ago
Alias: fennec-media-control

Updated

2 years ago
Depends on: 1284387

Updated

2 years ago
Depends on: 1287683

Updated

2 years ago
No longer blocks: 1282391
Depends on: 1282391
(Assignee)

Updated

2 years ago
Depends on: 1288972
(Assignee)

Updated

2 years ago
Depends on: 1288971
Depends on: 1289095
(Assignee)

Updated

2 years ago
Depends on: 1289356
Depends on: 1290098
Depends on: 1290109
Depends on: 1290029
Depends on: 1290467

Updated

2 years ago
Depends on: 1290510

Updated

2 years ago
Depends on: 1290836

Updated

2 years ago
No longer depends on: 1288972, 1289095, 1290029, 1290467, 1290510
Flags: needinfo?(liuche)
Depends on: 1292585
Depends on: 1305869
(Assignee)

Updated

2 years ago
Depends on: 1326114

Updated

a year ago
Depends on: 1347648

Updated

a year ago
Depends on: 1349503

Updated

a year ago
Depends on: 1349506

Updated

a year ago
Depends on: 1351087
You need to log in before you can comment on or make changes to this bug.