Closed Bug 1290467 Opened 3 years ago Closed 3 years ago

Media control notification is not displayed when no-audio media is playing

Categories

(Firefox for Android :: Audio/Video, defect, P2)

50 Branch
ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox49 --- affected
firefox50 --- affected
firefox51 --- verified

People

(Reporter: sflorean, Assigned: alwu)

References

(Depends on 1 open bug)

Details

Attachments

(10 files)

58 bytes, text/x-review-board-request
baku
: review+
Details
58 bytes, text/x-review-board-request
snorp
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
baku
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
58 bytes, text/x-review-board-request
sebastian
: review+
Details
Environment: 
Device: LG G4 (Android 5.1);
Build: Nightly 50.0a1 (2016-07-29);

Steps to reproduce:
1. Go to https://www.youtube.com/watch?v=jqROVzXZPJs&feature=youtu.be; 
2. Swipe down the notification bar.

Expected result:
Media control notification is displayed.

Actual result:
Media control notification is not displayed.

Notes:
On Android Chrome media control is displayed.
Priority: -- → P2
This is my design concept, because some websites would use the non-audible video as their background image, and I think we shouldn't show the control interface for such kinds of websites.

I requested the UX suggestion in [1], we can wait for Anthony's response.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1289095#c2
Blocks: 1290836
Does the video actually have no audio track or is it just silent?
No longer blocks: fennec-media-control
Has audio track, but it contains only silence data.
Assignee: nobody → alwu
Duplicate of this bug: 1289095
Hi, Anthony,
Continue the discussion from bug1289095 comment7,
We have decided to show the media control for the non-audible media (without audio track), but here I have a question.

In following case, which tab should be showed on control interface?
1. open and play an *audible* media from tab1
   - the control interface is for tab1
2. open and play an *non-audible* media from tab2
   - should we pause tab1?
   - if not, should we show the tab2 information in control interface?

If both tabs are audible, the audio competing mechanism would stop tab1 (only allow one audible tab), so the control interface would be for the tab2.

