1028835 - [dolphin][flame] video list display abnormal for a while when switch between landscape mode and portrait mode

Status: RESOLVED FIXED

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

Description

[Peipei Cheng (needinfo if you need my action)]

Attachment: STR video

This issue can be reproduced on both dolphin and flame v1.4

Steps to reproduce
--------------------------------------------------------------------------
1. Prepare many videos in SD card (for example you can make over 100 copy of one video). 
2. Start device
3. Launch Video
4. After all videos are loaded, change phone from portrait mode to landscape mode
   --> You will see the layout of video list is abnormal for about 1~2 seconds. After a while, it get recovered.

Please see attached video for STR.


Flame v1.4 build
----------------------------------------------------------
Gaia 4b3428e85b428e577a0f96cba6121c4ca1c91f8a
Gecko https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/8fa3849a97c8
BuildID 20140616160203
Version 30.0

Comment 1

[ben.song]

Specific analysis process is as follows:
    step 1:The premise of that sd card has a lot of video (such as 200), but regardless of the video size.
    step 2:When the user opens the Video application, and wait for all the video has finished loading. In list mode the phone screen is rotated 90 degrees, switch to landscape mode.This action will trigger pre-registered on the window object screenlayoutchange type of event listener, call the corresponding event handler video.js:handleScreenLayoutChange().(The process of switching back to the portrait mode is similar to above)
    step 3:Function handleScreenLayoutChange() only calls thumbnail_list.js:upateAllThumbnailTitle().
    step 4:Function upateAllThumbnailTitle() traverses all the thumbnail item, and calls thumbnail_item.js:updateTitleText() for each thumbnail item.
    step 5:Function updateTitleText() calls video_utils.js:getTruncated(),and sets the return value of function getTruncated() to the title of the thumbnail item.From the trigger to the call of function getTruncated() only takes 0.002s.
    step 6:Function getTruncated() transforms the title value of the thumbnail item into an array of characters,and traverses this array,Each iteration has three important values,oldHeight value:the content node height of the string composed of first n-1(n>=2) characters in the array;newHeight:the content node height of the string composed of first n characters and the specified ellipsis characters in the array;testHeight:the content node height of the string composed of first n characters in the array.Judge whether oldHeight value be equal to newHeight value,if true,to continue to traverse the situation which n value been added 1;otherwise judge  whether oldHeight value be equal to testHeight value,if true the currentlines will be added 1(the initial of the currentlines is 1).Then judge whether the currentlines is greater than the specified maxline, if true,stop traversal and return the string composed of the first n characters and the specified ellipsis characters before the newHeight value be alter the last time.Otherwise continue to traverse,If the currentLines number is not yet greater than the maxLine value at the end of the traversal so return original string.The process took 29.273 s, 99.99% of the total time, the average of function getTruncated() takes 0.15 s, along with the increase in quantity, this bug will become more and more obvious.The same bug also exists in other applications in the process of horizontal and vertical screen list mode switching of.

Comment 2

[ben.song]

The main problem of the bug is that almost time consuming in the update the title of all the video files, and the width of each character occupies in UI is inconsistent,so it is not easy to be optimized.The method of the Video APP code is transform the filename string into an array of characters each time, and from less to more put into thumbnail node, until the width and line number is appropriate,each file are doing like this. In this process, each attempt consumes about 0.005s, each file would try about 20 times,so a title's update requires 0.1s, so a large amount of files would be quite time-consuming and more.

Comment 3

[Russ Nicoletti [:russn]]

I'm not able to play the attached video but I am able to reproduce the "abnormal" video list when changing orientation when there are a lot of videos on the device.

Comment 4

[Russ Nicoletti [:russn]]

My first thought at optimizing the process of getting a truncated version of the video title is that the title of the vast majority of videos will not need to be truncated. At least, that's my assumption and I believe it is a reasonable one. In any event, it should be possible to determine upfront if the title needs to be truncated and only when it needs truncation to use the current truncation algorithm. I will pursue that approach.

Comment 5

[Russ Nicoletti [:russn]]

