903920 - [Flatfish][Video] support 2 column layout for tablet

Status: RESOLVED FIXED

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

Description

[[e-mail]]

Expect:

Modify and Merge experiment tablet work from 

https://github.com/gaia-local/gaia

to master

Comment 2

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Video.png

Comment 3

[Greg Weng [:snowmantw][:gweng][:λ]]

Attachment: Playing.png

Comment 4

[Steven Yang [:styang]]

Nice to have at current stage.

Comment 5

[Steven Yang [:styang]]

It's a must for flatfish. koi+

Comment 6

[Francis Lee [:frlee]]

Flatfish only UI change, move to 1.3+.

Comment 7

[Francis Lee [:frlee]]

according to 10/18 meeting result, ETA date will be 11/5 (ready for review), 11/14 landed. Targeted milestone assigned.

Comment 8

[John Hu [:johnhu][:johu][:醬糊小弟]]

We had made a discussion about this bug and transfer this bug to media team, myself.

Comment 9

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: [Flatfish]Media&settings framework_20131018.pdf

UX spec

Comment 10

[John Hu [:johnhu][:johu][:醬糊小弟]]

Sotaro,

According to the tablet UX spec, we may have a video element on the screen with a video loaded and have another offscreen video to parse metadata for video files. That means we may create two video element at the same time and both of them may use hardware codec to decode different files.

Is this possible in most of our devices?

Since we had put a limitation about this at bug 856542, our video app in gaia will play only one video at the same time. We pause the parsing procedure when playing video and resume it when video is end. 

I know some android devices may have an instance of hardware codec and an instance of software codec, but some devices are not. And there is a software codec implementation in Android when hardware codec is unavailable.

Do we have similar mechanism? If yes, may we let a video element use hardware codec and let another one use software codec by program?

Do you know the bug number about this?

Comment 11

[Sotaro Ikeda [:sotaro]]