Thanks!
Flags: needinfo?(alam)
Hi, Anthony,
Sorry to bother you again,
I need your help here because I can't continue to work on this bug before getting the specific UX spec for non-audible media.
Thanks!
(In reply to Alastor Wu [:alwu] from comment #5)
> If both tabs are audible, the audio competing mechanism would stop tab1
> (only allow one audible tab), so the control interface would be for the tab2.

I think we should follow this current behavior that you've outlined for now. The notification shortcut UI will always pertain to the most recent tab.

As I've mentioned before, I worry that we are trying to be too "smart" with the UX here so I'm hesitant to change too much without some more research/feedback. Let's monitor the feedback here and adjust.
Flags: needinfo?(alam)
Hi Anthony, thank you for your response to help us make progress !
Thank you very much!
Duplicate of this bug: 1285739
Comment on attachment 8786691 [details]
Bug 1290467 - part2 : request audio focus for any media instead of audible one.

https://reviewboard.mozilla.org/r/75598/#review73996
Attachment #8786691 - Flags: review?(snorp) → review+
Comment on attachment 8786692 [details]
Bug 1290467 - part3 : notify MediaControlService with event 'MEDIA_PLAYING_CHANGE'.

https://reviewboard.mozilla.org/r/75600/#review73998

::: mobile/android/base/java/org/mozilla/gecko/Tab.java:856
(Diff revision 2)
> +    public void setIsMediaPlaying(boolean isMediaPlaying) {
> +        mIsMediaPlaying = isMediaPlaying;
> +    }
> +
> +    public boolean isMediaPlaying() {
> +        return mIsMediaPlaying;
> +    }

nit: Can you add a javadoc here explaining the difference between audio playing and media playing?
Attachment #8786692 - Flags: review?(s.kaspari) → review+
Comment on attachment 8786693 [details]
Bug 1290467 - part4 : create helper function.

https://reviewboard.mozilla.org/r/75602/#review74002

::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java:77
(Diff revision 2)
>          return START_NOT_STICKY;
>      }
>  
>      @Override
>      public IBinder onBind(Intent intent) {
> +        initialize();

Why do we have two calls to initialize() now? (onCreate() and onBind()).
Comment on attachment 8786695 [details]
Bug 1290467 - part6 : remove redudant space.

https://reviewboard.mozilla.org/r/75606/#review74006
Attachment #8786695 - Flags: review?(s.kaspari) → review+
Comment on attachment 8786697 [details]
Bug 1290467 - part8 : rename 'ACTION_PLAY' to 'ACTION_RESUME'.

https://reviewboard.mozilla.org/r/75610/#review74008
Attachment #8786697 - Flags: review?(s.kaspari) → review+
(In reply to Sebastian Kaspari (:sebastian) from comment #31)
> Why do we have two calls to initialize() now? (onCreate() and onBind()).

Ah, sorry, we can remove the initialize() in onBind().
Hi, 
FYI the behavior on Nightly build from 31-08-2016 has change, please see the video: https://www.youtube.com/watch?v=a-XAI-u1aPE&feature=youtu.be and https://www.youtube.com/watch?v=XJ3TkHuelEo.
Device: Nexus 5 (Android 6.0.1)
Comment on attachment 8786690 [details]
Bug 1290467 - part1 : dispatch 'media-playback' event.

https://reviewboard.mozilla.org/r/75596/#review74174
Attachment #8786690 - Flags: review?(amarchesini) → review+
Comment on attachment 8786696 [details]
Bug 1290467 - part7 : enable audio competing for non-audible media.

https://reviewboard.mozilla.org/r/75608/#review74176
Attachment #8786696 - Flags: review?(amarchesini) → review+
Comment on attachment 8786693 [details]
Bug 1290467 - part4 : create helper function.

https://reviewboard.mozilla.org/r/75602/#review74862
Attachment #8786693 - Flags: review?(s.kaspari) → review+
Comment on attachment 8786698 [details]
Bug 1290467 - part9 : remove 'ACTION_REMOVE_CONTROL'.

https://reviewboard.mozilla.org/r/75612/#review74866
Attachment #8786698 - Flags: review?(s.kaspari) → review+
Comment on attachment 8787176 [details]
Bug 1290467 - part10 : update audio focus related control operations.

https://reviewboard.mozilla.org/r/75900/#review74868
Attachment #8787176 - Flags: review?(s.kaspari) → review+
Comment on attachment 8786694 [details]
Bug 1290467 - part5 : change service's life time.

https://reviewboard.mozilla.org/r/75604/#review74864

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:2273
(Diff revision 2)
> +        final Intent closeIntent = new Intent(getApplicationContext(),
> +                                   MediaControlService.class);
> +        getApplicationContext().stopService(closeIntent);
> +

Why do you stop the service here? This seems to be wrong. While the application is in the background Android can destroy the activity and will do so whenever it needs memory for other things. In this situation the service will be killed (and the notification removed).

You can test this behavior by enabling "Don't keep activities" in the developer settings. Now whenever you press home and background the app Android will destroy the activity (and re-create it when you resume the app). In this situation audio playback will continue (as it should) but we destroy the service and remove the notification. The service should continue to run (in "foreground" to avoid the process being killed and playback stopping completely).

::: mobile/android/base/java/org/mozilla/gecko/media/AudioFocusAgent.java:81
(Diff revision 2)
>                          break;
>                      default:
>                  }
>              }
>          };
> +        notifyMediaControlService(MediaControlService.ACTION_INIT);

Why do we start the service on every app start? We only need it when there's an audio session?

::: mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java:160
(Diff revision 2)
> +            case ACTION_INIT :
> +                // Do nothing.
> +                break;

