Closed Bug 1264901 (fennec-media-control) Opened 4 years ago Closed 4 years ago

Implement the remote media-control front-end

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: scottwu, Assigned: sebastian)

References

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

Details

Attachments

(5 files, 3 obsolete files)

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.
Assignee: nobody → scwwu
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)
(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)
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.
Attachment #8741711 - Flags: feedback?(margaret.leibovic) → feedback?(liuche)
(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.
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)
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)
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/
Flags: needinfo?(s.kaspari)
Can you rebase the patches for me? There are several errors when importing the patch in bug 1240423.
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!
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)
Attached patch Rebase patch (obsolete) — Splinter Review
Rebase for Scott's patch.
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)
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)
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)
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)
Sure! 
Scott is working on this bug now, I think he would post the picture when he done it :)
Flags: needinfo?(alwu)
Attached patch Rebase patch (obsolete) — Splinter Review
(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)
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
(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: nobody → s.kaspari
Status: NEW → ASSIGNED
Sebastian,
Thank you to help on this bug!
Attached file 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.
There's some cleanup I want to do in MediaControlService. But I'll file a follow-up bug for that.
Attachment #8755376 - Attachment is obsolete: true
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+
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)
(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.
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.
(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!
(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?
(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 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?
Depends on: 1278255
https://hg.mozilla.org/mozilla-central/rev/11010bdd45ac
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Depends on: 1278624
Depends on: 1278626
Flags: needinfo?(alam)
Depends on: 1279137
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+
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.
The media controls is only supported for android 5.0 and above.
See Also: → 1286433
Alias: fennec-media-control
Depends on: 1284387
No longer blocks: 1282391
Depends on: 1282391
Depends on: 1288972
Depends on: 1288971
Depends on: 1289095
Depends on: 1289356
Depends on: 1290029
Depends on: 1290467
Depends on: 1290510
Depends on: 1290836
No longer depends on: 1288972, 1289095, 1290029, 1290467, 1290510
Flags: needinfo?(liuche)
Depends on: 1292585
Depends on: 1305869
Depends on: 1326114
Depends on: 1347648
Depends on: 1349503
Depends on: 1349506
Depends on: 1351087
You need to log in before you can comment on or make changes to this bug.