Closed
Bug 1264901
(fennec-media-control)
Opened 8 years ago
Closed 8 years ago
Implement the remote media-control front-end
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(firefox49 fixed, firefox50 fixed)
RESOLVED
FIXED
Firefox 50
People
(Reporter: scottwu, Assigned: sebastian)
References
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.
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → scwwu
Reporter | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/46703/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46703/
Reporter | ||
Comment 2•8 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•8 years ago
|
||
Comment 4•8 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•8 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•8 years ago
|
Attachment #8741711 -
Flags: feedback?(margaret.leibovic) → feedback?(liuche)
Assignee | ||
Comment 6•8 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•8 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•8 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•8 years ago
|
||
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•8 years ago
|
Flags: needinfo?(s.kaspari)
Assignee | ||
Comment 10•8 years ago
|
||
Can you rebase the patches for me? There are several errors when importing the patch in bug 1240423.
Comment 11•8 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•8 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•8 years ago
|
||
Rebase for Scott's patch.
Reporter | ||
Comment 14•8 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•8 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)
Comment 16•8 years ago
|
||
(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•8 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)
Comment 18•8 years ago
|
||
(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 20•8 years ago
|
||
(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•8 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•8 years ago
|
||
(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•8 years ago
|
||
Thank you for rebasing! I'll have a look now. :)
Flags: needinfo?(s.kaspari)
Comment 24•8 years ago
|
||
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•8 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•8 years ago
|
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Comment 26•8 years ago
|
||
Sebastian, Thank you to help on this bug!
Comment 27•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
There's some cleanup I want to do in MediaControlService. But I'll file a follow-up bug for that.
Assignee | ||
Updated•8 years ago
|
Attachment #8755376 -
Attachment is obsolete: true
Comment 30•8 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•8 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•8 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•8 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.
Assignee | ||
Comment 34•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/11010bdd45ac89b20b4f0414a6a70e4f11301087 Bug 1264901 - Implement media-control front-end. r=ahunt
Comment 35•8 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•8 years ago
|
||
(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•8 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•8 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?
Comment 39•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/11010bdd45ac
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Updated•8 years ago
|
Flags: needinfo?(alam)
Comment 40•8 years ago
|
||
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•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/81216130eacc
status-firefox49:
--- → fixed
Comment 42•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
The media controls is only supported for android 5.0 and above.
Updated•8 years ago
|
Alias: fennec-media-control
Updated•8 years ago
|
Depends on: fennec-media-control-tests
Updated•8 years ago
|
Updated•8 years ago
|
Updated•8 years ago
|
Flags: needinfo?(liuche)
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•