(In reply to John Hu [:johnhu] from comment #10)
> Sotaro,
> 
> According to the tablet UX spec, we may have a video element on the screen
> with a video loaded and have another offscreen video to parse metadata for
> video files. That means we may create two video element at the same time and
> both of them may use hardware codec to decode different files.
> 
> Is this possible in most of our devices?

If we instantiate multiple video tags, first one owns a hardware codec, and second one wait until the first one release it.
 
> I know some android devices may have an instance of hardware codec and an
> instance of software codec, but some devices are not. And there is a
> software codec implementation in Android when hardware codec is unavailable.

The problem of sw codec is that android's standard H.264 sw codec support only base profile, if it is main/high profile, sw codec can not decode it. We do not have a H.264 sw codec that can decode all h.264 video to create a thumbnail. Another problem is that gecko can not recognize if video codec is used for video playback and thumbnail creation. Video playback with sw codec can not provide the enough preformance as a product quality. html video tag does not restrict the usage of the video, therefore video tag always need to use hw video codec on video tag.

> Do we have similar mechanism? If yes, may we let a video element use
> hardware codec and let another one use software codec by program?

gecko is restricting the video codec only to hw codec. one reason for it is the video playback performance as written above. Other reason is if h.264 sw video codec is used for video playback, it consumes too much memory and cause the application crash.

FYI:
Bug 873959 is a bug for generate video thumbnail api. It is necessary also from thumbnail generation performance point of view.

Comment 12

[John Hu [:johnhu][:johu][:醬糊小弟]]

The behavior of tablet will based on the patch of bug 921421

Comment 13

[John Hu [:johnhu][:johu][:醬糊小弟]]

A WIP patch can be found here:
https://github.com/huchengtw-moz/gaia/commit/5f746c215b0243eda17175d88db3b625d2cce43a

Everything works, except the followings
1. open activity
2. hardware codec issue mentioned in comment 11

Comment 14

[John Hu [:johnhu][:johu][:醬糊小弟]]

A WIP patch can be found here:
https://github.com/huchengtw-moz/gaia/commit/2789aa82e44cfbc1a52a2373d340a93a0e2d0487

Everything works, but no art works.

The hardware codec issue is fixed with the following workaround:
1. In tablet landscape mode, we lock all playing operations before the all files are parsed in the first start-up.
2. In tablet landscape mode, we release the hardware codec for metadata parsing while user switches previewed video.
3. In tablet portrait mode and mobile phone, we keep original behavior: parsing metadata while in list mode.

Comment 15

[John Hu [:johnhu][:johu][:醬糊小弟]]

A WIP can be found here:
https://github.com/huchengtw-moz/gaia/commit/b9c4858fc265ea4074223642c9578ff9c5c52156

Everything works, but the follow art works:
1. thumbnail list

Comment 16

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 821604 [details]
[Flatfish]Media&settings framework_20131018.pdf

UX will upload new spec.

Comment 17

[John Hu [:johnhu][:johu][:醬糊小弟]]

Please upload UX and Visual Spec. Thanks.

Comment 18

[Harly Hsu[:harly]]

Attachment: [Flatfish] Video Player v1.1.pdf

Comment 19

[Helen Huang [:HHuang]]

Attachment: Video visual spec.pdf

Visual spec.

Comment 20

[Al Tsai [:atsai]]

Please check the UX design. There should be no message app for sharing video. BTW, do we support select multiple video files at the same time?

Comment 21

[John Hu [:johnhu][:johu][:醬糊小弟]]

We support multiple video files sharing at the same time. But that's only from video app to other app, not from other app to pick video from video app.

Comment 22

[John Hu [:johnhu][:johu][:醬糊小弟]]

A workable WIP patch can be found here:
https://github.com/huchengtw-moz/gaia/commit/1154172378d66289abe6b07ceb9e4190918ab2c9

Everything is on. But I don't have a device with video playable. The app is tested in nightly. I will continue to test in mobile phone and a playable flatfish device.

Comment 23

[Harly Hsu[:harly]]

(In reply to Al Tsai [:atsai] from comment #20)
> Please check the UX design. There should be no message app for sharing
> video. BTW, do we support select multiple video files at the same time?

Just checked, user can share a single video through message app, but not multiple video. Will modified this on the UX spec, thanks.

Comment 24

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: the patch for adding the support of tablet.

This is a huge patch, sorry about that.

The followings are change list:
1. use layout mode to switch: list, selection, and fullscreen player
2. introduces tablet UI, css and images
3. use media query to load different css files
4. move and use getTruncated() function which is in the settings app before to get the truncated text of title.
5. use flex to layout text in details.
6. add lots tablet only codes

I had tested this patch on Firefox Nightly, Inari and Flatfish devices. A recorded video will be uploaded for showing this patch running in flatfish.

Comment 25

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: video of running in flatfish: pick mode.

Comment 26

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: video app running on flatfish

Sorry for low resolution. That's because of the limitation of attachment. I will make more when the device is free.

Comment 27

[Harly Hsu[:harly]]

Attachment: [Flatfish] Video Player v1.2.pdf

Comment 28

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 826669 [details] [review]
the patch for adding the support of tablet.

Arthur,

I had moved the getTruncated from settings app to shared. Please review this part. Thanks in advance.

Comment 29

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 826669 [details] [review]
the patch for adding the support of tablet.

We used 'getTruncated' function in onpair.html. Please include the newly created js in the html. And we don't use it in main settings app, so you don't need to include it in settings.js. Thanks!

Comment 30

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 826669 [details] [review]
the patch for adding the support of tablet.

Arthur,

Thanks for pointing that. I had changed the patch. Please review it. Thanks...

Comment 31

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 826669 [details] [review]
the patch for adding the support of tablet.

r=me for the settings part, thanks!

Comment 32

[Al Tsai [:atsai]]

(In reply to hhsu from comment #23)
> (In reply to Al Tsai [:atsai] from comment #20)
> > Please check the UX design. There should be no message app for sharing
> > video. BTW, do we support select multiple video files at the same time?
> 
> Just checked, user can share a single video through message app, but not
> multiple video. Will modified this on the UX spec, thanks.

As I said, there is no message app. Please remove the message app.

Comment 33

[Harly Hsu[:harly]]

Got it, will remove message app on UX spec.

(In reply to Al Tsai [:atsai] from comment #32)
> (In reply to hhsu from comment #23)
> > (In reply to Al Tsai [:atsai] from comment #20)
> > > Please check the UX design. There should be no message app for sharing
> > > video. BTW, do we support select multiple video files at the same time?
> > 
> > Just checked, user can share a single video through message app, but not
> > multiple video. Will modified this on the UX spec, thanks.
> 
> As I said, there is no message app. Please remove the message app.

Comment 34

[David Flanagan [:djf]]

Sorry that I haven't had time to review this yet. Since George is workign on a similar patch for Gallery, perhaps he could review this one.

Comment 35

[John Hu [:johnhu][:johu][:醬糊小弟]]

Video shares the same unofficial image with Gallery. Make this depends on gallery app to have the unofficial image.

Comment 36

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 826669 [details] [review]
the patch for adding the support of tablet.

As per comment 34, change reviewer to George Duan

Comment 37

[Francis Lee [:frlee]]

implementation resource and ETA date are both agree with RD team. this has to be done before 1.3FC

Comment 38

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 826669 [details] [review]
the patch for adding the support of tablet.

As discussed offline, we all agree that 
1. move the video_tiny.css to video.css 
2. rem -> %
please kindly update, thanks.

Comment 39

[Harly Hsu[:harly]]

Attachment: [Flatfish] Video Player v1.2.pdf

Comment 40

[Harly Hsu[:harly]]

Below are some UX & graphic issues from a quick review of current Video app on Flatfish.

UX
1. Landscape full screen view will not auto hide header and video controls
2. A whit dotted line appears in the border of every popup dialog
3. Share action menu appeared in full screen, not as a popup dialog
4. Tap share, delete, and info button in full screen mode should not auto hide header and video controls
5. Share video and press cancel, share button will be in pressed state
6. Info page background is not transparent
7. No on-press state in full screen mode (Back, Delete, Share, Info)
8. No on-press state in video picker preview(Back & Select)

Visual
1. Two lines on title bar in landscape mode
2. Spacing between video title and time
Thanks

Comment 41

[John Hu [:johnhu][:johu][:醬糊小弟]]

The item 2, 3 are in building-blocks.
The item 6 is the image according to visual spec. According to offline discussion, we will use the same background style of dialog in building-blocks.

The item 4, 6, 7, 8 are current behavior in mobile phone. According to offline discussion, we will use change to the behavior like above in this patch.

Thanks for this feedback.

Comment 42

[Harly Hsu[:harly]]

According to offline discussion, in full screen video mode, tapping the title bar and video controls will not autohide header and video controls. Thanks

Comment 43

[(Inactive after June) George Duan [:gduan] [:喬智]]

Just roughly play this patch on tablet, besides comment 42, I also have some small questions which will not seriously impact the function.

1. Can we hide the toolbar and controlle in pickView?
2. When first enter video app in landscape => click play, and wait for the end => swipe to home => go to video app, and the preview is blank.

Comment 44

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 826669 [details] [review]
the patch for adding the support of tablet.

Hi George,

Thanks for your reviewing. The patch had made the following changes:
1. Merge the changes of video_tiny.css into video.css 
2. The change of all rem -> % makes no sense. So, I changes some elements which may need to resize according to screen size, but not all.
3. UX reviewing result 1, 4~8, and Visual reviewing result: 1 and 2
4. Change the behavior of show/hide video controls, both video.js and view.js are synced.

Please review it again. Thanks.

Comment 45

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to George Duan [:gduan] from comment #43)
> 1. Can we hide the toolbar and controlle in pickView?
 No
> 2. When first enter video app in landscape => click play, and wait for the
> end => swipe to home => go to video app, and the preview is blank.
 I will going to check this. Please use the latest version instead of the old version. I had checked this kind of issues this morning..

Comment 46

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to George Duan [:gduan] from comment #43)
> 2. When first enter video app in landscape => click play, and wait for the
> end => swipe to home => go to video app, and the preview is blank.
This bug is fixed at bug 938967

Comment 47

[John Hu [:johnhu][:johu][:醬糊小弟]]

(In reply to John Hu [:johnhu] from comment #46)
> (In reply to George Duan [:gduan] from comment #43)
> > 2. When first enter video app in landscape => click play, and wait for the
> > end => swipe to home => go to video app, and the preview is blank.
> This bug is fixed at bug 938967

Sorry, I mean that it will be fixed while bug 938967 landed.

Comment 48

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 826669 [details] [review]
the patch for adding the support of tablet.

Hi John,
I just check your patch and put some comments on it, please kindly check.
Thanks.

Comment 49

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 826669 [details] [review]
the patch for adding the support of tablet.

George,

I had change the patch to take care all of your comments. Please review it.

Comment 50

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: bug903920.zip

Harly,

Please do the ux review on this patch.

Comment 51

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 826669 [details] [review]
the patch for adding the support of tablet.

Thanks for your fixing,
it works fine on tablet and mobile.
Please remember to do rebase.
Thanks.

Hi David,
could you give this patch an overall review?

Comment 52

[Harly Hsu[:harly]]

Comment on attachment 8343567 [details]
bug903920.zip

All the bugs previously noted are fixed, 
except the ones related to Building Blocks. 
Thanks for the great work.

Comment 53

[John Hu [:johnhu][:johu][:醬糊小弟]]

The tablet_player_background.png is removed, no longer to depend on it.

Comment 54

[David Flanagan [:djf]]

Comment on attachment 826669 [details] [review]
the patch for adding the support of tablet.

I didn't have time to do a careful review of this, and I know you really need to land it.

See my few nits on github.  Please coordinate with the visual design team that is working on the visual refresh for the media apps because landing this will probably create a lot of work for them.

I'd prefer if you could land without the new string utility in shared/js/ because I'd like a chance to review that more carefully before it goes into shared.

Comment 55

[John Hu [:johnhu][:johu][:醬糊小弟]]

Thanks David.

I will move the getTruncated code to VideoUtils. And I had discussed with a visual in Taipei about the visual refresh. She felt the visual refresh is targeted to phone and she will track the changes that affect the tablet visual.

Comment 56

[John Hu [:johnhu][:johu][:醬糊小弟]]

merged to master:
https://github.com/mozilla-b2g/gaia/commit/847ff13e62855c4f7ea2a4d4a4605a2f5655d156


Travis CI, all green: https://travis-ci.org/mozilla-b2g/gaia/builds/15385145