Closed Bug 1282410 Opened 8 years ago Closed 8 years ago

Using external app to open unsupported type media

Categories

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

Product:
Firefox for Android Graveyard
Component:
Audio/Video
Type:
defect

Tracking

(firefox49 affected, firefox50 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 50
Tracking Flags:
Tracking Status
firefox49 --- affected
firefox50 --- fixed

People

(Reporter: alwu, Assigned: alwu)

References

(Blocks 1 open bug)

Details

Attachments

(8 files, 1 obsolete file)

Bug 1282410 - part1 : open unsupported type media.
58 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1282410 - part2 : open media with external apps.
58 bytes, text/x-review-board-request
Margaret
: review+
Details
Bug 1282410 - part3 : add log.
58 bytes, text/x-review-board-request
cpearce
: review+
Details
Test page for unsupported video
579 bytes, text/html
Details
Screenshot_20160706-101430.png
114.64 KB, image/png
Details
Fennec HLS flow
1020.21 KB, image/png
Details
Samsung browser no default player assigned
1.76 MB, image/jpeg
Details
Screenshot_20160921-160633.png
80.07 KB, image/png
Details 
For all unsupported types media (eg. m3u8, mkv, 3gp...) on Android, we could use the external app to play them.
Attachment #8765782 - Attachment mime type: text/plain → text/html
Attachment #8765775 - Flags: review?(cpearce)
Attachment #8765776 - Flags: review?(margaret.leibovic)
Attachment #8765777 - Flags: review?(cpearce)
Hi, Chris, Margaret,
Could you help me review these patches?
The purpose of these patch is to use the external app to open the unsupported types media on Fennec.
Thanks!
Summary: Using external app to open unsupported types → Using external app to open unsupported type media
Severity: normal → critical
Priority: -- → P2
Comment on attachment 8765775 [details]
Bug 1282410 - part1 : open unsupported type media.

https://reviewboard.mozilla.org/r/60972/#review58324

::: dom/html/HTMLMediaElement.cpp:2491
(Diff revision 1)
>    // and our preload status.
>    AddRemoveSelfReference();
>    UpdatePreloadAction();
>    UpdateSrcMediaStreamPlaying();
>    UpdateAudioChannelPlayingState();
> +  OpenUnsupportedMediaWithExtenalAppIfNeeded();

The media load algorithm is asynchronous. So you should not be able to observe the load failing with MEDIA_ERR_SRC_NOT_SUPPORTED this early in the load algorithm; you must wait until the event loop has done another pass and the first SelectResource task has run before an error can be thrown.

So the only place you should need to call OpenUnsupportedMediaWithExtenalAppIfNeeded() is at the end of HTMLMediaElement::NoSupportedMediaSourceError().

Right?

Assuming that's the case, you might be able to simplify the logic there too. That is, in NoSupportedMediaSourceError() you know that the conditions you check in IsUnsupportedMediaType() will always be true.

::: dom/html/HTMLMediaElement.cpp:5684
(Diff revision 1)
>  
>    return true;
>  }
>  
> +bool
> +HTMLMediaElement::IsUnsupportedMediaType() const

Can we call this HaveFailedWithSourceNotSupportedError()? (If we still need this function given my other comments).

Naming it IsUnsupportedMediaType() makes it sound like it does what MediaSource.IsTypeSupported(mimeType) does, but this actually tells you whether the load has failed, not whether the load can be expected to fail.

