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

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Mike Hommey [:glandium]]

Attachment: 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

Comment 1

[Mike Hommey [:glandium]]

So that would be somewhere between f11f7ed625ba and a5413c3c1013

Comment 2

[Mike Hommey [:glandium]]

This could be related to bug 616469.

Comment 3

[Mike Hommey [:glandium]]

I could reproduce by forcing my X server to use a 16-bits display instead of 24.

Comment 4

[Matthew Gregan [:kinetik]]

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).

Comment 5

[Timothy B. Terriberry (:derf)]

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.

Comment 6

[Matthew Gregan [:kinetik]]

Ah yeah, silly me, sorry.

Comment 7

[Florian Hänel [:heeen]]

This also happens on Maemo.

Comment 8

[Timothy B. Terriberry (:derf)]

Really? It should only happen in Maemo if NEON detection fails. Or are you testing on something other than an N900?

Comment 9

[Florian Hänel [:heeen]]

Not testing on n900.

Comment 10

[Timothy B. Terriberry (:derf)]

Attachment: Fix

This one line change is enough to fix this for me.

Comment 11

[Timothy B. Terriberry (:derf)]

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.

Comment 12

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/1604da0cfed4

Comment 13

[u279076]

Can someone please point me to a test video I can use to verify this bug is fixed?

Comment 14

[Timothy B. Terriberry (:derf)]

Any video at all should do, for example the one linked in bug 641019: http://people.xiph.org/~tterribe/theora/coastguard_335-V500.ogv

Comment 15

[u279076]

(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.

Comment 16

[Timothy B. Terriberry (:derf)]

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.

Comment 17

[Mike Hommey [:glandium]]

I can confirm that rc1+patch works properly on 16-bits display.

Comment 18

[u279076]

Thanks Mike.