936851 - VideoPlayer doesn't work at all, preventing video playback on devices without YouTube application

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

Tested in Beta and Nightly, and no YouTube video will play. Doesn't crash, but only get a blank screen.

Need to get some logs for this; needinfo for myself.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

For those following along at home, the Kindle's don't have a YouTube player app, so we ship our own barebones VideoPlayer activity. It tries to parse out the video stream from the YouTube URL.

Comment 2

[Aaron Train [:aaronmt]]

What happened to VideoPlayer.java (reminisce down bug 847839 lane via patch 1a)?

Comment 3

[Richard Newman [:rnewman]]

My hypothesis is that it stopped working (or never worked), because we almost exclusively all run devices with YouTube players and Flash. Either that, or I regressed something in the stonkingly simple patch in Bug 886014.

We'll see when I sit down tomorrow and get a log...

Comment 4

[Richard Newman [:rnewman]]

exception
android.os.NetworkOnMainThreadException
	at android.os.StrictMode$AndroidBlockGuardPolicy.onNetwork(StrictMode.java:1117)
	at java.net.InetAddress.lookupHostByName(InetAddress.java:385)
	at java.net.InetAddress.getAllByNameImpl(InetAddress.java:236)
	at java.net.InetAddress.getAllByName(InetAddress.java:214)
	at libcore.net.http.HttpConnection.<init>(HttpConnection.java:70)
	at libcore.net.http.HttpConnection.<init>(HttpConnection.java:51)
	at libcore.net.http.HttpConnection$Address.connect(HttpConnection.java:359)
	at libcore.net.http.HttpConnectionPool.get(HttpConnectionPool.java:86)
	at libcore.net.http.HttpConnection.connect(HttpConnection.java:128)
	at libcore.net.http.HttpEngine.openSocketConnection(HttpEngine.java:316)
	at libcore.net.http.HttpEngine.connect(HttpEngine.java:311)
	at libcore.net.http.HttpEngine.sendSocketRequest(HttpEngine.java:290)
	at libcore.net.http.HttpEngine.sendRequest(HttpEngine.java:240)
	at libcore.net.http.HttpURLConnectionImpl.getResponse(HttpURLConnectionImpl.java:282)
	at libcore.net.http.HttpURLConnectionImpl.getInputStream(HttpURLConnectionImpl.java:177)
	at org.mozilla.gecko.VideoPlayer.getSpecFromYouTubeVideoID(VideoPlayer.java:69)
	at org.mozilla.gecko.VideoPlayer.onCreate(VideoPlayer.java:50)

Comment 5

[Richard Newman [:rnewman]]

Attachment: Part 1: initial fixes. v1

It turns out that VideoPlayer is borked for more than just StrictMode violations.

I fixed that, and now it seems like YouTube has changed their URI format. So I fixed that. And now the player UI loads some of the video, doesn't seem to play anything, and doesn't correctly respond to rotations.

Here's a patch, but we're not done yet.

mfinkle gets review, 'cos he's the only person in blame who's still on the team!

Comment 6

[Richard Newman [:rnewman]]

