Bad rendering of webm and ogv with 16-bits displays on Linux

VERIFIED FIXED in mozilla5

Status

()

Core
Audio/Video
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: glandium, Assigned: derf)

Tracking

Trunk
mozilla5
All
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

8 years ago
Created attachment 518356 [details]
Screenshot

A Debian user reported a problem with beta 12 that he verified to happen with Mozilla builds. He mentioned his CPU doesn't support SSE2, and that ogv rendering was working fine with older versions.
In fact, he narrowed down to a day it broke using nightlies:
The last working nightly is:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2010/12/2010-12-15-03-mozilla-central/firefox-4.0b9pre.en-US.linux-i686.tar.bz2
and the first broken one is:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2010/12/2010-12-16-03-mozilla-central/firefox-4.0b9pre.en-US.linux-i686.tar.bz2
(Reporter)

Comment 1

8 years ago
So that would be somewhere between f11f7ed625ba and a5413c3c1013
(Reporter)

Comment 2

8 years ago
This could be related to bug 616469.
(Reporter)

Comment 3

8 years ago
I could reproduce by forcing my X server to use a 16-bits display instead of 24.
Summary: Bad rendering of webm and ogv under some circumstances on Linux → Bad rendering of webm and ogv with 16-bits displays on Linux
(Reporter)

Updated

8 years ago
Hardware: x86 → All
The GTK reimplementation of GetOffscreenSurface will return ImageFormatRGB16_565 here:
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatformGtk.cpp#511

Then we'll hit this path: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicImages.cpp#206

And fire the assertion because have_ycbcr_to_rgb565() is only true if you have ARM NEON.  Since the assertion is non-fatal, we'll call ConvertYCbCrToRGB565 anyway, which does nothing unless HAVE_YCBCR_TO_RGB565 is defined (which is conditional on HAVE_ARM_NEON).
Blocks: 616469
(Assignee)

Comment 5

8 years ago
Technically that's not true, because above at
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicImages.cpp#155
we change the format to ImageFormatRGB24, since RGB565 isn't supported.

However, "format" is not "mOffscreenFormat", and so the eventual call to BasicPlanarYCbCrImage::GetAsSurface() will get data in a format that it is not at all expecting.

It seems like we should be calling SetOffscreenFormat() if we change the format. I don't know if that's guaranteed to be correct in all cases, but I've already tested it as part of my upcoming patch for bug 634557 and it appears to work.
Ah yeah, silly me, sorry.
This also happens on Maemo.
(Assignee)

Comment 8

8 years ago
Really? It should only happen in Maemo if NEON detection fails. Or are you testing on something other than an N900?
Not testing on n900.
(Assignee)

Comment 10

8 years ago
Created attachment 518864 [details] [diff] [review]
Fix

This one line change is enough to fix this for me.
Assignee: nobody → tterribe
Status: NEW → ASSIGNED
(Assignee)

Updated

8 years ago
Attachment #518864 - Flags: approval2.0?
(Assignee)

Comment 11

8 years ago
Comment on attachment 518864 [details] [diff] [review]
Fix

I should point out, for the purposes of evaluating approval, that this also fixes the crash in bug 641019 for systems without NEON.

Updated

7 years ago
Attachment #518864 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/1604da0cfed4
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Can someone please point me to a test video I can use to verify this bug is fixed?
(Assignee)

Comment 14

7 years ago
Any video at all should do, for example the one linked in bug 641019: http://people.xiph.org/~tterribe/theora/coastguard_335-V500.ogv
(In reply to comment #14)
> Any video at all should do, for example the one linked in bug 641019:
> http://people.xiph.org/~tterribe/theora/coastguard_335-V500.ogv

Thanks -- works for me using Firefox 4.0rc1.  However, I'm using a fairly modern machine. Perhaps someone could confirm this video working on a mobile device or computer which exhibited this bug before it was fixed.
(Assignee)

Comment 16

7 years ago
1) I don't think this patch is in RC1 (it landed on m-c, but not on mozilla-2.0), so you'd probably have to use a recent nightly to confirm (I assume nightlies are still being built from m-c? I don't actually know).

2) You'd have to set your display to 16 bits. You know you're testing the right code path if RC1 or a nightly prior to 2011-03-15 crashes on the above URL.
(Reporter)

Comment 17

7 years ago
I can confirm that rc1+patch works properly on 16-bits display.
Thanks Mike.
Status: RESOLVED → VERIFIED
(Reporter)

Updated

7 years ago
Target Milestone: --- → mozilla5
You need to log in before you can comment on or make changes to this bug.