Last Comment Bug 671818 - play a webm file report error after fennec6.0, crash [@ mozilla::gfx::ScaleYCbCrToRGB565]
: play a webm file report error after fennec6.0, crash [@ mozilla::gfx::ScaleYC...
Status: VERIFIED FIXED
verified-beta, verified-aurora
: crash, regression
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: ARM Android
: -- critical (vote)
: mozilla6
Assigned To: Timothy B. Terriberry (:derf)
:
Mentors:
: 671278 (view as bug list)
Depends on:
Blocks: 634557
  Show dependency treegraph
 
Reported: 2011-07-15 03:06 PDT by DongShengXue
Modified: 2011-08-23 16:57 PDT (History)
18 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
fixed


Attachments
Something went wrong while displaying a web page (51.93 KB, image/png)
2011-07-15 03:06 PDT, DongShengXue
no flags Details
Make libvpx's chroma plane rows 16-byte aligned (5.87 KB, patch)
2011-07-19 21:15 PDT, Timothy B. Terriberry (:derf)
no flags Details | Diff | Review
Make libvpx's chroma plane rows 16-byte aligned v2 (7.31 KB, patch)
2011-07-20 11:52 PDT, Timothy B. Terriberry (:derf)
no flags Details | Diff | Review
Make libvpx's chroma plane rows 16-byte aligned v3 (14.27 KB, patch)
2011-07-21 18:30 PDT, Timothy B. Terriberry (:derf)
cpearce: review+
Details | Diff | Review
Make libvpx's chroma plane rows 16-byte aligned v3 (for mozilla-beta) (14.21 KB, patch)
2011-08-02 14:03 PDT, Timothy B. Terriberry (:derf)
no flags Details | Diff | Review
Use unaligned loads for chroma in ScaleYCbCr42xToRGB565_BilinearY_Row_NEON (for mozilla-beta) (1.49 KB, patch)
2011-08-03 16:53 PDT, Timothy B. Terriberry (:derf)
jpr: approval‑mozilla‑aurora+
christian: approval‑mozilla‑beta+
Details | Diff | Review

Description DongShengXue 2011-07-15 03:06:47 PDT
Created attachment 546122 [details]
Something went wrong while displaying a web page

open http://dl1.hidrop.com/temp/mobile/big_buck_bunny_480p.webm report "Something went wrong while displaying a web page"
Comment 1 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-07-15 03:27:32 PDT
Confirmed, using the LG Optimus Black.

https://crash-stats.mozilla.com/report/index/bp-ebfa1954-8f4b-4993-b1df-470052110715
0 	libxul.so 	libxul.so@0x937e24 	
1 	libnspr4.so 	_pt_root 	nsprpub/pr/src/pthreads/ptthread.c:128
2 	libxul.so 	mozilla::gfx::ScaleYCbCrToRGB565 	gfx/ycbcr/ycbcr_to_rgb565.cpp:533
3 	libxul.so 	mozilla::layers::BasicPlanarYCbCrImage::SetData 	gfx/layers/basic/BasicImages.cpp:231
4 	libxul.so 	VideoData::Create 	nsAutoPtr.h:175
5 	libxul.so 	nsWebMReader::DecodeVideoFrame 	content/media/webm/nsWebMReader.cpp:744
6 	libxul.so 	nsBuiltinDecoderStateMachine::DecodeLoop 	content/media/nsBuiltinDecoderStateMachine.cpp:328
7 	libxul.so 	nsRunnableMethodImpl<void , true>::Run 	nsThreadUtils.h:343
8 	libxul.so 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:617
9 	libxul.so 	NS_ProcessNextEvent_P 	obj-firefox/xpcom/build/nsThreadUtils.cpp:245
10 	libxul.so 	nsThread::ThreadFunc 	xpcom/threads/nsThread.h:83
11 	libnspr4.so 	_pt_root 	nsprpub/pr/src/pthreads/ptthread.c:190
12 	libc.so 	libc.so@0x10f23 	
13 	libc.so 	libc.so@0x42327 	
14 	libc.so 	libc.so@0x10a13
Comment 2 Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-07-15 15:47:49 PDT
*** Bug 671278 has been marked as a duplicate of this bug. ***
Comment 3 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-07-18 10:30:08 PDT
I guess this is a Core->Video bug, right?
Comment 4 Timothy B. Terriberry (:derf) 2011-07-19 21:15:29 PDT
Created attachment 546985 [details] [diff] [review]
Make libvpx's chroma plane rows 16-byte aligned

So, the problem was that the NEON YCbCr->RGB+scaling code assumed that the input buffer rows had 16-byte alignment (because I thought this was actually true), but apparently libvpx only guarantees this for the luma plane, and not the chroma planes.

This patch updates libvpx to guarantee 16-byte alignment for all three planes. I'm still waiting to hear back from upstream if this is acceptable to them, and if not I'll fix the scaling code to use unaligned loads.
Comment 5 Timothy B. Terriberry (:derf) 2011-07-20 11:52:27 PDT
Created attachment 547185 [details] [diff] [review]
Make libvpx's chroma plane rows 16-byte aligned v2

There were still some issues with the previous patch, because a bunch of the libvpx code assumes that uv_stride == y_stride/2. This version addresses that, and also incorporates some of the clean-ups from upstream commit https://review.webmproject.org/2626

This patch (with adjustment for upstream changes and to address encoder issues) has been posted upstream as https://review.webmproject.org/2676
Comment 6 Timothy B. Terriberry (:derf) 2011-07-21 12:12:28 PDT
Comment on attachment 547185 [details] [diff] [review]
Make libvpx's chroma plane rows 16-byte aligned v2

http://tbpl.mozilla.org/?tree=Try&rev=a6bdcc2ad9aa
Comment 7 Chris Pearce (:cpearce) 2011-07-21 17:41:38 PDT
Comment on attachment 547185 [details] [diff] [review]
Make libvpx's chroma plane rows 16-byte aligned v2

Can you add these changes in a patch in media/libvpx/, with a line in update.sh to apply it?

Are any of the other patches which we apply in update.sh not yet upstreamed?

Do you have any idea when the next libvpx update which we should take will be released?
Comment 8 Timothy B. Terriberry (:derf) 2011-07-21 18:11:11 PDT
(In reply to comment #7)
> Can you add these changes in a patch in media/libvpx/, with a line in
> update.sh to apply it?

Duh, I of all people shouldn't forget to do that.

> Are any of the other patches which we apply in update.sh not yet upstreamed?

The "Patch to compile with Sun Studio on Solaris" isn't upstream. All the rest are.
 
> Do you have any idea when the next libvpx update which we should take will
> be released?

From 18 Jul in #vp8:
10:44:46 <@jk8> desti: gnafu: should be a release soon, I hope.
10:45:05 <@jk8> some last minute changes/bugfixes, plus lots of travel and vacation right now.
Comment 9 Timothy B. Terriberry (:derf) 2011-07-21 18:30:10 PDT
Created attachment 547588 [details] [diff] [review]
Make libvpx's chroma plane rows 16-byte aligned v3

This patch also removes the irrelevant yuv_row_arm.s change and contains a few minor updates make things more in-line with what was committed upstream. This version hasn't been through try yet.
Comment 11 :Ehsan Akhgari (busy, don't ask for review please) 2011-07-22 14:21:07 PDT
http://hg.mozilla.org/mozilla-central/rev/9739557bf0d0
Comment 12 Scoobidiver (away) 2011-07-27 13:32:42 PDT
With combined signatures, it is #3 top crasher in 6.0.
There have been no crashes in 5.0, so it's a regression.
Comment 13 Timothy B. Terriberry (:derf) 2011-08-02 14:03:59 PDT
Created attachment 550193 [details] [diff] [review]
Make libvpx's chroma plane rows 16-byte aligned v3 (for mozilla-beta)

I've rebased this patch for the beta branch. It applies and fixes the problem for me.
Comment 14 Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-08-03 11:43:52 PDT
Is this only applied to the nightly and not aurora? Reason I ask is because : 
https://crash-stats.mozilla.com/report/index/d37d58f3-fcb4-435f-ad0b-1561f2110729
is a 7/29 build
Comment 15 Timothy B. Terriberry (:derf) 2011-08-03 11:48:51 PDT
(In reply to comment #14)
> Is this only applied to the nightly and not aurora? Reason I ask is because
> : 
> https://crash-stats.mozilla.com/report/index/d37d58f3-fcb4-435f-ad0b-
> 1561f2110729
> is a 7/29 build

That is correct. I was going to ask for aurora approval as soon as I confirmed that this also fixed the problem on aurora, but bug 676141 was preventing me from actually testing that.
Comment 16 christian 2011-08-03 12:05:58 PDT
Cris and Tim: What's the risk/reward of this? At first glance it looks very, very risky and the risk looks for all platforms across desktop and mobile.
Comment 17 Timothy B. Terriberry (:derf) 2011-08-03 12:21:24 PDT
The main risk is that some other code in libvpx (particularly the asm) is making assumptions about the buffer layout. I believe I now understand all of the places it does this, however (and that was the reason the patch uses somewhat more padding than strictly necessary, see comment 5), and the patch has also gone through testing upstream on even more platforms in preparation for the 0.9.7 release of libvpx.

If you want me to try to work around this with a simpler patch to the ARM side only, I'm happy to do that, but that will take more time to verify and likely perform slightly worse.

Without this patch, roughly half of all WebM videos (possibly more, as this seems to be triggered by a common YouTube resolution) will cause a content crash on mobile.
Comment 18 christian 2011-08-03 14:05:08 PDT
Can we narrow down the bug that regressed this? Was it a NEON or a libvpx update?
Comment 19 christian 2011-08-03 14:08:26 PDT
Also, can you create a patch on releases/mozilla-beta, IFDEF'd only for ARM? That would be our preferred course of action unless you have any objections.
Comment 20 Timothy B. Terriberry (:derf) 2011-08-03 14:13:46 PDT
(In reply to comment #18)
> Can we narrow down the bug that regressed this? Was it a NEON or a libvpx
> update?

The bug that regressed this was bug 634557 (in the "Blocks" list above).

(In reply to comment #19)
> Also, can you create a patch on releases/mozilla-beta, IFDEF'd only for ARM?
> That would be our preferred course of action unless you have any objections.

Will do.
Comment 21 Timothy B. Terriberry (:derf) 2011-08-03 16:53:17 PDT
Created attachment 550557 [details] [diff] [review]
Use unaligned loads for chroma in ScaleYCbCr42xToRGB565_BilinearY_Row_NEON (for mozilla-beta)

This patch fixes the problem for me on beta and should have a greatly reduced risk/reward profile. Tested on both Android and Maemo.
Comment 22 Marty Rosenberg [:mjrosenb] 2011-08-03 18:00:22 PDT
(In reply to comment #21)
> Created attachment 550557 [details] [diff] [review] [diff] [details] [review]
> Use unaligned loads for chroma in ScaleYCbCr42xToRGB565_BilinearY_Row_NEON
> (for mozilla-beta)
> 
> This patch fixes the problem for me on beta and should have a greatly
> reduced risk/reward profile. Tested on both Android and Maemo.

This patch definitely makes a load 16 byte aligned, but without knowing any more of the code base, I don't know if that memory is any more or less correct than before doing the 16 byte alignment.
Comment 23 christian 2011-08-03 18:02:08 PDT
Ok, we're going to roll the dice and approve this to land w/o having a 100% finished review. Thanks Marty!
Comment 24 christian 2011-08-03 18:02:30 PDT
Comment on attachment 550557 [details] [diff] [review]
Use unaligned loads for chroma in ScaleYCbCr42xToRGB565_BilinearY_Row_NEON (for mozilla-beta)

Approved for releases/mozilla-beta
Comment 25 Timothy B. Terriberry (:derf) 2011-08-03 18:13:34 PDT
http://hg.mozilla.org/releases/mozilla-beta/rev/3ce34c96e574
Comment 26 Tony Chung [:tchung] 2011-08-03 20:50:35 PDT
Hi all, 
A little late getting to this bug, but QA has been watching this bug and will verify this against the beta.  

Here's what i can see that needs testing:
1) Target ARM based phones with NEON (eg. LG Optimus Black).  
** Also Do this also for the Maemo N900
2) playback a webm videos (eg. testcase from Comment 1)
3) do the same on youtube webm videos
4) Verify bug 634557 is fixed.
5) Run 2-3, but target ARM phone without NEON to make sure nothing else webm videos still playback there and didnt regress further.

Did i miss anything?  Please add additional STR tests for QA here.

We'll need to add a regression test for this bug.   Flagging to get litmus and unit tests added.
Comment 27 Johnny Stenback (:jst, jst@mozilla.com) 2011-08-04 14:24:37 PDT
Tracking since we approved the patch here.
Comment 28 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-08-04 16:53:18 PDT
Verified fixed on trunk Fennec on the LG Optimus black and the N900 using the video url mentioned in comment 0.

I tried getting it work, playing webm videos in youtube on the LG Optimus Black, but I couldn't get it working at all.
I did sign up for the html5 test at: http://www.youtube.com/html5
I tried testing with: http://www.youtube.com/watch?v=I7YllAOqpF4&feature=related
I did use the "&webm;=1", adding to the search url.
I also tried using Phony, setting the UAagent to desktop Firefox, setting advanced search options to look for webm only, but I keep getting a "need to upgrade your Adobe Flash Player to watch this video" message.
Comment 29 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-08-04 17:41:22 PDT
I tried http://dl1.hidrop.com/temp/mobile/big_buck_bunny_480p.webm on current Aurora build on the LG Optimus Black. Soon after the video starts playing, the tab turns white and in about:crashes, I see new crash reports appearing. So to confirm, it's still crashing in the Aurora builds.

I tried the Fennec6.0b5 candidate from:
http://ftp.mozilla.org/pub/mozilla.org/mobile/candidates/6.0b5-candidates/build1/android/multi/
And with the LG Optimus Black, the big_buck_bunny_480p.webm indeed doesn't crash anymore.

Also tried that same video with this N900 build: http://ftp.mozilla.org/pub/mozilla.org/mobile/candidates/6.0b5-candidates/build1/maemo5-gtk/multi/
Also, no crashes found with that build.
Comment 30 Timothy B. Terriberry (:derf) 2011-08-04 17:49:59 PDT
(In reply to comment #29)
> I tried http://dl1.hidrop.com/temp/mobile/big_buck_bunny_480p.webm on
> current Aurora build on the LG Optimus Black. Soon after the video starts
> playing, the tab turns white and in about:crashes, I see new crash reports
> appearing. So to confirm, it's still crashing in the Aurora builds.

Correct, I haven't landed the patch on aurora yet, because bug 645284 made it impossible to test there.
Comment 31 Timothy B. Terriberry (:derf) 2011-08-04 18:00:53 PDT
Comment on attachment 550557 [details] [diff] [review]
Use unaligned loads for chroma in ScaleYCbCr42xToRGB565_BilinearY_Row_NEON (for mozilla-beta)

With bug 645284 fixed, I can now test this on Android, and confirm that the patch that landed on beta does indeed solve the problem for aurora, too. Requesting approval to land there.
Comment 32 Tony Chung [:tchung] 2011-08-09 22:32:16 PDT
(In reply to Timothy B. Terriberry (:derf) from comment #31)
> Comment on attachment 550557 [details] [diff] [review]
> Use unaligned loads for chroma in ScaleYCbCr42xToRGB565_BilinearY_Row_NEON
> (for mozilla-beta)
> 
> With bug 645284 fixed, I can now test this on Android, and confirm that the
> patch that landed on beta does indeed solve the problem for aurora, too.
> Requesting approval to land there.

Can we get this landed on aurora soon?   We would like to verify this fix before the merge next week.
Comment 33 Timothy B. Terriberry (:derf) 2011-08-10 09:11:54 PDT
(In reply to Tony Chung [:tchung] from comment #32)
> Can we get this landed on aurora soon?   We would like to verify this fix
> before the merge next week.

http://hg.mozilla.org/releases/mozilla-aurora/rev/454007c65cea
Comment 34 Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-08-23 16:57:54 PDT
verified on aurora
thrive, galaxy tab
Mozilla/5.0 (Android; Linux armv7I; rv8.0a2) Gecko/20110822 Firefox/8.0a2 Fennec/8.0a2

Note You need to log in before you can comment on or make changes to this bug.