From this I do not understand why we always start the service but do not do anything here anyways.
Attachment #8786694 - Flags: review?(s.kaspari) → review-
(In reply to Sebastian Kaspari (:sebastian) from comment #41)
::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java:2273
> Why do you stop the service here? This seems to be wrong. While the
> application is in the background Android can destroy the activity and will
> do so whenever it needs memory for other things. In this situation the
> service will be killed (and the notification removed).
> 
> You can test this behavior by enabling "Don't keep activities" in the
> developer settings. Now whenever you press home and background the app
> Android will destroy the activity (and re-create it when you resume the
> app). In this situation audio playback will continue (as it should) but we
> destroy the service and remove the notification. The service should continue
> to run (in "foreground" to avoid the process being killed and playback
> stopping completely).

If we don't stop service when the app is shutdown, the service would crash and then it would be restarted immediately even the app isn't active anymore.

I observed that from log, after I remove fennec (which is playing now) from task list,
"ActivityManager: Scheduling restart of crashed service org.mozilla.fennec_Alastor/org.mozilla.gecko.media.MediaControlService in 1000ms"

Honestly, I don't know the actual reason why the service crashed, but it works to avoid the crashing if I stoped service before shutting down the app .

Do you know whether there is any better solutions for this case?

> ::: mobile/android/base/java/org/mozilla/gecko/media/AudioFocusAgent.java:81
> Why do we start the service on every app start? We only need it when there's
> an audio session?
> 
> :::
> mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java:160
> From this I do not understand why we always start the service but do not do
> anything here anyways.

Sorry for the confusion, let me explain that.

First, let's recap what we did in present codebase.

We are using "audio-playback" event to notify AudioFocusAgent via AndroidBridge and "DOMAudioPlaybackStarted/Stop" to notify the Tabs (this event is also passed via "audio-playback"). These events would triggered different operations, so we have two paths to communicate with the MediaControlService.

First path from AudioFocusAgent is to start service but doesn't show the notification because we don't know the active tab at that time, or stop service and clear the notification (from requestAudioFocusIfNeeded() and abandonAudioFocusIfNeeded()). Second path from Tabs is to notify MediaControlService the active tab by tab event, but this one should only be executed after starting service, because we register tab event listener in service's onCreate(). 

---

Let's return to my patch, the main purpose of this patch is to allow non-audible media element can also be controlled by control interface. For this reason, we can't use the "audio-playback" event because it's only used for audible media and showing the audio icon in tab view.

Therefore, new event was created, "media-playback" event which would be dispatched by Gecko whenever any media starts playing. This event would be used to notify AudioFocusAgent and Tabs, so now AudioFocusAgent and Tabs are waiting for the same event (dispatched by observer serivce). BUT, we can't control the notification order, that means Tabs might receive the event before the AudioFocusAgent, or AudioFocusAgent receives the event before Tabs.

In present design, the code can work well because they're waiting different events and we know the actual receiving order. But now, we can't control that, so the notification would not be showed if the Tabs receiving event before AudioFocusAgent. (the service doesn't be created yet, so the tab event was missing.)

I want to reduce this uncertain factor, so I decide to remove the communication path from AudioFocusAgent. To make the call flow more easily, we only let Tabs to control MediaControlService via tab event. 

But how to make sure the MediaControlService has already been created and registered the tab event listener? First method is to create binding in Tabs, and dispatch the tab event after the service is created. Second method is to allow MediaControlService always be alive.

I choose the second method because I don't want to expose more MediaControlService related code in Tabs (like binding, sending intent, checking the service is alive or not) and I think it might be more easily to understand.

Therefore, I want to create the MediaControlService when the fennec starts, but the MediaControlService doesn't need to do anything, it's just waiting the tab event.

How do you think?

Thanks!
Flags: needinfo?(s.kaspari)
Removing NI: We talked on IRC and the patch is already updated. I'll build, test and review the outstanding patches now.
Flags: needinfo?(s.kaspari)
While testing this patch (with "Don't keep activities" on) I came into a state (after playing a couple of things, and killing the app) where audio was still playing but the notification gone (and I guess the service too).
I also met this problem before (using the clear Nightly) and I think that might be another issue for fennec's media playback but not for media control.
How often can you reproduce this issue? Can you reproduce that in clean Nightly?
Comment on attachment 8786694 [details]
Bug 1290467 - part5 : change service's life time.

https://reviewboard.mozilla.org/r/75604/#review75978
Attachment #8786694 - Flags: review?(s.kaspari) → review+
(In reply to Alastor Wu [:alwu] from comment #56)
> How often can you reproduce this issue? Can you reproduce that in clean
> Nightly?

So far this only happened once (with the patches applied). I'll file a new bug once this lands and it happens again in Nightly (and I have some more details).
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c3210fd31140
part1 : dispatch 'media-playback' event. r=baku
https://hg.mozilla.org/integration/autoland/rev/ef1a345f206b
part2 : request audio focus for any media instead of audible one. r=snorp
https://hg.mozilla.org/integration/autoland/rev/fdc63760ddc8
part3 : notify MediaControlService with event 'MEDIA_PLAYING_CHANGE'. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/f1f65fd576fc
part4 : create helper function. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/e9512bfae27b
part5 : change service's life time. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/7eb048d423d1
part6 : remove redudant space. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/789d97d6ca60
part7 : enable audio competing for non-audible media. r=baku
https://hg.mozilla.org/integration/autoland/rev/9c459f432b36
part8 : rename 'ACTION_PLAY' to 'ACTION_RESUME'. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/0ccb80fc769b
part9 : remove 'ACTION_REMOVE_CONTROL'. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/52a630b9ed23
part10 : update audio focus related control operations. r=sebastian
No longer blocks: 1268368
Keywords: feature
Verified as fixed on the latest Nightly build (51.0a1 2016-09-11) on both Samung Galaxy S6 EDGE (Android 6.0) and Nexus 7 (Android 5.1.1)
Removing the "feature" keyword here. This is a bug affecting the Fennec Audio Controls Enhancement feature (which is bug 1268368), but not a feature in itself. Due to this, this bug only needs to block the main/meta bug (bug 1268368) so we know it's related to that.
Keywords: feature
Depends on: 1347647
Depends on: 1416169
You need to log in before you can comment on or make changes to this bug.