Note that until we finish this bug (i.e., figure out why videos aren't playing), I suggest we don't ship VideoPlayer at all, and instead fire a VIEW intent to let the system browser or other apps try.

People do sideload Firefox onto these devices, so continuing to ship something totally broken seems like a bad choice.

Comment 7

[Richard Newman [:rnewman]]

Also need to check this on an AOSP device other than the Kindle, see if it's just as borked there.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 831239 [details] [diff] [review]
Part 1: initial fixes. v1

The only part is was wondering about is the ThreadUtils part. GeckoApp sets some of the ThreadUtils data members, IIRC. Are we OK to be using ThreadUtils from VideoPlayer?

I guess those statics would have been setup since VideoPlayer is always launched from Fennec, which initializes them.

Comment 9

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #8)

> I guess those statics would have been setup since VideoPlayer is always
> launched from Fennec, which initializes them.

The only thing that wouldn't be set is sUiThread -- the background thread will be created by GeckoBackgroundThread if needed.

But yes, so long as GeckoApp runs first, we should be fine here.

Comment 10

[Richard Newman [:rnewman]]

Proposition: we should throw away VideoPlayer and ship

http://code.google.com/p/android-youtube-player/wiki/OpenYouTubePlayerActiviyInstructions

Alternatively, we should investigate whether playing the content from the mobile version of youtube.com is satisfactory.

Thoughts?

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #10)
> Proposition: we should throw away VideoPlayer and ship
> 
> http://code.google.com/p/android-youtube-player/wiki/
> OpenYouTubePlayerActiviyInstructions
What makes you think this is going to be maintained?
 
> Alternatively, we should investigate whether playing the content from the
> mobile version of youtube.com is satisfactory.
Requesting the HTML5 video version of YouTube would be more of a product call. Previously we've dcome the conclusion that the YouTube App is a better experience (and available to the vase majority of our users).

But... One solution may be to, when there is no handler for vnd-youtube, reload the page in a way to get the HTML5 version (I think you can do something like http://www.youtube.com/embed/<vidoe ID>?html5=1)

That said, not all videos will be available in HTML5, you know DRM and all that.
> 
> Thoughts?

Comment 12

[Richard Newman [:rnewman]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #11)

> What makes you think this is going to be maintained?

It's going to be maintained at least as much as VideoPlayer is.


> But... One solution may be to, when there is no handler for vnd-youtube,
> reload the page in a way to get the HTML5 version (I think you can do
> something like http://www.youtube.com/embed/<vidoe ID>?html5=1)

Yeah, that's the situation in which I'm suggesting this. Essentially, if you're on a device without a YouTube player, we should try playing the video inside the browser, rather than launching a separate activity, given that the separate activity doesn't actually seem to work.


> That said, not all videos will be available in HTML5, you know DRM and all
> that.

But that's much the same situation we have with VideoPlayer, right? -- sometimes the info request comes back with "This video isn't available due to...", or just no stream. I had a one-in-three success rate when I was trying to find a video to test against.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

I agree with the plan you two are forming:
1. Use vnd-youtube to spawn out to the YouTube player
2. If not available, reload to get the HTML5 version and play it in the browser
3. If no HTML5 version, we apologize

I think we still send the XUL Fennec UA to YouTube so we get vnd-youtube streams. Disabling that UA override should get the HTML5 version. It works for embedded YouTube videos.

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #13)
> I agree with the plan you two are forming:
> 1. Use vnd-youtube to spawn out to the YouTube player
> 2. If not available, reload to get the HTML5 version and play it in the
> browser
> 3. If no HTML5 version, we apologize
> 
> I think we still send the XUL Fennec UA to YouTube so we get vnd-youtube
> streams. Disabling that UA override should get the HTML5 version. It works
> for embedded YouTube videos.

going a step further, if there is no YouTube player available, set a pref to override the UA override in general so we only have to do this dance once

Comment 16

[Albert Scheiner [:alberts]]

To avoid further confusion (see bug 906869 comment 23) would it make sense to adjust the description of this bug?

Comment 17

[Richard Newman [:rnewman]]

I'm working on this.

The embedding video player is fullscreen, lacks comments, etc; we either need to find a different invocation (such as loading without a special UA), or continue using the action icon in the URL bar to launch the 'player'.

More as I test.

Comment 18

[Richard Newman [:rnewman]]

Tapping a YouTube link:

07-30 12:46:44.397 I/GeckoAppShell(22948): getHandlersForURL: vnd.youtube,
07-30 12:46:44.437 I/GeckoAppShell(22948): getHandlersForURL: vnd.youtube,
07-30 12:46:44.437 I/GeckoAppShell(22948): openUriExternal: vnd.youtube:SCLU3v_AY4Q?vndapp=youtube_mobile&vndclient=mv-google&vndel=watch&vnddnc=1

Comment 19

[Richard Newman [:rnewman]]

Attachment: Part 1: remove VideoPlayer. v1

This stubs out the real integration point; see Part 2.

Comment 20

[Richard Newman [:rnewman]]

Attachment: Part 2: delegate to Fennec itself for YouTube HTML5 video instead of VideoPlayer. v1

This works for me. Loading a YouTube video page gives you the usual clickable video. Tapping that opens the HTML player rather than opening the YouTube app; you can watch the video then hit Back to return to the video page. Really nice.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8465050 [details] [diff] [review]
Part 2: delegate to Fennec itself for YouTube HTML5 video instead of VideoPlayer. v1

>diff --git a/mobile/android/base/GeckoAppShell.java b/mobile/android/base/GeckoAppShell.java
>     static String[] getHandlersForURL(String aURL, String aAction) {
>+        Log.v(LOGTAG, "getHandlersForURL: " + aURL + ", " + aAction);

Don't leak URIs. Let's just remove it.

>     public static boolean openUriExternal(String targetURI,
>                                           String mimeType,
>               @OptionalGeneratedParameter String packageName,
>               @OptionalGeneratedParameter String className,
>               @OptionalGeneratedParameter String action,
>               @OptionalGeneratedParameter String title) {
>+        Log.v(LOGTAG, "openUriExternal: " + targetURI);
>+

Don't leak URIs. Let's just remove it.

>     public static Intent getShareIntent(final Context context,
>                                         final String targetURI,
>                                         final String mimeType,
>                                         final String title) {
>+        Log.v(LOGTAG, "getShareIntent: " + targetURI);
>+

Don't leak URIs. Let's just remove it.

>+            if (youtubeURI != null) {
>+                Log.v(LOGTAG, "Opening YouTube URI: " + youtubeURI);

Don't leak URIs. Let's just remove it.

>+    private static Uri getYouTubeHTML5URI(final Uri uri) {
>+        if (uri == null) {
>+            return null;
>+        }
>+
>+        Log.v(LOGTAG, "Incoming URI: " + uri);

Don't leak URIs. Let's just remove it.

Comment 22

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/2c54d1bdd339
https://hg.mozilla.org/integration/fx-team/rev/4cbef502c1dc

Comment 23

[Richard Newman [:rnewman]]

Relnote: "Fix YouTube playback on devices without YouTube app."

Assuming this verifies fine, I'd want this for 33. Any contrary opinions? mfinkle?

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2c54d1bdd339
https://hg.mozilla.org/mozilla-central/rev/4cbef502c1dc

Comment 25

[Aaron Train [:aaronmt]]

> # pm disable com.google.android.youtube

* Played video directly on YouTube on my Nexus 5 (4.4.4)
* Embedded YouTube works

Comment 26

[Richard Newman [:rnewman]]

Comment on attachment 8465050 [details] [diff] [review]
Part 2: delegate to Fennec itself for YouTube HTML5 video instead of VideoPlayer. v1

Approval Request Comment
[Feature/regressing bug #]:
  Forever.

[User impact if declined]:
  Crashes when attempting to play YouTube videos on AOSP/Kindle Fire.

[Describe test coverage new/current, TBPL]:
  Manually verified.

[Risks and why]: 
  We're loosely targeting 33 as our Kindle-supporting release, and this is actually quite serious for non-Google-branded users, too.

[String/UUID change made/needed]:
  None.

Comment 27

[Aaron Train [:aaronmt]]

(In reply to Richard Newman [:rnewman] from comment #23)
> Relnote: "Fix YouTube playback on devices without YouTube app."

It's a subpar experience in comparison to using the native YouTube application in entirety, not sure how you would describe that.

Comment 28

[Richard Newman [:rnewman]]

(In reply to Aaron Train [:aaronmt] from comment #27)

> It's a subpar experience in comparison to using the native YouTube
> application in entirety, not sure how you would describe that.

I think it's implied that we can't send you to the YouTube app… we don't suggest "reimplemented the entire YouTube experience".

Beyond that, I actually feel that the experience is pretty good! It does fullscreen, and even does casting, I think.

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #28)
> (In reply to Aaron Train [:aaronmt] from comment #27)
> 
> > It's a subpar experience in comparison to using the native YouTube
> > application in entirety, not sure how you would describe that.
> 
> I think it's implied that we can't send you to the YouTube app… we don't
> suggest "reimplemented the entire YouTube experience".
> 
> Beyond that, I actually feel that the experience is pretty good! It does
> fullscreen, and even does casting, I think.

Agreed. It's pretty good.

Comment 30

[Richard Newman [:rnewman]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/ae5bc80c1b9e
http://hg.mozilla.org/releases/mozilla-aurora/rev/3dc4eb0e81c6

Comment 31

[Richard Newman [:rnewman]]

https://hg.mozilla.org/mozilla-aurora/rev/2515f2da59d5

Comment 32

[Richard Newman [:rnewman]]

Comment on attachment 8465050 [details] [diff] [review]
Part 2: delegate to Fennec itself for YouTube HTML5 video instead of VideoPlayer. v1

Mark and I discussed getting this into Beta.

It's a safe change: we're happy with it in Nightly and on Aurora.

It addresses an obvious and painful crash for not only Kindle but also other AOSP users, which is a pretty big market (in some locales, bigger than Google-licensed Android; cf Xiaomi).

And the change only affects those users: even if this is buggy, it can't be worse than our current "just crash" behavior.

Comment 33

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in:
Build: Firefox for Android 33.0a2 (2014-08-11)
Device: Kindle Fire HD 7"

Comment 34

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8465050 [details] [diff] [review]
Part 2: delegate to Fennec itself for YouTube HTML5 video instead of VideoPlayer. v1

Ouch that we need to have more site specific code in Fennec. I agree with the risk assessment and this it is worth trying to push this out sooner given the limited impact that it should have on the general Fennec population. Let's get this into beta6. beta+

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e9fc6b3efaf3
https://hg.mozilla.org/releases/mozilla-beta/rev/086068fe1b98

Comment 36

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in:
Build: Firefox for Android 32 Beta 6
Device: Kindle Fire HD 7"