Closed Bug 1071831 Opened 10 years ago Closed 10 years ago

HTML5 video playback is not paused in the browser after opening in external player (open with app)

Categories

(Firefox for Android Graveyard :: General, defect)

32 Branch
All
Android
defect
Not set
normal

Tracking

(firefox32 affected, firefox33 affected, firefox34 affected, firefox35 affected, fennec+)

RESOLVED FIXED
Firefox 35
Tracking Status
firefox32 --- affected
firefox33 --- affected
firefox34 --- affected
firefox35 --- affected
fennec + ---

People

(Reporter: czerny.jakub, Assigned: esawin)

References

Details

(Keywords: reproducible)

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:32.0) Gecko/20100101 Firefox/32.0
Build ID: 20140831201947

Steps to reproduce:

a) <video> tag in HTML page
1. open page containing video tag with video with sound
2. start video playback
3. long click on video to view context menu
4. select "Open With an App"

 or
b) playing standalone video file
1. open video file with sound (*.mp4, *.webm)
2. ensure the video is playing
3. open video in external app by clicking on android icon at the end of the address bar


Actual results:

Video played inside Firefox is not paused. Screen is overlaid by player application. The sound of video is played twice. 


Expected results:

Playback in Firefox should be paused when the video file is opened in external application.
Which device, Android version, player used, and version of Firefox are you using?
OS: Android → Linux
Hardware: All → x86_64
OS: Linux → Android
Hardware: x86_64 → All
OS: Linux → Android
Hardware: x86_64 → All
Device: Samsung Galaxy S2
Android: 4.1
Player: VLC, generic android video player
Firefox: 32
Thanks. I was able to reproduce. Seems annoying, tracking?
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Ever confirmed: true
Keywords: reproducible
Summary: HTML5 video is not paused when opened in external player → HTML5 video playback is not paused in the browser after opening in external player (open with app)
Assignee: nobody → esawin
tracking-fennec: ? → +
This should be very trivial, but there are two paths through which this can happen. Long pressing on a video and opening a video directly in Firefox. The first is trivial. We just need to pause in here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#7812

The second goes through:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#7848

getting the video and pausing it there will be more complex.
Attached patch video-pause.patch (obsolete) — Splinter Review
Pause media playback when launching external apps, resume if external app launch is canceled.

We could resume playback when exiting the external app, too. It would need some more work though and might not be the desired behavior.
Attachment #8497547 - Flags: review?(mark.finkle)
Status: NEW → ASSIGNED
Comment on attachment 8497547 [details] [diff] [review]
video-pause.patch

This seems sane to me, but I want Wes to see if this is the right approach and whether you have enough of the code paths covered.
Attachment #8497547 - Flags: review?(wjohnston)
Attachment #8497547 - Flags: review?(mark.finkle)
Attachment #8497547 - Flags: feedback+
Comment on attachment 8497547 [details] [diff] [review]
video-pause.patch

Review of attachment 8497547 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/browser.js
@@ +7825,5 @@
>  
> +  _getMediaContentElement(element) {
> +    // Opening media files directly constructs a dummy document with one
> +    // child node holding the media content.
> +    return element && element.childNodes[0] || element;

I don't really like this. Its confusing enough what's happening here and why it works that you should add some inline docs. i.e. this function document care about media elements at all. It just checks if the element we're passed has a child, if it does it returns the first child, otherwise it returns the element itself. Video elements have source tags, so its possible they'll have children inside them. Will this fail then? What if you pass in a random <div>. This returns something to you in that case. Should it?

I guess this is really only ever passed document.activeElement for entire pages. On a MediaDocument there won't be any <source> tags so that will either be the <body> or the <video> and if we get the <body> this will pick the <video>. On non-MediaDocuments the results will be a bit random depending on what is focused.

I think for here we only really care about MediaDocuments so we should try to return null if the document is a non-Media one. Maybe you could check document.contentType before calling updatePageAction, and only pass an element if the type is video or audio. If it is one of those, document.body.firstChild is (probably?) always the node you want, so you can remove this check/function entirely. i.e. don't pass an element if its not a video/audio?

@@ +7845,5 @@
>        clickCallback: () => {
>          UITelemetry.addEvent("launch.1", "pageaction", null, "helper");
>  
> +        let wasPlaying = mediaElement && mediaElement.play && !mediaElement.paused && !mediaElement.ended;
> +        if (wasPlaying && mediaElement.pause) {

Its confusing that you check for mediaElement.play here, but check for pause in the line before you call pause. We should at least be consistent (or just throw if you get a mediaElement that's not instanceof Ci.nsIDOMHTMLMediaElement and don't do these checks at all).
Attachment #8497547 - Flags: review?(wjohnston) → review-
Attached patch video-pause.patch (obsolete) — Splinter Review
Thanks for the review!

I did remove the checks for the pause and play functions and check for the expected type explicitly instead.

However, I have left the type check inside of _setUriForPageAction, since I don't want to add redundancy each time before calling updatePageAction and would like to defer the check as long as possible, i.e., only do it when page actions are actually available for the given document.

Do you still prefer doing the content type checking before calling updatePageAction instead?
Attachment #8497547 - Attachment is obsolete: true
Attachment #8502101 - Flags: review?(wjohnston)
Comment on attachment 8502101 [details] [diff] [review]
video-pause.patch

Review of attachment 8502101 [details] [diff] [review]:
-----------------------------------------------------------------

I can live with this :) Thanks :)

::: mobile/android/chrome/content/browser.js
@@ +7903,5 @@
>  
> +  _getMediaContentElement(contentDocument) {
> +    if (contentDocument.contentType.indexOf("video/") !== 0 &&
> +        contentDocument.contentType.indexOf("audio/") !== 0) {
> +      return null;

"startsWith" is a little clearer than indexOf() !== 0.

@@ +7911,5 @@
> +
> +    if (element instanceof HTMLBodyElement) {
> +      element = element.firstChild;
> +    }
> +    if (element instanceof HTMLMediaElement) {

At a line between these two checks.
Attachment #8502101 - Flags: review?(wjohnston) → review+
Review comments addressed.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7c8f841a44a3
Attachment #8502101 - Attachment is obsolete: true
Attachment #8502584 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1cf977b4f846
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.