Closed Bug 903920 Opened 6 years ago Closed 6 years ago

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

Categories

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

Other
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gasolin, Assigned: johnhu)

References

Details

(Whiteboard: [Flatfish only][developer+])

Attachments

(7 files, 4 obsolete files)

Expect:

Modify and Merge experiment tablet work from 

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

to master
Blocks: flatfish
blocking-b2g: --- → koi+
Assignee: nobody → gduan
Nice to have at current stage.
blocking-b2g: koi+ → ---
It's a must for flatfish. koi+
blocking-b2g: --- → leo+
Whiteboard: [Flatfish only]
blocking-b2g: leo+ → koi+
Flatfish only UI change, move to 1.3+.
blocking-b2g: koi+ → 1.3+
Summary: [Flatfish][Video] add support for large device → [Flatfish][Video] support 2 column layout for tablet
according to 10/18 meeting result, ETA date will be 11/5 (ready for review), 11/14 landed. Targeted milestone assigned.
Target Milestone: --- → 1.3 Sprint 4 - 11/8
We had made a discussion about this bug and transfer this bug to media team, myself.
Assignee: gduan → 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?
Flags: needinfo?(sotaro.ikeda.g)
(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.
Flags: needinfo?(sotaro.ikeda.g)
The behavior of tablet will based on the patch of bug 921421
Depends on: 921421
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
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.
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 on attachment 821604 [details]
[Flatfish]Media&settings framework_20131018.pdf

UX will upload new spec.
Attachment #821604 - Attachment is obsolete: true
Please upload UX and Visual Spec. Thanks.
Flags: needinfo?(hhuang)
Attached file [Flatfish] Video Player v1.1.pdf (obsolete) —
Attached file Video visual spec.pdf
Visual spec.
Flags: needinfo?(hhuang)
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?
Flags: needinfo?(hhsu)
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.
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.
(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.
Flags: needinfo?(hhsu)
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.
Attachment #826669 - Flags: review?(dflanagan)
Attachment #826673 - Attachment is obsolete: true
Sorry for low resolution. That's because of the limitation of attachment. I will make more when the device is free.
Attached file [Flatfish] Video Player v1.2.pdf (obsolete) —
Attachment #825222 - Attachment is obsolete: true
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.
Attachment #826669 - Flags: review?(arthur.chen)
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!
Attachment #826669 - Flags: review?(arthur.chen)
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...
Attachment #826669 - Flags: review?(arthur.chen)
Comment on attachment 826669 [details] [review]
the patch for adding the support of tablet.

r=me for the settings part, thanks!
Attachment #826669 - Flags: review?(arthur.chen) → review+
(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.
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.
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.
Depends on: 905065
Video shares the same unofficial image with Gallery. Make this depends on gallery app to have the unofficial image.
Depends on: 928254
Comment on attachment 826669 [details] [review]
the patch for adding the support of tablet.

As per comment 34, change reviewer to George Duan
Attachment #826669 - Flags: review?(dflanagan) → review?(gduan)
implementation resource and ETA date are both agree with RD team. this has to be done before 1.3FC
Whiteboard: [Flatfish only] → [Flatfish only][developer+]
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.
Attachment #827847 - Attachment is obsolete: true
blocking-b2g: 1.3+ → ---
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
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.
According to offline discussion, in full screen video mode, tapping the title bar and video controls will not autohide header and video controls. Thanks
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 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.
(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..
Depends on: 938967
(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
(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.
No longer depends on: 938967
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.
Attachment #826669 - Flags: review?(gduan)
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.
Attachment #826669 - Flags: review?(gduan)
Attached file bug903920.zip
Harly,

Please do the ux review on this patch.
Attachment #8343567 - Flags: ui-review?(hhsu)
Target Milestone: 1.3 Sprint 4 - 11/8 → ---
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?
Attachment #826669 - Flags: review?(gduan)
Attachment #826669 - Flags: review?(dflanagan)
Attachment #826669 - Flags: review+
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.
Attachment #8343567 - Flags: ui-review?(hhsu) → ui-review+
The tablet_player_background.png is removed, no longer to depend on it.
No longer depends on: 928254
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.
Attachment #826669 - Flags: review?(dflanagan) → review+
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.
merged to master:
https://github.com/mozilla-b2g/gaia/commit/847ff13e62855c4f7ea2a4d4a4605a2f5655d156


Travis CI, all green: https://travis-ci.org/mozilla-b2g/gaia/builds/15385145
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Duplicate of this bug: 906545
Depends on: 987224
You need to log in before you can comment on or make changes to this bug.