Justin Dolske [:Dolske] Reporter
	Description • 14 years ago

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)

	Timothy B. Terriberry (:derf)
	Comment 1 • 14 years ago

This suggests the NEON detection code is failing. What's the contents of /proc/cpuinfo on your device?

	Makoto Kato [:m_kato] Assignee
	Comment 2 • 14 years ago

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.

	Doug Turner (:dougt)
	Comment 4 • 14 years ago

blocking for tegra 2 compat.

	Jeff Muizelaar [:jrmuizel]
	Comment 5 • 14 years ago

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.

	Makoto Kato [:m_kato] Assignee
	Comment 6 • 14 years ago

(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

	Makoto Kato [:m_kato] Assignee
	Comment 7 • 14 years ago

Also, scale function using NEON is bug 634557.

	Makoto Kato [:m_kato] Assignee
	Comment 8 • 14 years ago

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.

	Stuart Parmenter
	Comment 9 • 14 years ago

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

	Jeff Muizelaar [:jrmuizel]
	Comment 11 • 14 years ago

Comment on attachment 516198 [details] [diff] [review]
fix

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

	Makoto Kato [:m_kato] Assignee
	Comment 12 • 14 years ago

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?

	Timothy B. Terriberry (:derf)
	Comment 13 • 14 years ago

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.

	Joachim Herb
	Comment 14 • 14 years ago

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

	Joachim Herb
	Comment 15 • 14 years ago

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.

	Timothy B. Terriberry (:derf)
	Comment 16 • 14 years ago

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.

	Joachim Herb
	Comment 17 • 14 years ago

It looks like that patch (https://bugzilla.mozilla.org/attachment.cgi?id=518690) fixes the problem.