839600 - [Video] Start previously paused video from 0 if user has left and returned to video

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Video, defect)

Description

[Rick Waldron [:rwaldron]]

1. Open Videos app
2. Play any video in list (allow for a few seconds)
3. Pause the video
4. Hit the "<" to go back to video list
5. Play any other video and hit the "<" to go back to video list
6. Play the first video again

Expected: 
- Should start back at 0s

Actual:
- Starts from same time that it was paused

Precedent: 
- Parity with existing platforms, eg. Android, iOS

Comment 1

[Rick Waldron [:rwaldron]]

More to the point: Videos should always start at 0s, regardless of their prior playback state. 

...

There appears to be a bit of logic that preserves playback state from one viewing session to another (for the same video). This adds unnecessary complexity that doesn't improve the user experience of the application.

Comment 2

[Rick Waldron [:rwaldron]]

https://github.com/mozilla-b2g/gaia/pull/8035

Comment 3

[Rick Waldron [:rwaldron]]

The patch also includes non-critical (off-topic) refactors to explicitly check for undefined properties vs. using slower "in" operator.

Comment 4

[Rick Waldron [:rwaldron]]

Attachment: Fix for 839600

Comment 5

[Marcia Knous [:marcia]]

Hello Rick: You need to ask for review from someone from dev - I am QA.  It looks like Dale Harvey might be someone that you could ask.

