1294823 - Don't use a fullscreen view for HLS

Status: VERIFIED FIXED

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

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

It's jarring.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Don't use fullscreen mode for the HLS video view on Android

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8780708 [details] [diff] [review]
Don't use fullscreen mode for the HLS video view on Android

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

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
@@ +965,5 @@
>          if (hideFirstrunPager(TelemetryContract.Method.BACK)) {
>              return;
>          }
>  
> +        if (mVideoLayout.haveVideo()) {

Is it really haveVideo and not hasVideo? I'm confused.

@@ +4122,5 @@
>              Telemetry.sendUIEvent(TelemetryContract.Event.LAUNCH, method, "restricted");
>          }
>      }
> +
> +    static class VideoFrameLayout extends FrameLayout {

Can we move this to a separate file in org.mozilla.gecko.widget? The line count of BrowserApp is too damn high.

@@ +4183,5 @@
> +        }
> +
> +        @Override
> +        public boolean onTouchEvent(MotionEvent event) {
> +            mVideo.onTouchEvent(event);

* Doesn't the video view already get the touch events?
* Why don't we return the boolean from mVideo.onTouchEvent()?

@@ +4190,5 @@
> +
> +        @Override
> +        public boolean onTrackballEvent(MotionEvent event) {
> +            mVideo.onTrackballEvent(event);
> +            return true;

* return mVideo.onTrackballEvent(..) ?

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Sebastian Kaspari (:sebastian) from comment #2)
> Comment on attachment 8780708 [details] [diff] [review]
> Don't use fullscreen mode for the HLS video view on Android
> 
> Review of attachment 8780708 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
> @@ +965,5 @@
> >          if (hideFirstrunPager(TelemetryContract.Method.BACK)) {
> >              return;
> >          }
> >  
> > +        if (mVideoLayout.haveVideo()) {
> 
> Is it really haveVideo and not hasVideo? I'm confused.

Could be either. I guess hasVideo() seems better.

> 
> @@ +4122,5 @@
> >              Telemetry.sendUIEvent(TelemetryContract.Event.LAUNCH, method, "restricted");
> >          }
> >      }
> > +
> > +    static class VideoFrameLayout extends FrameLayout {
> 
> Can we move this to a separate file in org.mozilla.gecko.widget? The line
> count of BrowserApp is too damn high.

Yup, fair enough.

> 
> @@ +4183,5 @@
> > +        }
> > +
> > +        @Override
> > +        public boolean onTouchEvent(MotionEvent event) {
> > +            mVideo.onTouchEvent(event);
> 
> * Doesn't the video view already get the touch events?
> * Why don't we return the boolean from mVideo.onTouchEvent()?

It does, but we want touches in the containing FrameLayout to be eaten and not pass through to the underlying LayerView. That's why we always return true since that means the event has been handled.

Comment 4

[Barbara Bermes [:barbara] - NI please!]

Attachment: error shown altought users can view the video

Comment 5

[Barbara Bermes [:barbara] - NI please!]

Attachment: video playback

Comment 6

[Barbara Bermes [:barbara] - NI please!]

Tried it out on http://www.newshub.co.nz/business/powerball-winners-cash-in-tickets-2016071211#axzz4EA1uumyJ

I noticed a few things
- the overlay feels somehow not very accurate, spanning across the whole width of the screen (see attachment)
- it shows an error message for the video if it's not playing indicating to the user that they can't view the video (see attachment)

I'm currently worried this experience is actually worse than the full screen version.

Thoughts?

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Barbara Bermes [:barbara] from comment #6)
> Tried it out on
> http://www.newshub.co.nz/business/powerball-winners-cash-in-tickets-
> 2016071211#axzz4EA1uumyJ
> 
> I noticed a few things
> - the overlay feels somehow not very accurate, spanning across the whole
> width of the screen (see attachment)

That's by design here -- we aren't trying to position it on top of the real video.

> - it shows an error message for the video if it's not playing indicating to
> the user that they can't view the video (see attachment)

Yeah, that has always been the case, but we can probably fix that. I'll file separately.

> 
> I'm currently worried this experience is actually worse than the full screen
> version.
> 
> Thoughts?

Why do you think it's worse? The fullscreen one feels like you've been kicked into a totally different app unexpectedly to me. At least with this one you can tell you're still in Firefox, and you can see the web page coming through the overlay.

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Don't use fullscreen mode for the HLS video view on Android

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6e4605e1217
Don't use fullscreen mode for the HLS video view on Android r=sebastian

Comment 10

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a924e27a4666
Backout because I accidentally pushed the wrong patch r=me

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Don't use fullscreen mode for the HLS video view on Android

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8785465 [details] [diff] [review]
Don't use fullscreen mode for the HLS video view on Android

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

::: mobile/android/base/java/org/mozilla/gecko/media/VideoPlayer.java
@@ +1,1 @@
> +package org.mozilla.gecko.media;

nit: License header.

@@ +25,5 @@
> +
> +public class VideoPlayer extends FrameLayout {
> +    private VideoView mVideo;
> +    private FullScreenMediaController mController;
> +    private FullScreenListener mFullScreenListener;

nit: For new Java code we stopped using the 'm' prefix.

@@ +180,5 @@
> +            // http://androidxref.com/6.0.1_r10/xref/frameworks/base/core/java/android/widget/MediaController.java#239
> +            //
> +            // The media buttons are in a horizontal linear layout which is itself packed into
> +            // a vertical layout. The vertical layout is the only child of the FrameLayout which
> +            // MediaController inherits from.

Isn't this a bit fragile? I guess this will need some testing on various Android versions and devices?

@@ +182,5 @@
> +            // The media buttons are in a horizontal linear layout which is itself packed into
> +            // a vertical layout. The vertical layout is the only child of the FrameLayout which
> +            // MediaController inherits from.
> +            LinearLayout child = (LinearLayout)getChildAt(0);
> +            LinearLayout buttons = (LinearLayout)child.getChildAt(0);

nit: space after (LinearLayout) - Did you run checkstyle yet?

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Sebastian Kaspari (:sebastian) from comment #12)
> Comment on attachment 8785465 [details] [diff] [review]
> Don't use fullscreen mode for the HLS video view on Android
> 
> Review of attachment 8785465 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/java/org/mozilla/gecko/media/VideoPlayer.java
> @@ +1,1 @@
> > +package org.mozilla.gecko.media;
> 
> nit: License header.
> 
> @@ +25,5 @@
> > +
> > +public class VideoPlayer extends FrameLayout {
> > +    private VideoView mVideo;
> > +    private FullScreenMediaController mController;
> > +    private FullScreenListener mFullScreenListener;
> 
> nit: For new Java code we stopped using the 'm' prefix.
> 
> @@ +180,5 @@
> > +            // http://androidxref.com/6.0.1_r10/xref/frameworks/base/core/java/android/widget/MediaController.java#239
> > +            //
> > +            // The media buttons are in a horizontal linear layout which is itself packed into
> > +            // a vertical layout. The vertical layout is the only child of the FrameLayout which
> > +            // MediaController inherits from.
> 
> Isn't this a bit fragile? I guess this will need some testing on various
> Android versions and devices?

Maybe, but nothing has really ever changed there since it was written: http://androidxref.com/7.0.0_r1/xref/frameworks/base/core/res/res/layout/media_controller.xml

> 
> @@ +182,5 @@
> > +            // The media buttons are in a horizontal linear layout which is itself packed into
> > +            // a vertical layout. The vertical layout is the only child of the FrameLayout which
> > +            // MediaController inherits from.
> > +            LinearLayout child = (LinearLayout)getChildAt(0);
> > +            LinearLayout buttons = (LinearLayout)child.getChildAt(0);
> 
> nit: space after (LinearLayout) - Did you run checkstyle yet?

I didn't run checkstyle, I'll definitely do that.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f617fcd2cd2
Don't use fullscreen mode for the HLS video view on Android r=sebastian

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3f617fcd2cd2

Comment 16

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8785465 [details] [diff] [review]
Don't use fullscreen mode for the HLS video view on Android

Approval Request Comment
[Feature/regressing bug #]: 1286133
[User impact if declined]: Bad HLS experience
[Describe test coverage new/current, TreeHerder]: Aurora, Nightly
[Risks and why]: Low
[String/UUID change made/needed]: None

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8785465 [details] [diff] [review]
Don't use fullscreen mode for the HLS video view on Android

HLS video is currently planned for Fx50, let's uplift this to Beta.

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This patch didn't apply cleanly to beta, could we get a rebased patch?

Comment 19

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/releases/mozilla-beta/rev/583b7822b8b8

Comment 20

[u549602]

Verified as fixed on both Aurora (51.0a2 - 2016-10-06) and Beta 50.0b4 builds, on a Samsung Galaxy S6 EDGE (android 6.0) and on a Nexus 7 (Android 5.1)