Attachment: Pull request: https://github.com/mozilla-b2g/gaia/pull/20945

I have created a patch for master (since it is affected as well). The patch implements the approach I outlined in comment #4. If and when the issue is deemed to be blocking 1.4, the patch can be applied to 1.4.

Comment 6

[ben.song]

In attachment 8445555 [details] [review] this bug has a great improve when the title of video nodes is short, especially switching process from horizontal screen to vertical screen,the effect is obvious, but it do not deal with the situation when  the title of video nodes is long. The shorter title When the majority of video can be quickly switched over, the longer title when the majority still takes a long time. The resolution of this bug can also reduce the video application initialization time, for the process of loading the video APP would update all the video titles once or twice.

Comment 7

[Hema Koka [:hema]]

Not a release blocker based on these guidelines: https://wiki.mozilla.org/B2G/Triage#Blocker_Triage_Guidelines

Russ, since you already started looking into a solution, please proceed with it and get it landed on master.

Comment 8

[Russ Nicoletti [:russn]]

Ben, you have raised two issues:

1) When there are many videos on the device, regardless of the length of the titles, the process of rendering the list view is very time consuming and produces bad user experience.
2) The process of truncating video titles is very time consuming.

Issue #1 will affect all users who have lots of videos on their device.
Issue #2 will affect only those users who have a significant number of videos with titles that won't fit on two lines.

Issue #2 will affect only a small subset of the users affected by issue #1. Also, the bug you created was regarding issue #1. Therefore, I would like to get the patch that addresses issue #1 landed before addressing issue #2.

Comment 9

[ben.song]

Russ, the analysis about the two issues is valid for this patch to issue #1 is important to user experience, and i will continue to track this bug for issue #2.

Comment 10

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

Comment on attachment 8445555 [details] [review]
Pull request: https://github.com/mozilla-b2g/gaia/pull/20945

Thanks for this patch. It looks good to me. It works good in both flame and flatfish.

Comment 11

[James Zhang (Spreadtrum)]

Yang, please land this WIP patch to test.

Comment 12

[Russ Nicoletti [:russn]]

Attachment: Updated PR: https://github.com/mozilla-b2g/gaia/pull/20945

I have done some refactoring to video_utils getTruncated and added some unit tests. The PR has been updated. John, please review again. Thanks.

Comment 13

[Russ Nicoletti [:russn]]

I would like to clarify what I meant to say in comment #8. I meant to say that my preference is for this bug to represent issue #1 (when there are many videos on the device, regardless of the length of the titles, the process of rendering the list view is very time consuming and produces bad user experience), because it will affect far more users than issue #2 (only will affect users who have a significant number of videos whose titles are so long that they don't fit on 2 lines). My guess is we don't have any statistics for users who will be affected by issue #2, but in any case, I believe the best course of action is to create a separate bug for issue #2 if we think it will result in bad user experience for more than a trivial number of users.

Comment 14

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

Comment on attachment 8446665 [details] [review]
Updated PR: https://github.com/mozilla-b2g/gaia/pull/20945

Hi Russ,

This version is better. Thanks you.

As to the issue, including #1 and #2, I think it's my fault to introduce this function. It originated from settings app. And David and I made a short discussion on moving the code to shared. There are few concerns on the code. So, I just put it inside of video_utils.js.

For the #1, this patch should work pretty good.

For the #2, I would suggest to modify the code to use the "measureText" method of Canvas to calculate the width of text. That performs far more better than our current version. The current code is trying to use layout engine to re-layout the span for us which causes reflow calculation on DOM and that's the root of slowness. We can leverage the code in shared/font_size_utils.js which already uses measureText and used by other apps. So, that should be robust enough. But, we may need to wrap it to support multiple line, 2 in phone and 4 in tablet.

Comment 15

[yang.zhao]

Already landed it under sprd_patch,commit id is: 5b3ec83bf8ca7314d68f37be10277c3e8ced2148

Comment 16

[Preeti Raghunath(:Preeti)]

1.4+ for rather bad UX per video.

Comment 17

[Russ Nicoletti [:russn]]

Master commit that addresses issue #1: 

https://github.com/mozilla-b2g/gaia/commit/cece82beba47a1ebdea6dec8f28ac8ec14cf7636

Comment 18

[Russ Nicoletti [:russn]]

John, I've done some experimenting with measureText. I have some observations. 

I don't see any apps that consume the font_size_utils functions that use measureText. E.g., I don't see any consumers of getFontWidth or getOverflowCount. 

I added some instrumentation of video_utils getTruncated regarding measureText. I found that I don't know what width to use to compare to the value returned by measureText. For example, in portrait, a title whose font width according to measureText is 118 needs truncation -- according to the existing logic using cientHeight -- even though the node width (node.clientWidth) is 80. I would have naively assumed that since there are two lines available for the title the font width would have to be 160 before it needs truncation. Furthermore, a title whose font width is 113 does not need truncation -- according to the current approach.

Can you provide more guidance on using measureText in getTruncated?

Comment 19

[Wayne Chang [:wchang]]

Hi Russ,
In case it got missed, this is now 1.4+ so if you could please also land it to 1.4 when you're ready it would be great.

Thanks.

Comment 20

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

(In reply to Russ Nicoletti [:russn] from comment #18)
> John, I've done some experimenting with measureText. I have some
> observations. 
> 
> I don't see any apps that consume the font_size_utils functions that use
> measureText. E.g., I don't see any consumers of getFontWidth or
> getOverflowCount. 

It was used by lots app before. But they were removed.

> I added some instrumentation of video_utils getTruncated regarding
> measureText. I found that I don't know what width to use to compare to the
> value returned by measureText. For example, in portrait, a title whose font
> width according to measureText is 118 needs truncation -- according to the
> existing logic using cientHeight -- even though the node width
> (node.clientWidth) is 80. I would have naively assumed that since there are
> two lines available for the title the font width would have to be 160 before
> it needs truncation. Furthermore, a title whose font width is 113 does not
> need truncation -- according to the current approach.
> 
> Can you provide more guidance on using measureText in getTruncated?

Yes, but I don't have any experience with Canvas's measureText. Ha. I had lots experience of it in Java. Basically, we need to read font-size, font-style, font-variant from span's css and put them as the properties of canvas. For real measurement, we may need to put enough characters to get the width exceeding the span's width to find a line of text, and do it line by line.

Comment 21

[Russ Nicoletti [:russn]]

(In reply to Wayne Chang [:wchang] from comment #19)
> Hi Russ,
> In case it got missed, this is now 1.4+ so if you could please also land it
> to 1.4 when you're ready it would be great.
> 
> Thanks.

I've created and tested on my dolphin the 1.4 patch. The time to rotate from portrait to landscape and vice-versa is improved from 10-11 seconds before the patch to 3-4 seconds after. 

I'm waiting for a clean travis build before landing.

1.4 PR: https://github.com/mozilla-b2g/gaia/pull/21239

Comment 22

[Russ Nicoletti [:russn]]

v1.4:

https://github.com/mozilla-b2g/gaia/commit/53a4ff9264d601ea963ead1a58e810c39580c0aa

Comment 23

[Russ Nicoletti [:russn]]

The v1.4 commit is a port of the master commit (comment #17), meaning the v1.4 commit addresses issue #1.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

This was reverted and re-landed while investigating Gaia unit test failures on TBPL that turned out to be from an unrelated mozharness change.

Revert:  https://github.com/mozilla-b2g/gaia/commit/e4f7bc7bc121d6351959a9578457f9d7f63d7430
Re-land: https://github.com/mozilla-b2g/gaia/commit/b6af029e82e8f77799dcefa22cd3da99a106d22c

Sorry for the churn :(

Comment 25

[Russ Nicoletti [:russn]]

I am marking this resolved because:

1) The main user-facing issue is resolved.
2) The second issue will require a fair amount of investigation and research and is low priority given 1).

I have created bug 1031611 to track issue #2.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/cb0d4cf1f0b9a0ab80c9db3c78cd0d6556314748