Closed Bug 1294823 Opened 3 years ago Closed 3 years ago

Don't use a fullscreen view for HLS

Categories

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

50 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 51
Tracking Status
fennec 50+ ---
firefox50 --- verified
firefox51 --- verified

People

(Reporter: snorp, Assigned: snorp)

References

Details

Attachments

(3 files, 2 obsolete files)

Assignee: nobody → snorp
tracking-fennec: --- → ?
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(..) ?
Attachment #8780708 - Flags: review?(s.kaspari) → review+
(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.
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?
(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.
tracking-fennec: ? → 50+
Attachment #8780708 - Attachment is obsolete: true
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6e4605e1217
Don't use fullscreen mode for the HLS video view on Android r=sebastian
Backout by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a924e27a4666
Backout because I accidentally pushed the wrong patch r=me
Attachment #8783688 - Attachment is obsolete: true
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?
Attachment #8785465 - Flags: review?(s.kaspari) → review+
(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.
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f617fcd2cd2
Don't use fullscreen mode for the HLS video view on Android r=sebastian
Priority: -- → P2
https://hg.mozilla.org/mozilla-central/rev/3f617fcd2cd2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
QA Contact: mihai.ninu
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
Attachment #8785465 - Flags: approval-mozilla-beta?
Flags: qe-verify+
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.
Attachment #8785465 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This patch didn't apply cleanly to beta, could we get a rebased patch?
Flags: needinfo?(snorp)
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)
See Also: → 1308205
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.