Closed Bug 637961 Opened 11 years ago Closed 11 years ago

Crash in [@ yv12_to_rgb565_neon]

Categories

(Core :: Audio/Video, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0
Tracking Status
fennec 2.0+ ---

People

(Reporter: Dolske, Assigned: m_kato)

References

Details

Attachments

(1 file)

I was investigating bug 627752 (unrelated) in a current Fennec nightly on my new Atrix, and am able to reliably crash on the mozilla.com testcase page there: http://www.mozilla.com/en-US/firefox/video/

In the stock configuration I couldn't get the video on that page to play at all, so Wes had me change media.preload.default to "2" (default is "1").

I also crash on http://people.opera.com/howcome/2010/video/norway/index.html, but interestingly not http://people.mozilla.com/~dolske/tmp/rickroll.ogg. 

Crash reports:

bp-3a7b2dd3-383f-44fb-a50f-a84d82110301 (mozilla.com)
bp-897cca7a-08b0-4d38-9096-698ed2110301 (opera.com)
This suggests the NEON detection code is failing. What's the contents of /proc/cpuinfo on your device?
Maybe, this is Tegra 2.

Also is have_ycbcr_to_rgb565 really called on all ARM build?  Bug 620526's fix is incorrect since have_ycbcr_to_rgb565 isn't used.
Assignee: nobody → m_kato
Status: NEW → ASSIGNED
Attached patch fixSplinter Review
Attachment #516198 - Flags: review?(jmuizelaar)
blocking for tegra 2 compat.
tracking-fennec: ? → 2.0+
Comment on attachment 516198 [details] [diff] [review]
fix

># HG changeset patch
># Parent dad15c7d80d7d0bbf7f2c02ca06ed2c5f8e00ede
>Bug 637961 - Crash in [@ yv12_to_rgb565_neon]
>
>diff --git a/gfx/layers/basic/BasicImages.cpp b/gfx/layers/basic/BasicImages.cpp
>--- a/gfx/layers/basic/BasicImages.cpp
>+++ b/gfx/layers/basic/BasicImages.cpp
>@@ -43,16 +43,17 @@
> 
> #ifdef XP_MACOSX
> #include "gfxQuartzImageSurface.h"
> #endif
> 
> #include "cairo.h"
> 
> #include "yuv_convert.h"
>+#include "ycbcr_to_rgb565.h"
> 
> #include "gfxPlatform.h"
> 
> using mozilla::Monitor;
> 
> namespace mozilla {
> namespace layers {
> 
>@@ -141,24 +142,23 @@ BasicPlanarYCbCrImage::SetData(const Dat
>   }
> 
>   gfxASurface::gfxImageFormat format = GetOffscreenFormat();
> 
>   // 'prescale' is true if the scaling is to be done as part of the
>   // YCbCr to RGB conversion rather than on the RGB data when rendered.
>   PRBool prescale = mScaleHint.width > 0 && mScaleHint.height > 0;
>   if (format == gfxASurface::ImageFormatRGB16_565) {
>-#ifndef HAVE_SCALE_YCBCR_TO_RGB565
>-    // yuv2rgb16 with scale function not yet available
>-    prescale = PR_FALSE;
>-#endif
>-#ifndef HAVE_YCBCR_TO_RGB565
>-    // yuv2rgb16 function not yet available for non-arm
>-    format = gfxASurface::ImageFormatRGB24;
>-#endif
>+    if (have_ycbcr_to_rgb565()) {
>+      // yuv2rgb16 with scale function not yet available for NEON
>+      prescale = PR_FALSE;

Won't this break if a yuv2rgb16 scale function becomes available?
It seems like a better solution to this would be to add a c implementation of the neon function.

However, I can be convinced to do this as a temporary solution.
Attachment #516198 - Flags: review?(jmuizelaar)
(In reply to comment #5)
> Won't this break if a yuv2rgb16 scale function becomes available?

Now, scale function for RGB16 isn't implemented yet and there is no HAVE_SCALE_YCBCR_TO_RGB565 macro.

If NEON, prescale have to be PR_FALSE.  If non-NEON, we should use RGB32 scale function.

> It seems like a better solution to this would be to add a c implementation of
> the neon function.

For NEON platform?  This bug is for non-NEON target such as Tegra 2.
Of course, at the feature, we should implement scale function using NEON.

 
> However, I can be convinced to do this as a temporary solution.

This is fennec-blocker and crash bug.  We need short team solution.  As long term such as Firefox 5, for performance, we will need scale function using NEON
Also, scale function using NEON is bug 634557.
Comment on attachment 516198 [details] [diff] [review]
fix

I reply your comment by comment #6.   I need a comment If review-.

Also, scale function using NEON is bug 634557.  This bug is for non-NEON platform.
Attachment #516198 - Flags: review?(jmuizelaar)
Whiteboard: [needs-review (jmuizelaar)]
jeff: for this release we mostly just need to fix the crash on devices without neon -- getting video working better (neon and not) will be a very high priority as soon as we release
Duplicate of this bug: 638800
Comment on attachment 516198 [details] [diff] [review]
fix

Ok. Please file a bug about adding a c implementation of ycbcr_to_rgb565.
Attachment #516198 - Flags: review?(jmuizelaar) → review+
Keywords: checkin-needed
Whiteboard: [needs-review (jmuizelaar)]
landed
http://hg.mozilla.org/mozilla-central/rev/c4bd627b9dce


> Ok. Please file a bug about adding a c implementation of ycbcr_to_rgb565.

NEON implementation is already filed as bug 634557.  Should I file non-NEON ycbcr_to_rgb565 for this?
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0
Depends on: 640964
Adding a C implementation is an easy side-effect of the current patch 634557, but it should have its own bug for tracking. I filed bug 640964 for it.
This patchset breaks the building of Thunderbird on Windows. As the mailnews and mail directories are not touched it is probably not a problem of Thunderbird.

With patchset 63335:6dab3f1dfa0e the compilation works, with 63336:c4bd627b9dce the compilation breaks with the following error message:

   Creating library thebes.lib and object thebes.exp
BasicImages.obj : error LNK2019: unresolved external symbol "int __cdecl have_ycbcr_to_rgb565(void)" (?have_ycbcr_to_rgb565@@YAHXZ) referenced in function "public: virtual void __thiscall mozilla::layers::BasicPlanarYCbCrImage::SetData(struct mozilla::layers::PlanarYCbCrImage::Data const &)" (?SetData@BasicPlanarYCbCrImage@layers@mozilla@@UAEXABUData@PlanarYCbCrImage@23@@Z)
thebes.dll : fatal error LNK1120: 1 unresolved externals
make[2]: *** [thebes.dll] Error 96
make[2]: Leaving directory `/c/Users/joachim/workspace/comm/obj-tb/mozilla/gfx/thebes'
make[1]: *** [libs] Error 2
make[1]: Leaving directory `/c/Users/joachim/workspace/comm/obj-tb/mozilla/gfx'
make: *** [all] Error 2

These are the settings in .mozconfig:
ac_add_options --enable-application=mail
ac_add_options --disable-static --disable-libxul
ac_add_options --disable-crashreporter
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-tb
mk_add_options MOZ_CO_PROJECT=mail
ac_add_options --enable-chrome-format=jar
ac_add_options --disable-angle

I use MozillaBuildSetup-1.5.1
More details of the build environment:
"Mozilla tools directory: c:\Users\joachim\mozilla-build\"
Windows SDK directory: C:\Program Files\Microsoft SDKs\Windows\v7.0\
Windows SDK version: 0.0
Platform SDK directory: C:\Program Files\Microsoft Platform SDK for Windows Server 2003 R2\
Platform SDK version: 5
Setting environment for using Microsoft Visual Studio 2008 x86 tools.
Mozilla build environment: MSVC version 9.
Can you test with the "Add ScaleYCbCrToRGB565" patch in bug 634557? That fixes a number of issues with this code, and eliminates have_ycbcr_to_rgb565() entirely.
It looks like that patch (https://bugzilla.mozilla.org/attachment.cgi?id=518690) fixes the problem.
You need to log in before you can comment on or make changes to this bug.