1264901 - (fennec-media-control) Implement the remote media-control front-end

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Scott Wu [:scottwu]]

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.

Comment 1

[Scott Wu [:scottwu]]

Attachment: 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/

Comment 2

[Scott Wu [:scottwu]]

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!

Comment 3

[Scott Wu [:scottwu]]

Attachment: Notification expanded big view with pixelated favicon

Comment 4

[:Margaret Leibovic]

(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).

Comment 5

[:Margaret Leibovic]

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.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

(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.

Comment 7

[Scott Wu [:scottwu]]

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!

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 9

[Scott Wu [:scottwu]]

Attachment: 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/

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

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

Comment 11

[Alastor Wu [:alwu]]

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!

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 13

[Alastor Wu [:alwu]]

Attachment: Rebase patch

Rebase for Scott's patch.

Comment 14

[Scott Wu [:scottwu]]

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 :)

Comment 15

[Alastor Wu [:alwu]]

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

Comment 16

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(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?

Comment 17

[Alastor Wu [:alwu]]

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!

Comment 18

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(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.

Comment 19

[Alastor Wu [:alwu]]

It sounds good! Thanks :)

Comment 20

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(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? :)

Comment 21

[Alastor Wu [:alwu]]

Sure! 
Scott is working on this bug now, I think he would post the picture when he done it :)

Comment 22

[Scott Wu [:scottwu]]

Attachment: 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

Comment 23

[Sebastian Kaspari (:sebastian; :pocmo)]

Thank you for rebasing! I'll have a look now. :)

Comment 24

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

(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.

Comment 26

[Blake Wu [:bwu][:blakewu]]

Sebastian,
Thank you to help on this bug!

Comment 27

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: 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.

Comment 28

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 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/

Comment 29

[Sebastian Kaspari (:sebastian; :pocmo)]

There's some cleanup I want to do in MediaControlService. But I'll file a follow-up bug for that.

Comment 30

[Andrzej Hunt :ahunt]

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).

Comment 31

[Alastor Wu [:alwu]]

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

Comment 32

[Sebastian Kaspari (:sebastian; :pocmo)]

(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.

Comment 33

[Sebastian Kaspari (:sebastian; :pocmo)]

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 34

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/11010bdd45ac89b20b4f0414a6a70e4f11301087
Bug 1264901 - Implement media-control front-end. r=ahunt

Comment 35

[Alastor Wu [:alwu]]

(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!

Comment 36

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 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

[Alastor Wu [:alwu]]

(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 :)

Comment 38

[Sebastian Kaspari (:sebastian; :pocmo)]

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]: -

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/11010bdd45ac

Comment 40

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/81216130eacc

Comment 42

[Kevin Brosnan [Ex-Mozilla]]

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 43

[Kevin Brosnan [Ex-Mozilla]]

Using https://commons.wikimedia.org/wiki/File:Elephants_Dream.ogg as a test file as well as http://www.bbc.com/sport/football/36761013

Comment 44

[Alastor Wu [:alwu]]

The media controls is only supported for android 5.0 and above.