::: dom/html/HTMLMediaElement.cpp:5699
(Diff revision 1)
> +}
> +
> +void
> +HTMLMediaElement::OpenUnsupportedMediaWithExtenalAppIfNeeded()
> +{
> +  if (!Preferences::GetBool("dom.media.openUnsupportedTypeWithExternalApp")) {

Convention on Media pref would be to name this: "media.openUnsupportedTypeWithExternalApp".
Attachment #8765775 - Flags: review?(cpearce) → review-
Comment on attachment 8765777 [details]
Bug 1282410 - part3 : add log.

https://reviewboard.mozilla.org/r/60976/#review58328

r+ with spelling fixed.

::: dom/html/HTMLMediaElement.h:1632
(Diff revision 1)
>    bool mIsAudioTrackAudible;
>  
>    // True if media element is audible for users.
>    bool mAudible;
> +
> +  nsAutoCString mMineType;

s/mMineType/mMimeType/g

Mi*m*e not Mi*n*e
Attachment #8765777 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #9)
> ::: dom/html/HTMLMediaElement.cpp:2491
> The media load algorithm is asynchronous. So you should not be able to
> observe the load failing with MEDIA_ERR_SRC_NOT_SUPPORTED this early in the
> load algorithm; you must wait until the event loop has done another pass and
> the first SelectResource task has run before an error can be thrown.
> 
> So the only place you should need to call
> OpenUnsupportedMediaWithExtenalAppIfNeeded() is at the end of
> HTMLMediaElement::NoSupportedMediaSourceError().
> 
> Right?
> 
> Assuming that's the case, you might be able to simplify the logic there too.
> That is, in NoSupportedMediaSourceError() you know that the conditions you
> check in IsUnsupportedMediaType() will always be true.

No, here OpenUnsupportedMediaWithExtenalAppIfNeeded() is to handle another case, that is start playing after load failed.

Think about this scenario,
1. Load a media element with unsupported media src 
2. Load fail, so jump out the menu to choose the external app
   At the same time, video control would show "unsupported type"
3. Click media element again, it would play video again

Note. we can play video by "clicking media elemet itself/clicking play-button/pressing space key".
  
In step3, we call play() after load fail, and we need to choose the external app at that time. 
That's why I add OpenUnsupportedMediaWithExtenalAppIfNeeded() in PlayInternal().

> ::: dom/html/HTMLMediaElement.cpp:5684
> Can we call this HaveFailedWithSourceNotSupportedError()? (If we still need
> this function given my other comments).
> 
> Naming it IsUnsupportedMediaType() makes it sound like it does what
> MediaSource.IsTypeSupported(mimeType) does, but this actually tells you
> whether the load has failed, not whether the load can be expected to fail.

Sure!

> ::: dom/html/HTMLMediaElement.cpp:5699
> (Diff revision 1)
> > +}
> > +
> > +void
> > +HTMLMediaElement::OpenUnsupportedMediaWithExtenalAppIfNeeded()
> > +{
> > +  if (!Preferences::GetBool("dom.media.openUnsupportedTypeWithExternalApp")) {
> 
> Convention on Media pref would be to name this:
> "media.openUnsupportedTypeWithExternalApp".

OK.
Flags: needinfo?(cpearce)
(In reply to Alastor Wu [:alwu] from comment #11)
> (In reply to Chris Pearce (:cpearce) from comment #9)
> > ::: dom/html/HTMLMediaElement.cpp:2491
> > The media load algorithm is asynchronous. So you should not be able to
> > observe the load failing with MEDIA_ERR_SRC_NOT_SUPPORTED this early in the
> > load algorithm; you must wait until the event loop has done another pass and
> > the first SelectResource task has run before an error can be thrown.
> > 
> > So the only place you should need to call
> > OpenUnsupportedMediaWithExtenalAppIfNeeded() is at the end of
> > HTMLMediaElement::NoSupportedMediaSourceError().
> > 
> > Right?
> > 
> > Assuming that's the case, you might be able to simplify the logic there too.
> > That is, in NoSupportedMediaSourceError() you know that the conditions you
> > check in IsUnsupportedMediaType() will always be true.
> 
> No, here OpenUnsupportedMediaWithExtenalAppIfNeeded() is to handle another
> case, that is start playing after load failed.

OK. Please add a comment explaining that.

r+ with that, and other issued addressed.
Flags: needinfo?(cpearce)
Comment on attachment 8765775 [details]
Bug 1282410 - part1 : open unsupported type media.

https://reviewboard.mozilla.org/r/60972/#review58486

r+ with comment explaining why we need to check for unsupported stream in PlayInternal(), and other issues addressed.
Attachment #8765775 - Flags: review- → review+
Comment on attachment 8765775 [details]
Bug 1282410 - part1 : open unsupported type media.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60972/diff/1-2/
Comment on attachment 8765776 [details]
Bug 1282410 - part2 : open media with external apps.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60974/diff/1-2/
Comment on attachment 8765777 [details]
Bug 1282410 - part3 : add log.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60976/diff/1-2/
Hi, Margaret,
Do you have time to help me review the patch?
If not, could you help me to forward it to other related person?
Thanks :)
Flags: needinfo?(margaret.leibovic)
Comment on attachment 8765776 [details]
Bug 1282410 - part2 : open media with external apps.

https://reviewboard.mozilla.org/r/60974/#review59088

Sorry for the slow review! This seems reasonable to me, but I think you may want to change the `useCapture` parameter to true.

::: mobile/android/chrome/content/browser.js:4688
(Diff revision 2)
>        BrowserApp.deck.addEventListener("touchend", this, true);
>      }
>  
>      BrowserApp.deck.addEventListener("DOMUpdatePageReport", PopupBlockerObserver.onUpdatePageReport, false);
>      BrowserApp.deck.addEventListener("MozMouseHittest", this, true);
> +    BrowserApp.deck.addEventListener("OpenMediaWithExtenalApp", this, false);

The other listeners here set the `useCapture` parameter to true. I seem to recall there are issues with event bubbling and our browser deck, but I don't remember exactly what the issue is. Should you be using true here as well?
Attachment #8765776 - Flags: review?(margaret.leibovic) → review+
Flags: needinfo?(margaret.leibovic)
(In reply to :Margaret Leibovic from comment #18)
> The other listeners here set the `useCapture` parameter to true. I seem to
> recall there are issues with event bubbling and our browser deck, but I
> don't remember exactly what the issue is. Should you be using true here as
> well?

OK, I'll modify that, thanks!
Comment on attachment 8765775 [details]
Bug 1282410 - part1 : open unsupported type media.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60972/diff/2-3/
Comment on attachment 8765776 [details]
Bug 1282410 - part2 : open media with external apps.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60974/diff/2-3/
Comment on attachment 8765777 [details]
Bug 1282410 - part3 : add log.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60976/diff/2-3/
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/339b5c5d3595
part1 : open unsupported type media. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/577309fc4bbb
part2 : open media with external apps. r=Margaret
https://hg.mozilla.org/integration/autoland/rev/39c367ecc144
part3 : add log. r=cpearce
Just noticed a small typo in the event name:

https://hg.mozilla.org/integration/autoland/rev/339b5c5d3595#l1.81
https://hg.mozilla.org/integration/autoland/rev/577309fc4bbb#l1.12

s/Extenal/External/

(though it should still work as-is)
Ah, sorry for that, I would modify it on another bug (bug1284754).
Alastor, 
Good job! I think this is worth uplifting to Firefox 49. 
Let's play HLS on nightly for a couple of days. 
If it is no problem, let's uplift it to 49.
status-firefox49: --- → affected
I was testing bug 943584 and was presented with this (attached) screen to download the m3u8 file. (Note the screenshot is from NASA but same result on CBC). Is this the expected behavior now?
(In reply to Adam Stevenson from comment #28)
> I was testing bug 943584 and was presented with this (attached) screen to
> download the m3u8 file. (Note the screenshot is from NASA but same result on
> CBC). Is this the expected behavior now?

Yes, it's correct behavior. The external apps service would detect which apps can be used to play m3u8 format. The external app menu might not have the useful options if your phone doesn't have any app can be used to play that format.

In your case, it seems you can only use "photo" app to play the file or choose to download the file. 

---

In addition, notice that, there are two bugs related with handling m3u8 format.

(1) the bug780379 is to show the external apps menu when users click the link with m3u8 format, and the menu would be showed as dialog. Eg. the attachment in comment28.

(2) this bug is to show the external apps menu when user want to play the media element with unsupported types, not only for m3u8 format. And the menu would be showed as the external app list. Eg. [1]

[1] https://photos.google.com/share/AF1QipP4oy8HokC3UShu3d8EfxCF6fFKVe6xhqqrBMN-lV_ZtShh77cChy8xvLcs2Wo7BA/photo/AF1QipM7xIKsTshGV2LZCZWga2B0Vo06Xcaj1csejqan?key=LWI4UVJVS1Vaam9hRVpGUVVnb1JfQjZTX0JvUG53
It's a bit confusing that Firefox is in the list.
Attached image Fennec HLS flow — 
Hm, this is going to be confusing for our users I think. 

When I visit the HLS video page and click play, I'm prompted to click on Firefox to open the video and then I'm prompted to either click Firefox (which downloads a m3u8 file) or I click on Photos (which results in a "photo" that it can't display).

I guess stock Android doesn't have an app that I can just play the HLS stream?
Comment on attachment 8765775 [details]
Bug 1282410 - part1 : open unsupported type media.

Why are these changes to the media element needed? We already have much the same mechanism with the "decoder-doctor-notification" notifications that we could listen to.
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #32)
> Comment on attachment 8765775 [details]
> Bug 1282410 - part1 : open unsupported type media.
> 
> Why are these changes to the media element needed? We already have much the
> same mechanism with the "decoder-doctor-notification" notifications that we
> could listen to.

I don't know we have this notifications, after tracing the code, this notification seems not related with the media element's play(). 

Could you help me explain how it works?

The reason of binding with media element's codes is because the external app menu should be showed every time users start playing the media element instead of first time playing, so we need to check the media element's status in play().
(In reply to Mike Taylor [:miketaylr] from comment #31)
> Created attachment 8768475 [details]
> Fennec HLS flow
> 
> Hm, this is going to be confusing for our users I think. 
> 
> When I visit the HLS video page and click play, I'm prompted to click on
> Firefox to open the video and then I'm prompted to either click Firefox
> (which downloads a m3u8 file) or I click on Photos (which results in a
> "photo" that it can't display).
> 
> I guess stock Android doesn't have an app that I can just play the HLS
> stream?

yes, you need to get the app in advance which can play the HLS stream. eg. VLC, MX player...e.t.c
Are we sure we actually want this? I'm afraid most users will just be confused and won't be able to use this feature.
(In reply to Marco Castelluccio [:marco] from comment #35)
> Are we sure we actually want this? I'm afraid most users will just be
> confused and won't be able to use this feature.
This is not new. We used to use this kind of mechanism to play the content on a link we cannot play as Bug 941351 comment 0 mentioned. 
This bug is to handle video src="foo.m3u8" and other unsupported formats.
(In reply to Marco Castelluccio [:marco] from comment #35)
> Are we sure we actually want this? I'm afraid most users will just be
> confused and won't be able to use this feature.

If we want to make the UX more friendly, we should file some follow-up bugs to modify our front-end design. Eg. we can show the the msg "unsupported type, need to use other apps to open it" instead of showing "can't playback this unsupported type" with error image on our video-control interface.
(In reply to Blake Wu [:bwu][:blakewu] from comment #36)
> (In reply to Marco Castelluccio [:marco] from comment #35)
> > Are we sure we actually want this? I'm afraid most users will just be
> > confused and won't be able to use this feature.
> This is not new. We used to use this kind of mechanism to play the content
> on a link we cannot play as Bug 941351 comment 0 mentioned. 
> This bug is to handle video src="foo.m3u8" and other unsupported formats.

The situation of links vs media elements looks different to me, why should we adopt the
same behavior in the two situations?
Users clicking on a link might expect the link to be opened in an external application,
users clicking play on a video might not expect that (and, the dialog we show if they
don't have an application installed to handle the content is confusing).

How frequent are links to m3u8 files compared to media elements with m3u8?

(In reply to Alastor Wu [:alwu] from comment #37)
> (In reply to Marco Castelluccio [:marco] from comment #35)
> > Are we sure we actually want this? I'm afraid most users will just be
> > confused and won't be able to use this feature.
> 
> If we want to make the UX more friendly, we should file some follow-up bugs
> to modify our front-end design. Eg. we can show the the msg "unsupported
> type, need to use other apps to open it" instead of showing "can't playback
> this unsupported type" with error image on our video-control interface.

Shouldn't we ask for some UX feedback before implementing these kind of changes?
(In reply to Marco Castelluccio [:marco] from comment #38)
> Shouldn't we ask for some UX feedback before implementing these kind of
> changes?

I just provide an idea for that, we don't have a plan to change the front-end design now :p
(In reply to Alastor Wu [:alwu] from comment #39)
> (In reply to Marco Castelluccio [:marco] from comment #38)
> > Shouldn't we ask for some UX feedback before implementing these kind of
> > changes?
> 
> I just provide an idea for that, we don't have a plan to change the
> front-end design now :p

What I meant was: "shouldn't we ask for some UX feedback before implementing the kind of changes included in this bug?".
Flags: needinfo?(alam)
As for comment 19 in this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=943584#c19

Blake, in Canada, I installed VLC & I tried to reproduce these steps (for Nightly) and although I can now select "VLC" as an option, the VLC icon shows up briefly in the status bar on top but nothing happens after that. VLC does not start.

I actually noticed an even worse UX behaviour on Aurora and Release; the pre-ad plays fine and then it shows the CBC can't play image without even giving me the option to open it in another app.
(In reply to Marco Castelluccio [:marco] from comment #40)
> (In reply to Alastor Wu [:alwu] from comment #39)
> > (In reply to Marco Castelluccio [:marco] from comment #38)
> > > Shouldn't we ask for some UX feedback before implementing these kind of
> > > changes?
> > 
> > I just provide an idea for that, we don't have a plan to change the
> > front-end design now :p
> 
> What I meant was: "shouldn't we ask for some UX feedback before implementing
> the kind of changes included in this bug?".
Agree. It would be better to have UX feedback here.
(In reply to Barbara Bermes [:barbara] from comment #41)
> As for comment 19 in this bug:
> https://bugzilla.mozilla.org/show_bug.cgi?id=943584#c19
> 
> Blake, in Canada, I installed VLC & I tried to reproduce these steps (for
> Nightly) and although I can now select "VLC" as an option, the VLC icon
> shows up briefly in the status bar on top but nothing happens after that.
> VLC does not start.
> 
> I actually noticed an even worse UX behaviour on Aurora and Release; the
> pre-ad plays fine and then it shows the CBC can't play image without even
> giving me the option to open it in another app.
If you are not using stock Android (most users would not use stock Android), normally you can find a player to play it without installing any other App, like VLC. 
Could you attache your screen? Or record a video for us to check it.
Thanks.
Flags: needinfo?(bbermes)
(In reply to Blake Wu [:bwu][:blakewu] from comment #43)
> (In reply to Barbara Bermes [:barbara] from comment #41)
> > As for comment 19 in this bug:

> If you are not using stock Android (most users would not use stock Android),
> normally you can find a player to play it without installing any other App,
> like VLC. 
On the Nexus 6p there is no player for HLS (unless you install VLC) and that's about as stock as you can get.
Not an ideal scenario, I can tell.

We should add probes to the intent when it's being used. Once the video finished, it should normally bounce you back into the browser. Also here, is there anything we can pass pack to Telemetry, so we get a sense of how people are watching HLS videos on Firefox via intents?

How about on a Samsung device (that's where most of our users are).

I tried: http://fish.schou.me 

a) Nexus 5 (stock): Nightly opened VLC
b) Samsung (non-stock) Nightly didn't offer me a player to view the video in, only showed error page, so there seems to be no default player available either.
c) Samsung (non-stock) Internet browser didn't offer me a player to view the video in, only showed error page, so there seems to be no default player available either.

https://bugzilla.mozilla.org/attachment.cgi?id=8770640
(In reply to Barbara Bermes [:barbara] from comment #46)
> Not an ideal scenario, I can tell.
> 
> We should add probes to the intent when it's being used. Once the video
> finished, it should normally bounce you back into the browser. Also here, is
> there anything we can pass pack to Telemetry, so we get a sense of how
> people are watching HLS videos on Firefox via intents?
> 
> How about on a Samsung device (that's where most of our users are).
> 
> I tried: http://fish.schou.me 
> 
> a) Nexus 5 (stock): Nightly opened VLC
I don't think VLC is on the stock image, so a true stock experience on the Nexus 5 would be like (b)
Also, on the Nexus 6p, it is the same, (b) if you haven't installed anything extra, (a) if you have installed VLC.
> b) Samsung (non-stock) Nightly didn't offer me a player to view the video
> in, only showed error page, so there seems to be no default player available
> either.
> c) Samsung (non-stock) Internet browser didn't offer me a player to view the
> video in, only showed error page, so there seems to be no default player
> available either.
> 
> https://bugzilla.mozilla.org/attachment.cgi?id=8770640
Talked to blassey yesterday, we should back this out, not ship but rather use the following solution instead (once snorp reviewed it): https://bugzilla.mozilla.org/show_bug.cgi?id=1286133#c2
Flags: needinfo?(snorp)
Flags: needinfo?(blassey.bugs)
Flags: needinfo?(alam)
(In reply to Barbara Bermes [:barbara] from comment #48)
> Talked to blassey yesterday, we should back this out, not ship but rather
> use the following solution instead (once snorp reviewed it):
> https://bugzilla.mozilla.org/show_bug.cgi?id=1286133#c2

what info do you need?
Flags: needinfo?(blassey.bugs) → needinfo?(bbermes)
Just wanted to keep you in the loop and based on your bug line (use needinfo?)
Flags: needinfo?(bbermes)
I'll backout.
Flags: needinfo?(s.kaspari)
Flags: needinfo?(s.kaspari)
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/c28668cae17e
part1 : open unsupported type media. r=cpearce
Blocks: 1302362
Tested using:
Device: One A2001 (Android 6.0.1)
Build: Firefox for Android 50 Beta 1

Tapping the play button for all videos from the test page will open and play the video.

But after playing them all, "Video format or MIME type is not supported" message is displayed. The videos cannot be played once again.
Because these patches didn't be landed into m-c completely, we used the way in bug1286133 which doesn't allow user to replay media again.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: