video app crashes allocating gralloc buffer for hw video codec

RESOLVED FIXED in mozilla24

Status

()

defect
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

({crash})

Trunk
mozilla24
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [b2g-crash], crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

+++ This bug was initially created as a clone of Bug #843599 +++

Since June/05, on master Firefox OS(gonk), when video app try to load hw codec, display becomes black and all b2g related process becomes dead. This problem happens by regression of Bug 843599.

The problem happens at gralloc buffer allocation in b2g process. b2g process aborted in layers::GrallocBufferActor::Create().
By temporarily changing "MOZ_NOT_REACHED("Unknown gralloc pixel format");" part in BytesPerPixelForPixelFormat(), video playback problem was fixed on buri device.
Blocks: 843599
Summary: implement gralloc and IPC backend for gl streaming → video app crashes allocating gralloc buffer for hw video codec
Severity: normal → critical
Crash Signature: [@ mozalloc_abort(char const*) | BytesPerPixelForPixelFormat(android::PixelFormat)]
Keywords: crash
Whiteboard: [b2g-crash]
Assignee: nobody → sotaro.ikeda.g
Add handling default pixel format. A size of the format is 0. It is used for gralloc buffers for hw codec. The codec normally uses vendor proprietary color format. B2g does not use this size for processing, but uses it just for tracking the allocation.
By applying the attachment 759461 [details] [diff] [review], the crash in b2g is fixed. But video update during video playback stopped soon after started the playback. It seems other defect. I am not sure it is related to Bug 843599.
Attachment #759461 - Flags: review?(vladimir)
Comment on attachment 759461 [details] [diff] [review]
patch - handle default pixel format in BytesPerPixelForPixelFormat()

Review of attachment 759461 [details] [diff] [review]:
-----------------------------------------------------------------

Can you add a comment in the default: block explaining why we're doing that instead of aborting?  Just something like "Likely a platform-specific hw decoder format"
Attachment #759461 - Flags: review?(vladimir) → review+
(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #5)
> Comment on attachment 759461 [details] [diff] [review]
> 
> Can you add a comment in the default: block explaining why we're doing that
> instead of aborting?  Just something like "Likely a platform-specific hw
> decoder format"

Yes, will add to a next patch.
Status: NEW → ASSIGNED
Posted patch Patch (obsolete) — Splinter Review
I think a proper fix should handle the YUV format explicitly, and update the calculation part as well.
(In reply to Chiajung Hung [:chiajung] from comment #7)
> Created attachment 759674 [details] [diff] [review]
> Patch
> 
> I think a proper fix should handle the YUV format explicitly, and update the
> calculation part as well.

If you want to handle YUV format, can you create another bug for it? This bug needs to be fixed as soon as possible.
Keywords: checkin-needed
(In reply to Chiajung Hung [:chiajung] from comment #7)
> Created attachment 759674 [details] [diff] [review]
> Patch

Some comments to the patch.

>diff --git a/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp b/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
>--- a/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
>+++ b/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp

>+  case 0x11: // NV21
>+  case 0x32315659: // YV12
>+    return -1;

Write concrete value need to be prevented.
Pixcel format is android's pixcel format definition.

OEM pixcel format range if from 0x100 to 0x1FF, within this range any color format could be here. Almost all cases are YUV related proprietary formats.

http://androidxref.com/4.0.4/xref/frameworks/base/libs/gui/SurfaceTexture.cpp#849
Attachment #759674 - Attachment is obsolete: true
(In reply to Sotaro Ikeda [:sotaro] from comment #10)
> (In reply to Chiajung Hung [:chiajung] from comment #7)
> >diff --git a/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp b/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
> >--- a/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
> >+++ b/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
> 
> >+  case 0x11: // NV21
> >+  case 0x32315659: // YV12
> >+    return -1;
> 
> Write concrete value need to be prevented.
> Pixcel format is android's pixcel format definition.
> 
> OEM pixcel format range if from 0x100 to 0x1FF, within this range any color
> format could be here. Almost all cases are YUV related proprietary formats.

I understand these values are come from system/core/include/system/graphics.h
(In reply to Sotaro Ikeda [:sotaro] from comment #12)
> > 
> > OEM pixcel format range if from 0x100 to 0x1FF, within this range any color
> > format could be here. Almost all cases are YUV related proprietary formats.
> 
> I understand these values are come from system/core/include/system/graphics.h

I thought only external format, 0x32315659(HAL_PIXEL_FORMAT_YV12) is not external format, so any value could be possible. Anyway, it is better to handle there formats in different bug.
Guys, please get something checked in today -- no reason to let things be broken.  We should handle whatever we can reasonably handle if the issue is the bytes-per-pixel function; at a minimum, we should return 0 for formats we don't immediately understand like the original patch does.  After that, we can look into a followup patch to extend the set formats the we do understand.
https://hg.mozilla.org/mozilla-central/rev/952393b787bf
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> By applying the attachment 759461 [details] [diff] [review], 

Sorry, I forgot to add comment to the patch. I created a patch that added a comment. But forgot to replace to the new one :-(
No problem; it sounds like there might be a followup anyway.  Thanks!
You need to log in before you can comment on or make changes to this bug.