If NEON optimizations are available, we try to use them to do Y'CbCr to RGB conversion in ConvertYCbCrToRGB565(). However, for odd widths, the buffer we allocate does not respect Cairo's alignment requirements (it has a stride which is a multiple of 2, when Cairo demands at least 4). Thus Cairo fails to create a surface for the image, but the code tries to use the surface anyway, and crashes the content process. Steps to reproduce: 1. Use a device with NEON (N900, non-Tegra2 Android) 2. Visit http://people.xiph.org/~tterribe/theora/coastguard_335-V500.ogv The "Add ScaleYCbCrToRGB565" patch in bug 634557 contains a working fix. Breaking this issue out into a separate bug for the purposes of tracking and review.
This crash will actually occur on any video with an odd width on a 16-bit display, regardless of whether NEON optimizations are enabled or not, because of bug 640588 (we don't update mOffscreenFormat when we fallback to 32-bit RGB, so we'll still be trying to create the surface as 16-bit RGB565, with the wrong stride).
Summary: Crash on video with odd widths and NEON optimizations → Crash on video with odd widths and 16-bit displays
Created attachment 519310 [details] [diff] [review] Proposed fix Cairo doesn't expose the CAIRO_STRIDE_ALIGNMENT parameter it actually uses to decide on alignment, but instead requires callers to call cairo_format_stride_for_width() to get a valid stride, which we weren't doing. This patch adds the necessary call (wrapped in a static gfxASurface::FormatStrideForWidth() function). It also actually checks that the surface was created properly before trying to use it, and updates ConvertYCbCrToRGB565() to actually use the stride that was being passed to it, instead of always assuming that stride == picture width (modulo being in units of bytes vs. pixels).
Attachment #519310 - Flags: review?(jones.chris.g)
Attachment #519310 - Flags: review?(jones.chris.g) → review+
Created attachment 519522 [details] [diff] [review] Proposed fix (version 2) This is an updated version which removes the line - int bpp = gfxASurface::BytePerPixelFromFormat(format); That variable isn't used anymore after this patch, so removing it saves the function call and fixes a compiler warning.
Comment on attachment 519522 [details] [diff] [review] Proposed fix (version 2) Forgot to note, carrying forward r=cjones
Will take this patch if it lands soon, but won't block on the bug
tracking-fennec: 2.0+ → 2.0-
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.