Crash on video with odd widths and 16-bit displays

RESOLVED FIXED

Status

()

Core
Audio/Video
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: derf, Assigned: derf)

Tracking

Trunk
ARM
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec-)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

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

Updated

7 years ago
Summary: Crash on video with odd widths and NEON optimizations → Crash on video with odd widths and 16-bit displays
(Assignee)

Comment 2

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

Updated

7 years ago
tracking-fennec: ? → 2.0+
(Assignee)

Comment 3

7 years ago
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.
Attachment #519310 - Attachment is obsolete: true
Attachment #519522 - Flags: review+
(Assignee)

Comment 4

7 years ago
Comment on attachment 519522 [details] [diff] [review]
Proposed fix (version 2)

Forgot to note, carrying forward r=cjones

Updated

7 years ago
Attachment #519522 - Flags: approval2.0+

Comment 5

7 years ago
Will take this patch if it lands soon, but won't block on the bug
tracking-fennec: 2.0+ → 2.0-

Comment 6

7 years ago
http://hg.mozilla.org/mozilla-central/rev/7d92741097d9
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.