play a webm file report error after fennec6.0, crash [@ mozilla::gfx::ScaleYCbCrToRGB565]

VERIFIED FIXED in Firefox 6

Status

()

Core
Audio/Video
--
critical
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: DongShengXue, Assigned: derf)

Tracking

({crash, regression})

Trunk
mozilla6
ARM
Android
crash, regression
Points:
---

Firefox Tracking Flags

(firefox6+ fixed, firefox7+ fixed, firefox8 fixed)

Details

(Whiteboard: verified-beta, verified-aurora, crash signature)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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"
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
Severity: major → critical
Crash Signature: [@ mozilla::gfx::ScaleYCbCrToRGB565]
Keywords: crash
Summary: play a webm file report error after fennec6.0. → play a webm file report error after fennec6.0, crash [@ mozilla::gfx::ScaleYCbCrToRGB565]
Duplicate of this bug: 671278
Crash Signature: [@ mozilla::gfx::ScaleYCbCrToRGB565] → [@ mozilla::gfx::ScaleYCbCrToRGB565][@ libxul.so@0x9cfce4]
I guess this is a Core->Video bug, right?
Component: General → Video/Audio
Product: Fennec → Core
QA Contact: general → video.audio
Version: Firefox 6 → Trunk
(Assignee)

Updated

6 years ago
Assignee: nobody → tterribe
Blocks: 634557
Status: NEW → ASSIGNED
(Assignee)

Comment 4

6 years ago
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.
(Assignee)

Comment 5

6 years ago
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
Attachment #546985 - Attachment is obsolete: true
Crash Signature: [@ mozilla::gfx::ScaleYCbCrToRGB565][@ libxul.so@0x9cfce4] → [@ mozilla::gfx::ScaleYCbCrToRGB565] [@ libxul.so@0x9cfce4 ] [@ libxul.so@0x9cbea4 ]
Crash Signature: [@ mozilla::gfx::ScaleYCbCrToRGB565] [@ libxul.so@0x9cfce4 ] [@ libxul.so@0x9cbea4 ] → [@ mozilla::gfx::ScaleYCbCrToRGB565] [@ libxul.so@0x9cfce4 ] [@ libxul.so@0x9cbea4 ] [@ libxul.so@0x9cfda4 ]
(Assignee)

Comment 6

6 years ago
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
Attachment #547185 - Flags: review?(chris)
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?
(Assignee)

Comment 8

6 years ago
(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.
(Assignee)

Comment 9

6 years ago
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.
Attachment #547185 - Attachment is obsolete: true
Attachment #547588 - Flags: review?(chris)
Attachment #547185 - Flags: review?(chris)
Attachment #547588 - Flags: review?(chris) → review+
(Assignee)

Comment 10

6 years ago
Try results: http://tbpl.mozilla.org/?tree=Try&rev=27235829a364

Inbound push: http://hg.mozilla.org/integration/mozilla-inbound/rev/9739557bf0d0
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/9739557bf0d0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8

Comment 12

6 years ago
With combined signatures, it is #3 top crasher in 6.0.
There have been no crashes in 5.0, so it's a regression.
Crash Signature: [@ mozilla::gfx::ScaleYCbCrToRGB565] [@ libxul.so@0x9cfce4 ] [@ libxul.so@0x9cbea4 ] [@ libxul.so@0x9cfda4 ] → [@ mozilla::gfx::ScaleYCbCrToRGB565] [@ libxul.so@0x9cfce4 ] [@ libxul.so@0x9cbea4 ] [@ libxul.so@0x9cfda4 ] [@ libxul.so@0x9cfe24 ]
status-firefox6: --- → affected
tracking-firefox6: --- → ?
Keywords: regression
status-firefox7: --- → affected
tracking-firefox7: --- → ?
(Assignee)

Comment 13

6 years ago
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.
Attachment #550193 - Flags: approval-mozilla-beta?
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
(Assignee)

Comment 15

6 years ago
(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

6 years ago
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.
(Assignee)

Comment 17

6 years ago
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

6 years ago
Can we narrow down the bug that regressed this? Was it a NEON or a libvpx update?

Comment 19

6 years ago
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.
(Assignee)

Comment 20

6 years ago
(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.
(Assignee)

Comment 21

6 years ago
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.
Attachment #550193 - Attachment is obsolete: true
Attachment #550193 - Flags: approval-mozilla-beta?
(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

6 years ago
Ok, we're going to roll the dice and approve this to land w/o having a 100% finished review. Thanks Marty!

Comment 24

6 years ago
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
Attachment #550557 - Flags: approval-mozilla-beta+
(Assignee)

Comment 25

6 years ago
http://hg.mozilla.org/releases/mozilla-beta/rev/3ce34c96e574
status-firefox6: affected → fixed
Target Milestone: mozilla8 → mozilla6
status-firefox8: --- → fixed
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.
Tracking since we approved the patch here.
tracking-firefox6: ? → +
tracking-firefox7: ? → +
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.
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.
Whiteboard: verified-beta

Updated

6 years ago
Status: RESOLVED → VERIFIED
(Assignee)

Comment 30

6 years ago
(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.
(Assignee)

Comment 31

6 years ago
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.
Attachment #550557 - Flags: approval-mozilla-aurora?

Updated

6 years ago
Attachment #550557 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
(Assignee)

Comment 33

6 years ago
(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
status-firefox7: affected → fixed
verified on aurora
thrive, galaxy tab
Mozilla/5.0 (Android; Linux armv7I; rv8.0a2) Gecko/20110822 Firefox/8.0a2 Fennec/8.0a2
Whiteboard: verified-beta → verified-beta, verified-aurora
You need to log in before you can comment on or make changes to this bug.