(In reply to Rick Waldron from comment #4)
> Created attachment 712192 [details] [diff] [review]
> Fix for 839600

Comment 6

[Rick Waldron [:rwaldron]]

Marcia, sorry about that... thanks for pointing me in the right direction!

Comment 7

[Dale Harvey (:daleharvey)]

Comment on attachment 712192 [details] [diff] [review]
Fix for 839600

>diff --git a/apps/video/js/video.js b/apps/video/js/video.js
>index 716f8baa212a341b4e7793923138af9f1975bab4..79d49cbf702df6a26651cfad16a7e98f59884083 100644
>--- a/apps/video/js/video.js
>+++ b/apps/video/js/video.js
>@@ -273,7 +273,7 @@ function captureFrame(player, metadata, callback) {
>       canvas.width = THUMBNAIL_WIDTH;
>       canvas.height = THUMBNAIL_HEIGHT;
>       // If a rotation is specified, rotate the canvas context
>-      if ('rotation' in metadata) {
>+      if (metadata.rotation !== undefined) {
>         ctx.save();
>         switch (metadata.rotation) {
>         case 90:
>@@ -388,8 +388,8 @@ function setPlayerSize() {
>   if (!dom.player.videoWidth || !dom.player.videoHeight)
>     return;
> 
>-  var width, height; // The size the video will appear, after rotation
>-  var rotation = 'metadata' in currentVideo ?
>+  var width, height, rotation; // The size the video will appear, after rotation
>+  var rotation = currentVideo.metadata !== undefined ?
>     currentVideo.metadata.rotation : 0;

Duplicate definition of rotation

> 
>   switch (rotation) {
>@@ -444,13 +444,13 @@ function setPlayerSize() {
> }
> 
> function setVideoUrl(player, video, callback) {
>-  if ('name' in video) {
>+  if (video.name !== undefined) {
>     videodb.getFile(video.name, function(file) {
>       var url = URL.createObjectURL(file);
>       player.onloadedmetadata = callback;
>       player.src = url;
>     });
>-  } else if ('url' in video) {
>+  } else if (video.url !== undefined) {
>     player.onloadedmetadata = callback;
>     player.src = video.url;
>   }
>@@ -479,7 +479,7 @@ function showPlayer(data, autoPlay) {
>         play();
>       }
> 
>-      if ('metadata' in currentVideo) {
>+      if (currentVideo.metadata !== undefined) {
>         currentVideo.metadata.watched = true;
>         videodb.updateMetadata(currentVideo.name, currentVideo.metadata);
>       }
>@@ -487,6 +487,7 @@ function showPlayer(data, autoPlay) {
>   }
> 
>   setVideoUrl(dom.player, currentVideo, function() {
>+    var title;
> 
>     dom.durationText.textContent = formatDuration(dom.player.duration);
>     timeUpdated();
>@@ -496,21 +497,20 @@ function showPlayer(data, autoPlay) {
>     playerShowing = true;
>     setPlayerSize();
> 
>-    if ('name' in currentVideo && /^DCIM/.test(currentVideo.name)) {
>+    if (currentVideo.name !== undefined && /^DCIM/.test(currentVideo.name)) {
>       dom.deleteVideoButton.classList.remove('hidden');
>     }
> 
>-    if ('metadata' in currentVideo) {
>-      if (currentVideo.metadata.currentTime === dom.player.duration) {
>-        currentVideo.metadata.currentTime = 0;
>-      }
>-      dom.videoTitle.textContent = currentVideo.metadata.title;
>-      dom.player.currentTime = currentVideo.metadata.currentTime || 0;
>+    if (currentVideo.metadata !== undefined) {
>+      title = currentVideo.metadata.title;
>     } else {
>-      dom.videoTitle.textContent = currentVideo.title || '';
>-      dom.player.currentTime = 0;
>+      title = currentVideo.title;
>     }
> 
>+    // Always start the video at 0s. #839600
>+    dom.player.currentTime = 0;
>+    dom.videoTitle.textContent = title;
>+
>     if (dom.player.seeking) {
>       dom.player.onseeked = doneSeeking;
>     } else {
>@@ -535,7 +535,7 @@ function hidePlayer() {
>     updateDialog();
>   }
> 
>-  if (!('metadata' in currentVideo)) {
>+  if (currentVideo.metadata === undefined) {
>     completeHidingPlayer();
>     return;
>   }

Comment 8

[Dale Harvey (:daleharvey)]

Heh sorry didnt mean to post the whole thing for that single comment

The ability for video to resume was a UX requirement, whether videos 'should always' start from original position or resume is subjective, I think ideally if we see that the user is half way through a long video we ask them if they want to resume or start from beginning.

If we remove resume we may or may not want to remove the code which stores the last played video frame in the thumbnail list.

I feel this bug is probably invalid and we should open a new bug asking for confirmation from the user where they want to start, but I will needinfo UX on it

Comment 9

[Dale Harvey (:daleharvey)]

Comment on attachment 712192 [details] [diff] [review]
Fix for 839600

Apart from fixing the nit the rest is r+ for me, but removing the review request until we know whether the bug is valid or not

Comment 10

[Rick Waldron [:rwaldron]]

Thanks for the review.

(In reply to Dale Harvey (:daleharvey) from comment #8)
> Heh sorry didnt mean to post the whole thing for that single comment

No worries

> 
> The ability for video to resume was a UX requirement, whether videos 'should
> always' start from original position or resume is subjective, I think
> ideally if we see that the user is half way through a long video we ask them
> if they want to resume or start from beginning.

Fair enough. When I first experienced the "play from where I was" behaviour, I immediately thought something was wrong. I compared against 3 Android devices (running versions: 2.3.7, 4.0.4 and 4.2) and 2 iOS (iOS 5, iOS6) devices and this was the only one with that behaviour. If it's intended to provide a different behaviour then this bug is invalid :)

> 
> If we remove resume we may or may not want to remove the code which stores
> the last played video frame in the thumbnail list.

I had planned to propose that as a follow-up to this patch, instead of including two fixes in one patch... unless you think otherwise?

> 
> I feel this bug is probably invalid and we should open a new bug asking for
> confirmation from the user where they want to start, but I will needinfo UX
> on it

Fair enough.

Where can I find documentation of UX requirements?

Comment 11

[Josh Carpenter [:jcarpenter]]

Product requirements for Video were never provided, IIRC, so the UX specs are thin at best. IMO (admittedly without having had time to do prior art check on this) we should resume from the previous play point (the current behavior) if the user exits the video and comes straight back. But we should resume from zero if the user views another video in the interim, or if the app is closed.

Casey, I defer to your judgement on this one.

Comment 12

[Casey Yee]

(In reply to Josh Carpenter [:jcarpenter] from comment #11)
> we should resume from the previous play point (the current
> behavior) if the user exits the video and comes straight back. But we should
> resume from zero if the user views another video in the interim, or if the
> app is closed.

Sounds good to me.   

I think that videos over a certain length should also resume.   This will be useful when users are watching long play videos like TV shows or Movies.

Comment 13

[BMO Automation]

Firefox OS is not being worked on