705572 - Kindle Fire: YouTube videos do not open in unavailable YouTube App

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Alex Keybl [:akeybl]]

When trying to play a YouTube video, on the Kindle Fire I get a dialog that this action will open an external app, but when I hit "OK" no app is launched. There is no YouTube app available on the Kindle Fire.

Comment 1

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

Attachment: patch to ship a simple youtube player

Comment 2

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

Attachment: patch to ship a simple youtube player

Comment 3

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

pushed to try https://tbpl.mozilla.org/?tree=Try&rev=7feacaa09970

Comment 4

[Mozilla RelEng Bot]

Try run for 7feacaa09970 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=7feacaa09970
Results (out of 1 total builds):
    failure: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-7feacaa09970

Comment 5

[Mozilla RelEng Bot]

Try run for b5fd4bb0ee8c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b5fd4bb0ee8c
Results (out of 2 total builds):
    failure: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b5fd4bb0ee8c

Comment 6

[Alex Keybl [:akeybl]]

(In reply to Brad Lassey [:blassey] from comment #2)
> Created attachment 578978 [details] [diff] [review] [diff] [details] [review]
> patch to ship a simple youtube player

In my email I implied that this youtube player was not the same as presenting a webm video. Is that true? What video are we sourcing here?

Comment 7

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

(In reply to Alex Keybl [:akeybl] from comment #6)
> (In reply to Brad Lassey [:blassey] from comment #2)
> > Created attachment 578978 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > patch to ship a simple youtube player
> 
> In my email I implied that this youtube player was not the same as
> presenting a webm video. Is that true? What video are we sourcing here?

this patch is using the h264 stream.

Comment 8

[Doug Turner (:dougt)]

Comment on attachment 578978 [details] [diff] [review]
patch to ship a simple youtube player

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

::: embedding/android/AndroidManifest.xml.in
@@ +125,5 @@
>                  <category android:name="android.intent.category.DEFAULT" />
>              </intent-filter>
>          </activity>
> +	<activity android:name="org.mozilla.gecko.YouTubePlayer"
> +                  android:label="YouTubePlayer"

localized string.  Can we not call this YouTube specifically?  Maybe we can reuse this activity for different players.  Also, it gets out out of worrying about Trademarks.

::: embedding/android/GeckoAppShell.java
@@ +813,5 @@
> +                String[] handlers = getHandlersForURL(aUriSpec, aAction);
> +                if (handlers.length == 0) {
> +                    intent = new Intent(Intent.ACTION_MAIN);
> +                    intent.setClassName(GeckoApp.mAppContext.getPackageName(),
> +                                        "org.mozilla.gecko.YouTubePlayer");

Again, the name of this class should probably be VideoPlayer or something more general.

::: embedding/android/YouTubePlayer.java
@@ +1,1 @@
> +package org.mozilla.gecko;

missing license header.

@@ +40,5 @@
> +    try {
> +      String info_uri = "http://www.youtube.com/get_video_info?&video_id=" + id;
> +      URL url = new URL(info_uri);
> +      URLConnection urlConnection = url.openConnection();
> +      InputStream in = new BufferedInputStream(urlConnection.getInputStream());

This is all blocking.  Although, I didn't see a real problem, I am not sure we should be doing this on the main thread.  It could be that I am on a very fast network and didn't see any hang or something.

@@ +45,5 @@
> +      try {
> +        int i;
> +        StringBuffer sb = new StringBuffer();
> +        while (-1 != (i = in.read()))
> +          sb.append((char)i);

Is this the best way to convert a InputStream to a StringBuffer?  Do you know the max size of the response and preallocate a byte array?

@@ +52,5 @@
> +        for (i = 0; i < data.length; i++) {
> +          String[] key_val = data[i].split("=");
> +	  if (key_val.length == 2)
> +	    map.put(key_val[0], URLDecoder.decode(key_val[1]));
> +        }

I really don't know what the response of this get is.  I kinda wish it was just a json blob.  If we had that, most of this parsing goes away.  Is there such a thing?  http://code.google.com/intl/da-DK/apis/youtube/2.0/developers_guide_json.html

Comment 9

[Alex Keybl [:akeybl]]

Here's Tony's feedback from the try build:

"Tested against brad's 12/4 try server build + youtube plugin.

- launching youtube video prompts "Launch appliction - This link needs to be opened with an application".  
** it would be nice if we can offer the Android sharing menu with a youtube player option to choose from.

- video playback in youtube player is not centered.  video is upper left justified.  exists on any orientation.

- find a youtube video where the screen image is maximizes the viewport.   Video playback does not display in full screen.

- switch video from landscape to portrait orientation during playback.   Not only does the video recalibrate viewport, but at times will leave black boxes

- Youtube player video (from Silk) will hide the system menu at the bottom.   Loading it from fennec does not have the minimized system control menu.

- Back button on youtube player doesnt take one click to return back to Fennec.  If you make touch actions within the video player controls, those will be recorded part of the history.  (eg. click play, pause, then play - 3 touch actions.  Now if you hit back, you'll have to triple click it before returning to fennec)   The more users click the controls, the less they will know how to get back to fennec due to larger click history."

Comment 10

[Alex Keybl [:akeybl]]

(In reply to Alex Keybl [:akeybl] from comment #9)
> Here's Tony's feedback from the try build:
> 
> "Tested against brad's 12/4 try server build + youtube plugin.
> 
> - launching youtube video prompts "Launch appliction - This link needs to be
> opened with an application".  
> ** it would be nice if we can offer the Android sharing menu with a youtube
> player option to choose from.

We're not considering this a blocker.

> - video playback in youtube player is not centered.  video is upper left
> justified.  exists on any orientation.

This is fixed in the latest patch.

> - find a youtube video where the screen image is maximizes the viewport.  
> Video playback does not display in full screen.

Can you clarify Tony?

> - switch video from landscape to portrait orientation during playback.   Not
> only does the video recalibrate viewport, but at times will leave black boxes

We're not considering this a blocker.

> - Youtube player video (from Silk) will hide the system menu at the bottom. 
> Loading it from fennec does not have the minimized system control menu.

We're not considering this a blocker.

> - Back button on youtube player doesnt take one click to return back to
> Fennec.  If you make touch actions within the video player controls, those
> will be recorded part of the history.  (eg. click play, pause, then play - 3
> touch actions.  Now if you hit back, you'll have to triple click it before
> returning to fennec)   The more users click the controls, the less they will
> know how to get back to fennec due to larger click history."

We're going to try to get this fixed before landing - hopefully today before going to build for beta.

Comment 11

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

Comment on attachment 578978 [details] [diff] [review]
patch to ship a simple youtube player


>diff --git a/embedding/android/AndroidManifest.xml.in b/embedding/android/AndroidManifest.xml.in

>+	<activity android:name="org.mozilla.gecko.YouTubePlayer"
>+                  android:label="YouTubePlayer"

Doesn't look like we need a "label" and lets use "org.mozilla.gecko.VideoPlayer" here. Therefore we won't have any strings to localize

In fact, I like using "VideoPlayer" and variants in all places we are currently using "YouTubePlayer", just to keep thing general.

Your indenting in this file is a bit off too

>diff --git a/embedding/android/GeckoAppShell.java b/embedding/android/GeckoAppShell.java

>+                    intent.setClassName(GeckoApp.mAppContext.getPackageName(),
>+                                        "org.mozilla.gecko.YouTubePlayer");

org.mozilla.gecko.VideoPlayer

>diff --git a/embedding/android/Makefile.in b/embedding/android/Makefile.in

>+  YouTubePlayer.java \

VideoPlayer.java

>+  res/layout/youtubeplayer.xml \

videoplayer.xml

>diff --git a/embedding/android/YouTubePlayer.java b/embedding/android/YouTubePlayer.java

Indenting in here is 2 spaces, but we use 4 for Java files

>+public class YouTubePlayer extends Activity

VideoPlayer

>+{
>+  /** Called when the activity is first created. */
>+  @Override
>+  public void onCreate(Bundle savedInstanceState)
>+  {
>+    super.onCreate(savedInstanceState);
>+    setContentView(R.layout.youtubeplayer);
>+    mVideoView = (VideoView) findViewById(R.id.VideoView);
>+    MediaController mediaController = new MediaController(this);
>+    mediaController.setAnchorView(mVideoView);
>+    // Set video link (mp4 format )
>+    Intent intent = getIntent();
>+    Uri data = intent.getData();
>+    String ssp = data.getSchemeSpecificPart();
>+    String id = ssp.substring(0, ssp.indexOf('?'));
>+    String spec = getSpecFromVideoID(id);

Do you want to verify that spec is not "" (failure case)

>+  String getSpecFromVideoID(String id) {

Can you add an example of what the URL returns? So people know what the format of the text is we are parsing here

>+        for (i = 0; i < data.length; i++) {
>+          String[] key_val = data[i].split("=");
>+	  if (key_val.length == 2)

TAB snuck in here

>+	    if (key_val.length == 2) {
>+	      Log.i("YouTubePlayer", key_val[0] + ": " + URLDecoder.decode(key_val[1]));

VideoPlayer

>+	  if (mime != null && mime.startsWith("video/mp4") && uri != null) {
>+	    spec = uri;
>+	  }
>+	}

TAB here too

>+    } catch (Exception e) {
>+      Log.e("YouTubePlayer", "exception", e);

VideoPlayer

I agree with Doug about moving to a simpler parsing format, like JSON - but I don't think we should hold up landing and testing the feature on that

Let's file a followup to investigate JSON or other formats.

>diff --git a/embedding/android/resources/layout/youtubeplayer.xml b/embedding/android/resources/layout/youtubeplayer.xml

>+<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
>+    android:orientation="vertical"
>+    android:layout_width="fill_parent"
>+    android:layout_height="fill_parent"
>+    >
>+<VideoView android:layout_height="fill_parent"
>+android:layout_width="fill_parent" android:id="@+id/VideoView"></VideoView>

VideoView needs some indenting love

r+ with the nits fixed

Comment 12

[Tony Chung [:tchung]]

(In reply to Alex Keybl [:akeybl] from comment #10)
> > - find a youtube video where the screen image is maximizes the viewport.  
> > Video playback does not display in full screen.
> 
> Can you clarify Tony?
> >

yeah, poorly worded.  basically, video does not stretch entirely across the device display, when redirected from fennec.

It does stretch correctly if you redirect from silk.

Tony

Comment 13

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

> - Back button on youtube player doesnt take one click to return back to
> Fennec.  If you make touch actions within the video player controls, those
> will be recorded part of the history.  (eg. click play, pause, then play - 3
> touch actions.  Now if you hit back, you'll have to triple click it before
> returning to fennec)   The more users click the controls, the less they will
> know how to get back to fennec due to larger click history."

I don't see what you describe here. If the controls are up, hitting back dismisses them. Hitting back when the controls aren't up brings you back to fennec.

Comment 14

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

Attachment: patch

Comment 15

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

Attachment: patch

Doug suggested using android.net.Uri to do the parsing, this patch does that

Comment 16

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

Comment on attachment 579589 [details] [diff] [review]
patch

>@@ -34,11 +34,11 @@
>     <uses-feature android:name="android.hardware.camera.autofocus" android:required="false"/>
>  
>     <application android:label="@MOZ_APP_DISPLAYNAME@"
>-		 android:icon="@drawable/icon"
>+                 android:icon="@drawable/icon"
> #if MOZILLA_OFFICIAL
>-		 android:debuggable="false">
>+                 android:debuggable="false">
> #else
>-		 android:debuggable="true">
>+                 android:debuggable="true">
> #endif
> 
>         <activity android:name="App"
>@@ -113,7 +113,7 @@
>           <intent-filter>
>             <action android:name="org.mozilla.gecko.reportCrash" />
>           </intent-filter>
>-	</activity>
>+        </activity>
> #endif
> 
>         <activity android:name="LauncherShortcuts"
Note, I removed the tabs on these lines, hense the ws changes

Comment 17

[Tony Chung [:tchung]]

(In reply to Brad Lassey [:blassey] from comment #13)
> > - Back button on youtube player doesnt take one click to return back to
> > Fennec.  If you make touch actions within the video player controls, those
> > will be recorded part of the history.  (eg. click play, pause, then play - 3
> > touch actions.  Now if you hit back, you'll have to triple click it before
> > returning to fennec)   The more users click the controls, the less they will
> > know how to get back to fennec due to larger click history."
> 
> I don't see what you describe here. If the controls are up, hitting back
> dismisses them. Hitting back when the controls aren't up brings you back to
> fennec.

hmm, i rebooted the device and i can't reproduce the multiple backs anymore.   back dismisses the onscreen controls, and then skips back to fennec as you described.  send me a build with the video centering fix, and i'll be happy to validate it.

Comment 18

[Alex Keybl [:akeybl]]

Comment on attachment 579589 [details] [diff] [review]
patch

[Triage Comment]
Should only affect behavior on Kindle Fire and other devices without the YouTube system app, and may pave the way for submitting to the Amazon Appstore. Let's land on aurora and beta.

Comment 19

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

pushed https://hg.mozilla.org/mozilla-central/rev/658fad825c36, https://hg.mozilla.org/releases/mozilla-aurora/rev/ca986aa710c7 and https://hg.mozilla.org/releases/mozilla-beta/rev/2bbfe14f8b8d

Comment 20

[Tony Chung [:tchung]]

Verified fix on beta 5, Kindle Fire.  youtube player launches when play button is clicked. 

Mozilla/5.0 (Android; Linux armv7l; rv:9.0 Gecko/2011011206 Firefox/9.0 Fennec/9.0

Comment 21

[Ioana Chiorean]

Build id : Mozilla/5.0 (Android; Linux armv7l; rv:9.0 Gecko/2011011206 Firefox/9.0 Fennec/9.0
Devices: Samsung Galasy SII and Acer Iconia (2.3.4 and 3.2)

- youtube app plays the video after the play button is pressed. No pop up or notification appears.

Comment 22

[Aaron Train [:aaronmt]]

This caused a regression in that it creates a second application shortcut in the Android launcher; which when invoked causes an NPE: bug 708574