1079692 - [Flame][Woodduck][Free Test][Video]The position of camcorder icon displayed error when changing portrait mode from landscape mode(5/5)

Status: RESOLVED FIXED

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

Description

[Josh Cheng [:josh]]

+++ This bug was initially created as a clone of Bug #1075284 +++

This issue seems the same as https://bugzilla.mozilla.org/show_bug.cgi?id=1028835, which has been marked "resolved"
But we still found this problem on woodduck

Comment 2

[Hema Koka [:hema]]

Please NI devices team to investigate first if this is happening on woodduck. 

Is this issue still happening on Flame?

Thanks
Hema

Comment 3

[David Flanagan [:djf]]

Josh,

This bug also needs explicit STR, and perhaps a video. Bug 1028835 had a couple of different issues. One of those issues was only an issue if there were lots of videos with very long titles. This is probably not a realistic test scenario, so if this is a bug that only occurs in that situation, I suggest you reconsider blocking. 

If this is really a bug that needs to be fixed, perhaps the new shared/js/font_size_utils.js are more efficient than the getTruncated() code in apps/video/js/video_utils.js.  Though it could be that with a multi-line display we can't use that shared module.

Comment 4

[Sherman Chen [:chens] (inactive)]

Device team already investigated this on bug 1075284, this issue also happens on Flame.
Providing QA branch check result from bug 1075284 comment 5, and bug 1075284 comment 10:

This issue can be reproduced on Flame v2.0, Flame v2.1, and Flame v2.2
==== Build Information - Flame 2.0 ============
Gaia-Rev        8079cba2133e6f5443dba24dad077f7f91e6c978
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/3a656e533675
Build-ID        20141001160205
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141001.193506
FW-Date         Wed Oct  1 19:35:16 EDT 2014
Bootloader      L1TC10011800

======= Build Information - Flame v2.1 ========================
Gaia-Rev        778ebac47554e1c4b7e9a952d73e850f58123914
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-aurora/rev/aa98eb8873a2
Build-ID        20141005160206
Version         34.0a2
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141005.194124
FW-Date         Sun Oct  5 19:41:35 EDT 2014
Bootloader      L1TC10011800

======= Build Information - Flame v2.2 ========================
Gaia-Rev        83f495a1c12687970f7f2840c2729795c4b88177
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/0ed32d9a42d6
Build-ID        20141005160202
Version         35.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141005.192646
FW-Date         Sun Oct  5 19:26:59 EDT 2014
Bootloader      L1TC10011800

Comment 5

[Keven Kuo [:kkuo]]

Hi!

Every 2.0+ bug was triaged by TAM, Device EM, EPM and QA.
It means this bug is also happening on Flame. Thanks

--
Keven

(In reply to Hema Koka [:hema] from comment #2)
> Please NI devices team to investigate first if this is happening on
> woodduck. 
> 
> Is this issue still happening on Flame?
> 
> Thanks
> Hema

Comment 6

[Josh Cheng [:josh]]

(In reply to Hema Koka [:hema] from comment #2)
> Please NI devices team to investigate first if this is happening on
> woodduck. 
> 
> Is this issue still happening on Flame?
> 
> Thanks
> Hema

Hi Hema,
You can check QA reproduce log for 2.0, 2.1 and 2.2 in bug 1075284
Thanks!

Comment 7

[Hema Koka [:hema]]

Could QA help provide STR and exact error seen when we switch between portrait and landscape. (If this is happening only in the case when we have 100+ videos and somehow related to videos having long names, then probably we should rethink blocking: https://wiki.mozilla.org/B2G/Triage#Blocker_Triage_Guidelines)

njpark, can you help here

Comment 8

[Hema Koka [:hema]]

canceling NI for njpark since the video is attached in the duplicate bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1075284

Comment 9

[Russ Nicoletti [:russn]]

I have confirmed the issues is related to there being many videos whose title needs truncating in portrait orientation. The process of determining where to truncate the titles is inefficient and causes the delay in the footer moving from the middle of the screen to the bottom.

I am working with a load of 50 videos whose titles need truncating. I have a POC patch that improves performance:

Before patch:
"update thumbnail title: 433 -- portrait to landscape" video.js:354
"update thumbnail title: 7676 -- landscape to portrait" video.js:354
"update thumbnail title: 437 -- portrait to landscape" video.js:354
"update thumbnail title: 7749 -- landscape to portrait" video.js:354

After patch:
"update thumbnail title: 478 -- portrait to landscape" video.js:354
"update thumbnail title: 1036 -- landscape to portrait" video.js:354
"update thumbnail title: 452 -- portrait to landscape" video.js:354
"update thumbnail title: 994 -- landscape to portrait" video.js:354

I will finalize the patch and submit for review today.

Comment 10

[Russ Nicoletti [:russn]]

Attachment: bug-1079692-patch.txt

I've tested the patch manually. I have not yet run the video unit tests with the patch.

Comment 11

[Russ Nicoletti [:russn]]

Attachment: Github PR: https://github.com/mozilla-b2g/gaia/pull/25046

The video unit tests pass when run locally.

Comment 12

[Russ Nicoletti [:russn]]

The performance improvement comes from changing the algorithm used to find where the title should be truncated (where the ellipse should be positioned) from a linear search to a binary search.

Comment 13

[Russ Nicoletti [:russn]]

Expanding on comment 12, it is important to reduce the number of iterations in finding the truncation point because for every iteration we are calculating the clientHeight of the node which is time consuming.

Comment 14

[David Flanagan [:djf]]

Comment on attachment 8503515 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/25046

The performance improvements of this patch seem great.

I'm concerned that this patch makes the assumption that each new line of text increases the size of the element by baseHeight pixels. I don't think that can be true in the general case where the element can have padding and/or borders. See my comments on github for thoughts on how you can fix this.

Comment 15

[Russ Nicoletti [:russn]]

Comment on attachment 8503515 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/25046

David, I've addressed your review comments and updated the PR. I have added my own comments in the PR.

Comment 16

[David Flanagan [:djf]]

Comment on attachment 8503515 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/25046

This looks good to me.

I trust that this fixes the reported bug and did not check the improved performance myself.

I did play around with the code, however, to add logging statements and to modify the CSS to add padding, and verified that the line height computation seems to work correctly. Titles that wrap onto more than one line do have heights that are integer multiples of the computed base height, so I think the underlying assumptions of the truncation algorithm are valid.

Comment 17

[Russ Nicoletti [:russn]]

Master: https://github.com/russnicoletti/gaia/commit/a2128a4afe5bebf60bc6e2ea8bcde940838430c1

Comment 18

[Josh Cheng [:josh]]

Hi Kai-Zhen,
Could you please help to land this fix on 2.0M? Thanks!

Comment 19

[Kai-Zhen Li [:seinlin][:kli]]

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

Comment 20

[Russ Nicoletti [:russn]]

Comment on attachment 8503515 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/25046

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #)
Truncating video titles when they don't fit the specified number of lines

[User impact] if declined:
Extremely bad user experience when there are many (10 or more) videos on the device whose titles need truncating. When rotating to portrait, it can be several seconds while the list view appears in only half the screen. With 10 videos whose titles need truncating, there is a delay of approximately two seconds when switching from landscape to portrait before the video list appears properly. 20 videos takes 4 seconds. With the fix, there is no perceptible delay with 20 videos whose title needs truncating.

[Testing completed]:
Unit tests and manual testing.

[Risk to taking this patch] (and alternatives if risky):
Low. The change is limited to the algorithm that truncates video titles.

[String changes made]: 
none.

Comment 21

[Lancy(Leave from Mozilla)]

Hi Kai-Zhen,

This issue does not exist in the Flame 2.0 and Woodduck 2.0M.

Flame version:
Gaia-Rev        76070c98dd0068324938c79bede50fe6d90bd996
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/a966f9f6e3ac
Build-ID        20141019161201
Version         34.0
Device-Name     flame-kk
FW-Release      4.4.2
FW-Incremental  34
FW-Date         Tue Sep 30 14:06:36 CST 2014
Bootloader      L1TC00011840

Woodduck version:

Gaia-Rev        a273ab9c18e9184eb02722b25c73e2ba7680cc09
Gecko-Rev       e7df4dde2d9dbedee942333d34eaea2afe32bebc
Build-ID        20141017100433
Version         32.0
Device-Name     soul35
FW-Release      4.4.2
FW-Incremental  1413510704
FW-Date         Fri Oct 17 09:52:15 CST 2014

Comment 22

[Josh Cheng [:josh]]

Hi Rachelle,
Could you please check whether this bug is required for Madai 2.0. Thanks!

Comment 23

[Sherman Chen [:chens] (inactive)]

(In reply to Lancy from comment #21)
> This issue does not exist in the Flame 2.0 and Woodduck 2.0M. 

Patch is not yet land on 2.0 branch and I think this issue still occur on 2.0.
Flame 2.0 without patch: http://youtu.be/Ap3vshYaqpQ

Also provide video from woodduck 2.0m
Woodduck 2.0m without patch: http://youtu.be/v5TX4AAtRUA
Woodduck 2.0m with patch: http://youtu.be/q_aXij-Kfoo

Comment 24

[Rachelle Yang [:ryang][[e-mail]]]

[Blocking Requested - why for this release]:

Hi Josh , thanks for your remind.
I will check with partner, and get back to you and make decision in the next triage meeting. Thanks!

Comment 25

[Dave Hylands [:dhylands]]

Based on the woodduck video (last link of comment 23) it still looks wrong, even with the patch.

Comment 26

[Josh Cheng [:josh]]

2.0 Triage:
Trivial for 2.0. Only fix on 2.0M.

@David,
The patch seems not working. Could you please have a check again? Thanks!

Comment 27

[David Flanagan [:djf]]

I lost track of this bug. Sorry to take so long to respond.

Because there were no STR for this bug, Russ and I assumed that the issue was with videos that have long names. His patch makes that much faster.

Reading the bug that this was cloned from, it sounds like this occurs when there are 200+ videos on the phone. So the issue is only about the large number of videos.

I suspect, therefore that the problem is this code in thumbnail_list.js:

ThumbnailList.prototype.updateAllThumbnailTitle = function() {
  for (var key in this.thumbnailMap) {
    this.thumbnailMap[key].updateTitleText();
  }
};

This runs synchronously and will block things. The solution might be to replace this with an async version using setTimeout. Russ is busy with other bugs, so I'll take this bug and see if I can fix it.

Comment 28

[David Flanagan [:djf]]

I pushed 250 videos to my Flame with:

  cd test_media/reference-workload
  ./generateVideos.sh 250

After scanning those, it takes 3 to 4 seconds for the UI to finish updating when I rotate the phone. So I guess I can reproduce what is shown in the Woodduck videos

Comment 29

[David Flanagan [:djf]]

Attachment: new github pr for master

This is a new PR to be applied with Russ's previous PR. The first PR fixed the issue with long titles taking forever to do line breaking on. This PR fixes the issue where many thumbnails are handled synchronously.

John: I think the thumbnail list code is yours, so I'm wondering if you are able to review this. (If not, please redirect the review to :russn). The tricky part of this patch is my code for cancelling an update in progress (if the user rotates back and forth quickly). Does it look okay to you?

Comment 30

[David Flanagan [:djf]]

Sherman (or Josh): would you check whether this new patch solves the problem on Woodduck?  (You'll have to cherry-pick the patch into the 2.0M branch, but I think it will apply cleanly)

Comment 31

[David Flanagan [:djf]]

If this fixes the bug, I'll create a patch for 2.1 that combines both patches and will request uplift of that.

Comment 32

[David Flanagan [:djf]]

Reopening this so it appears in my dashboard and I don't forget about it again.

Comment 33

[Sherman Chen [:chens] (inactive)]

Attachment: video-footer

It works fine on master, but not on 2.0 and 2.0m. 
Video app will start with a blank view, no thumbnails but only footer shows up.

Comment 34

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

Comment on attachment 8521051 [details] [review]
new github pr for master

It looks good to me. It's fine to post-pone the execution of title text update.

Comment 35

[David Flanagan [:djf]]

Attachment: combined patch for 2.0

This patch combines Russ's patch for dealing with long video names with my patch for dealing asynchonously with many videos.

The reason the previous patch did not work for 2.0 was a spelling error in the method name in 2.0 that cause the patch to not apply cleanly.

Sherman: I think this should work for you, if you want to test it. I've tried with 2.0 but not 2.0m

Comment 36

[David Flanagan [:djf]]

Attachment: combined patch for v2.1

This patch is for the 2.1 branch. It combines Russ's original patch to efficiently find the ellipsis location with my patch to do it all asynchronously. Together they solve the bug for users who have videos with long titles, and user who have many videos (or both).

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): not a regression

[User impact] if declined: user with many videos or videos with long names will find the video app UX unresponsive for multiple seconds when they rotate their phone.  Users who rotate their phone back and forth repeatedly may manage to really hose the app.

[Testing completed]: yes, locally.

[Risk to taking this patch] (and alternatives if risky): not very risky. The underlying feature is just about displaying an ellipsis at the right place when the video title is too long to fit. So if this patch messes something up it will only be correct display of long titles.

[String changes made]: none

Comment 37

[David Flanagan [:djf]]

Comment on attachment 8521613 [details] [review]
combined patch for 2.0

Josh: this patch is available for uplift to 2.0 if you want it for Woodduck. See comment #36 for the approval request comments.

Comment 38

[David Flanagan [:djf]]

Landed on master: https://github.com/mozilla-b2g/gaia/commit/217cd29ec38a80a937d140e7a0556b222d528644

Comment 39

[David Flanagan [:djf]]

Comment on attachment 8503515 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/25046

Cancelling the uplift request for the original patch. There are now combined patches for 2.1 and 2.0 both with pending uplift requests.

Comment 40

[Sherman Chen [:chens] (inactive)]

Thanks David, I've try on both 2.0 and 2.0m, patch works perfectly!

Comment 41

[Josh Cheng [:josh]]

Comment on attachment 8521613 [details] [review]
combined patch for 2.0

Hi David,
There is no approval requirement for 2.0M. Thus I will ask Kai-Zhen to merge on 2.0M. For 2.0 approval we should consult Bhavana.

Comment 42

[Josh Cheng [:josh]]

Comment on attachment 8521613 [details] [review]
combined patch for 2.0

Hi Bhavana,
per comment 20. Please decide whether to land this patch on 2.0. Thank you!

Comment 43

[Josh Cheng [:josh]]

Hi Kai-Zhen,
Can you help to land this new patch on 2.0M? Thanks!

Comment 44

[Kai-Zhen Li [:seinlin][:kli]]

v2.0m: https://github.com/mozilla-b2g/gaia/commit/807aedadcc63ae13ca2d1f74266240021a51cd17

Comment 45

[Ryan VanderMeulen [:RyanVM]]

v2.1: https://github.com/mozilla-b2g/gaia/commit/3210b4c4a9b7272820ab1d40835217e3de440652

Comment 46

[bhavana bajaj [:bajaj]]

Comment on attachment 8521613 [details] [review]
combined patch for 2.0

Rachelle, please re-nom this if this is a show-stopper for Madai, in which case we can consider uplift, else this will get fixed on 2.0M and 2.1 only at this point.

Comment 47

[Lance（leave from Mozilla）]

This issue has been verified successfully on Flame 2.1, 2.2, because the 1103917 bug still existed in Woodduck, so we can't verify the bug on woodduck.
See attachment: 1058.MP4
Reproducing rate: 0/5

Step:
1.Launch Video.
2.Switch the page to landscape.
3.Switch the page to portrait.
4.Repeat step 2-3.

Actual result:
The camcorder icon display correctly.

Flame 2.1 version:
Gaia-Rev        ccb49abe412c978a4045f0c75abff534372716c4
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-2g34_v2_1/rev/18fb67530b22
Build-ID        20141201001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141201.034405
FW-Date         Mon Dec  1 03:44:15 EST 2014
Bootloader      L1TC00011880

Flame 2.2 version:
Gaia-Rev        39214fb22c203e8849aaa1c27b773eeb73212921
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/08be3008650f
Build-ID        20141201040205
Version         37.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141201.074509
FW-Date         Mon Dec  1 07:45:20 EST 2014
Bootloader      L1TC00011880

Comment 48

[Lance（leave from Mozilla）]

Attachment: 1058.MP4