[Gonk-L-Camera] : Video recording blocking in drawImage

VERIFIED FIXED in Firefox 39

Status

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: behsieh, Assigned: mwu)

Tracking

({crash})

unspecified
2.2 S8 (20mar)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox37 wontfix, firefox38 wontfix, firefox39 fixed, b2g-v2.2 verified, b2g-master verified)

Details

(Whiteboard: [b2g-crash][caf-crash 519][caf priority: p2][CR 798548])

Attachments

(5 attachments, 6 obsolete attachments)

59 bytes, text/x-github-pull-request
sotaro
: review+
Details | Review
52 bytes, text/x-github-pull-request
seinlin
: review+
Details | Review
2.00 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
105.41 KB, text/plain
Details
239.39 KB, text/plain
Details
(Reporter)

Description

4 years ago
because of this commit, makes L branch video blocking inside drawimage 

commit 6b1948cffc043c4a62d34a8598dcb28455957d45
Merge: 9f40063 9f353ff
Author: Wilson Page <wilsonpage@me.com>
Date:   Tue Jan 6 10:51:27 2015 +0000

    Merge pull request #26644 from jyavenard/master
    
    Bug 1101230: Properly create camera's preview images.
(Reporter)

Updated

4 years ago
Depends on: 1114477
Hi :jya

With the patch of Bug 1101230, Camera app can't generate thumbnail after recording in L build. For more detailed information for porting of L build, please see bug 1114477. The root cause would be blocking in CanvasRenderingContext2D::DrawImage(...).

https://dxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp#4304

drawImage never been re-called even it has represented the element is still loading from the last call. Would you please help to figure it out? Thanks
Flags: needinfo?(jyavenard)
Summary: Video recording blocking in drawimage → [Gonk-L-Camera] : Video recording blocking in drawImage
I'm not the best person to ask for this...

My earlier changes was a trivial change on the javascript side of things that used the wrong event to kick start the capture.

It was working with the dev build at the time (early December)
Flags: needinfo?(jyavenard)
(Reporter)

Comment 3

4 years ago
Attachment #8549331 - Flags: review?(sotaro.ikeda.g)
Attachment #8549331 - Flags: review?(dwilson)
(Reporter)

Updated

4 years ago
blocking-b2g: --- → 2.2?
(Reporter)

Updated

4 years ago
blocking-b2g: 2.2? → ---
Assign to :behsieh since he is working on this issue.
Assignee: nobody → behsieh
(Assignee)

Comment 5

4 years ago
Comment on attachment 8549331 [details] [review]
fix in recording thumbnail created issue

Cherry pick sotaro's fix. https://github.com/mozilla-b2g/platform_frameworks_av/commit/6a961033d917805b52c913936d3b9552c0a78f4a
Attachment #8549331 - Flags: review?(sotaro.ikeda.g)
Attachment #8549331 - Flags: review?(dwilson)
Attachment #8549331 - Flags: review-
(Reporter)

Comment 6

4 years ago
Hi Michael:
thanks for your information.
the commit you mention already phase in b2g/b2g-5.0.0_r6 as below

commit 16793265f0b6328942ab18f099ba57d036f03887
Author: Sotaro Ikeda <sikeda@mozilla.com>
Date:   Tue Sep 24 08:09:22 2013 -0700

    Bug 911548 - Add YCbCr_420_SP_TILED's color conversion

and for this fix.
in QCT platfrom , there is a new type of format(HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS) needs to support in nexus5 , that's why we phase in this code

thanks
(Reporter)

Comment 7

4 years ago
Hi Michael:
depend on comment 6 , please let me know your thought if any.
thank you
Flags: needinfo?(mwu)
specification of color format of HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS is not in public, though source code of converting YCbCr_420_SP_TILED is in public.
(Reporter)

Comment 9

4 years ago
Comment on attachment 8549331 [details] [review]
fix in recording thumbnail created issue

hi sotaro:
can you please help me review this code?
Or you can suggest right person to help review this code.
thanks
Attachment #8549331 - Flags: review- → review?(sotaro.ikeda.g)
Comment on attachment 8549331 [details] [review]
fix in recording thumbnail created issue

Looks good.

In the past, source code of almost all platforms specific color conversion were exposed in public as part of android::ColorConverter, But on recent androids, sadly, it is only convenient way to do color conversion of a platform specific color :-(
Attachment #8549331 - Flags: review?(sotaro.ikeda.g) → review+
(Assignee)

Comment 11

4 years ago
Comment on attachment 8549331 [details] [review]
fix in recording thumbnail created issue

I think this is actually a planar format, so it should be simple to add support.

Isn't this the result of bug 1117662 enabling venus, though? How come that bug affects us but not AOSP? I'm not sure the analysis there is sufficient. It also pulls in a CAF specific repo which is a clear sign something is wrong.
Flags: needinfo?(mwu)
Attachment #8549331 - Flags: superreview-
(Reporter)

Comment 12

4 years ago
Hi michael:
in bug 1117662 ,it was a fix which in camera ,camera needs to enable VENUS support to output 1080p.
in QCT platform, image height needs to divisible by 16,but 1080 dosent. enable VENUS could fix divisible issue.
pull code from CAF is because of Camera needs utils.mk to define VENUS, in AOSP there is no such define.
but it just a define. doesn't mean it can fix all of QCT platform depend issue.
the better way to get QCT depend source code is pull all the code from CAF.
but it just only for QCT platform.
AOSP BSP control really needs to plan well, or it will confuse all the function owner.
thanks
functionality failure. blocking 2.2+
blocking-b2g: --- → 2.2+
(Reporter)

Comment 14

4 years ago
hi michael:
since this is 2.2 blocking and code modify inside AOSP stagefright.
what is superreview- means?
what action do I need to take?
thanks
Flags: needinfo?(mwu)
Duplicate of this bug: 1124558
(Reporter)

Comment 16

4 years ago
hi Michael:
for your question
Isn't this the result of bug 1117662 enabling venus, though? 
Ans: this is not because of bug 1117662 regression issue.

How come that bug affects us but not AOSP? 
Ans: this is only happened in Nexus 5 device which we are working on L porting right now.

I'm not sure the analysis there is sufficient. It also pulls in a CAF specific repo which is a clear sign something is wrong.
Ans: since this fix is cherry-pick from flamekk, without this patch , video thumbnail will not generate.

thanks
I just built nexus-5-l with:
- gecko master:ba2f5fcd614709cb2ec0a69d685c89bc8d454c9b
- gaia  master:be64e0a1412c1b8c40b7597c77ed16841bdb152f

...and the front and back cameras work (take pictures, record video), although the app interface seems to freeze/become non-responsive when I stop recording. (Switching to the Homescreen and back seems to unstick it.)

Also, the Video apps plays the videos back with a green stripe down the side of the screen, and the Gallery app shows previews for the taken photos, but doesn't seem to be able to display them.
Confirmed: the photos are stored correctly, despite not appearing in the Gallery. Also, the recorded videos do contain a green stripe down their sides: on the left for the away-facing camera; on the right for the user-facing camera.
More data: the user-facing camera records at 1280x720 (i.e. 720p) rotated 90-degrees clockwise. The width of 720 includes the green stripe of pixels, which I measure to be 32 pixels wide (i.e. the normal part of the video frame is 688 pixels wide).
And for the 1920x1080 (i.e. 1080p) away-facing camera, the green stripe is (only) 16 pixels wide.
(Note that the green stripes do not appear in the camera viewfinder, only in the recorded video.)
(Reporter)

Comment 22

4 years ago
Posted patch I420colorconvert_0210.patch (obsolete) — Splinter Review
hi sotaro:
attached file is my patch which move I420colorconvert to gfx/layer
can you please help me to try it on your environment?
thanks
Flags: needinfo?(sotaro.ikeda.g)
mColorConverter is used in convertQCOMYUV420SemiPlanarVenus(). convertQCOMYUV420SemiPlanarVenus() is not part of GrallocImage instance.
Flags: needinfo?(sotaro.ikeda.g)
(Reporter)

Comment 24

4 years ago
use I420coloeconvert in gecko to do VENUS color convert.
Attachment #8549331 - Attachment is obsolete: true
Attachment #8561834 - Attachment is obsolete: true
Attachment #8562494 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8562494 [details] [diff] [review]
bug-1120780 color conversion in gecko layer

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

::: gfx/layers/GrallocImages.cpp
@@ +22,5 @@
>  
>  #define ALIGN(x, align) ((x + align - 1) & ~(align - 1))
>  
> +android::I420ColorConverterHelper mColorConverter;
> +

It causes, I420ColorConverterHelper allocation when content process is created. Then it causes unnecessary memory consumption and library load. We need to limit this only when it is necessary.
Attachment #8562494 - Flags: review?(sotaro.ikeda.g)
(Reporter)

Comment 26

4 years ago
move I420ColorConverterHelper into convertQCOMYUV420SemiPlanarVenus
Attachment #8562494 - Attachment is obsolete: true
Attachment #8562533 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8562533 [details] [diff] [review]
bug-1120780 move color conversion into gecko layer.

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

::: gfx/layers/GrallocImages.cpp
@@ +273,5 @@
> +static status_t convertYUV420Planar(
> +        const void* src,size_t srcWidth, size_t srcHeight,
> +        ARect src_crop,
> +        const void* dst,size_t dstWidth, size_t dstHeight,
> +        ARect dst_crop)

Can you explain why you need to add this function? Similar capability is already exit in gecko.

@@ +362,5 @@
> +    return OK;
> +}
> +
> +static status_t
> +convertQCOMYUV420SemiPlanarVenus(

It is not good that the function name have qcom and venus. The implementation does not depend on them.

@@ +373,5 @@
> +{
> +    int32_t err;
> +    uint32_t kI420BPP = 2;
> +    void* yuvbuffer;
> +    android::I420ColorConverterHelper mColorConverter;

It cause library's load and unload for every function call. I do not think it is a good idea.
Attachment #8562533 - Flags: review?(sotaro.ikeda.g) → review-
To protect I420ColorConverterHelper creation conflict. It seems better to add that mechanism to gfxAndroidPlatform. gfxAndroidPlatform  is derived class of gfxPlatform. That could make implementation simpler.
(Assignee)

Comment 29

4 years ago
Comment on attachment 8562533 [details] [diff] [review]
bug-1120780 move color conversion into gecko layer.

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

This is not actually address any of my objections earlier.
Attachment #8562533 - Flags: superreview-
(In reply to Michael Wu [:mwu] from comment #11)
> Comment on attachment 8549331 [details] [review]
> fix in recording thumbnail created issue
> 
> I think this is actually a planar format, so it should be simple to add
> support.
> 
> Isn't this the result of bug 1117662 enabling venus, though? How come that
> bug affects us but not AOSP? I'm not sure the analysis there is sufficient.
> It also pulls in a CAF specific repo which is a clear sign something is
> wrong.

It is not a result of bug 1117662. codeauroa uses VENUS color format for nexus-5's video decoding. codeaurora always use proprietary color format for hw video decodeing to improve the performance.
(Reporter)

Comment 32

4 years ago
hi michael:
do you have any suggest?
Do you want me to remove commit from bug 1117662 and do color convert in grallocimages?
(Reporter)

Comment 33

4 years ago
hi michael:
do you have any suggest?
Do you want me to remove commit from bug 1117662 and do color convert in grallocimages?
(Assignee)

Comment 34

4 years ago
(In reply to becker hsieh{:behsieh} from comment #32)
> hi michael:
> do you have any suggest?
> Do you want me to remove commit from bug 1117662 and do color convert in
> grallocimages?

If we can fix bug 1117662 differently, we should revert the change in bug 1117662.

Color conversion in the stagefright side is preferred. Refer to the code in comment 31. This is just a planar format with different padding/alignment requirements.
Flags: needinfo?(mwu)
(Reporter)

Comment 35

4 years ago
Posted file Do color convert in stagefright (obsolete) —
Attachment #8568372 - Flags: review?(mwu)
Michael, could you help Becker review attachment 8568372 [details] [review]?
Flags: needinfo?(mwu)
(Reporter)

Comment 38

4 years ago
per discussion with sku. mwu wants to take this bug.
(Reporter)

Updated

4 years ago
Attachment #8568372 - Flags: review?(mwu)
(Assignee)

Comment 39

4 years ago
Assignee: behsieh → mwu
Attachment #8562533 - Attachment is obsolete: true
Attachment #8568372 - Attachment is obsolete: true
Flags: needinfo?(mwu)
Attachment #8577836 - Flags: review?(sotaro.ikeda.g)
(Assignee)

Comment 40

4 years ago
Attachment #8577838 - Flags: review?(kli)
(Assignee)

Comment 41

4 years ago
This puts our conversion code on the generic path for venus so it can use the lock_ycbcr path.
Attachment #8577840 - Flags: review?(sotaro.ikeda.g)
Attachment #8577838 - Flags: review?(kli) → review+
(Assignee)

Comment 42

4 years ago
Just updated the lock_ycbcr PR - I had the U and V planes swapped.
Comment on attachment 8577838 [details] [review]
Switch N5 to fork of hardware/qcom/display

FYI, you can specify the URL's content type as 'text/x-github-pull-request' and get an automatic redirect.
Attachment #8577840 - Flags: review?(sotaro.ikeda.g) → review+
Comment on attachment 8577836 [details] [review]
Add support for venus in lock_ycbcr

I also confirmed the patch work on nexus-5-l.
Attachment #8577836 - Flags: review?(sotaro.ikeda.g) → review+

Updated

4 years ago
Duplicate of this bug: 1133147
https://hg.mozilla.org/mozilla-central/rev/e86c7b372d7a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Please request b2g37 on this when you get a chance.
Flags: needinfo?(mwu)
Target Milestone: --- → 2.2 S8 (20mar)
attachment 8577840 [details] [diff] [review] seem to cause Bug 1144224. I am confirming it.
(In reply to Sotaro Ikeda [:sotaro] from comment #50)
> attachment 8577840 [details] [diff] [review] seem to cause Bug 1144224. I am
> confirming it.

I confirmed that attachment 8577840 [details] [diff] [review] causes the regression. I forgot that flame uses  HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS during the review:-(
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Carsten Book [:Tomcat] from comment #48)
> https://hg.mozilla.org/mozilla-central/rev/e86c7b372d7a

Ryan, can you backout the commit from m-c? This regression blocks a lot of b2g video's test :(
Flags: needinfo?(ryanvm)
Attachment 8577840 [details] [diff] causes a corrupt preview of the "big buck bunny" video in the Video app.

The matching code for attachment 8577836 [details] [review] in our tree is here [1].  Slightly different, but if I make the what's on CAF match attachment 8577836 [details] [review] then I see more misalignment of the preview.

Let's not uplift this to b2g37 until this potential regression is better understood please.  There's devices with :sku that you could try this patch on.

[1] https://www.codeaurora.org/cgit/quic/la/platform/hardware/qcom/display/tree/libgralloc/alloc_controller.cpp?h=LA.BR.1.2.3#n572
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Attachment 8577840 [details] [diff] causes a corrupt preview of the "big buck bunny" video in the Video app.

The matching code for attachment 8577836 [details] [review] in our tree is here [1].  Slightly different, but if I make the what's on CAF match attachment 8577836 [details] [review] then I see more misalignment of the preview.

Let's not uplift this to b2g37 until this potential regression is better understood please.  There's devices with :sku that you could try this patch on.

[1] https://www.codeaurora.org/cgit/quic/la/platform/hardware/qcom/display/tree/libgralloc/alloc_controller.cpp?h=LA.BR.1.2.3#n572
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fb7a59b725e
Flags: needinfo?(ryanvm)
Target Milestone: 2.2 S8 (20mar) → ---
Infrastructure failures with hg.mozilla.org stopped me from merging this over to m-c in time for the 4pm nightlies. (Though I guess the hg.m.o issues would likely break the nightlies entirely, so no still-broken nightlies would be pushed out.)
(In reply to Michael Vines [:m1] [:evilmachines] from comment #53)
> Attachment 8577840 [details] [diff] causes a corrupt preview of the "big
> buck bunny" video in the Video app.
> 
> The matching code for attachment 8577836 [details] [review] in our tree is here [1]. 
> Slightly different, but if I make the what's on CAF match attachment 8577836 [details] [review]
> [details] then I see more misalignment of the preview.
> 
> Let's not uplift this to b2g37 until this potential regression is better
> understood please.  There's devices with :sku that you could try this patch
> on.

It might be caused by stride problem.
Yeah it was definitely a stride problem introduced by that patch.
(Assignee)

Comment 59

4 years ago
This lets us fallback on lock_ycbcr when android::ColorConverter doesn't work. This avoids lock_ycbcr on devices where it isn't implemented or isn't implemented correctly.
Attachment #8577840 - Attachment is obsolete: true
Attachment #8579102 - Flags: review?(sotaro.ikeda.g)
Comment on attachment 8579102 [details] [diff] [review]
Fallback on lock_ycbcr conversion

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

The implementation looks good. But I confirmed that the patch caused the crash on master flame-kk. The crash stack was the following. On master flame-kk, mAllocMod->lock_ycbcr was nulptr. Then it caused the crash.


(gdb) info stack
#0  0x00000000 in ?? ()
#1  0xb4baf7e6 in android::GraphicBufferMapper::lockYCbCr (this=0xb21be9c8, handle=0xb0641110, usage=usage@entry=3, bounds=..., 
    ycbcr=ycbcr@entry=0xbefc883c) at frameworks/native/libs/ui/GraphicBufferMapper.cpp:95
#2  0xb4bae9ce in android::GraphicBuffer::lockYCbCr (this=0xb283b080, usage=usage@entry=3, rect=..., ycbcr=ycbcr@entry=0xbefc883c)
    at frameworks/native/libs/ui/GraphicBuffer.cpp:222
#3  0xb4bae9f4 in android::GraphicBuffer::lockYCbCr (this=<optimized out>, usage=usage@entry=3, ycbcr=ycbcr@entry=0xbefc883c)
    at frameworks/native/libs/ui/GraphicBuffer.cpp:208
#4  0xb54a960e in ConvertVendorYUVFormatToRGB565 (aMappedSurface=0xbefc882c, aSurface=0xb36ba170, aBuffer=...)
    at ../../../gecko/gfx/layers/GrallocImages.cpp:229
#5  mozilla::layers::GrallocImage::GetAsSourceSurface (this=0xafef2500) at ../../../gecko/gfx/layers/GrallocImages.cpp:411
#6  0xb54a0704 in mozilla::layers::ImageContainer::LockCurrentAsSourceSurface (this=0xb0dff140, aSize=aSize@entry=0xbefc8938, 
    aCurrentImage=aCurrentImage@entry=0xbefc8934) at ../../../gecko/gfx/layers/ImageContainer.cpp:293
#7  0xb54a6b0c in AutoLockImage (aSurface=0xbefc8924, aContainer=<optimized out>, this=0xbefc8930)
    at ../../../gecko/gfx/layers/ImageContainer.h:599
#8  mozilla::layers::BasicImageLayer::Paint (this=0xb2140000, aDT=0xb09d81e0, aDeviceOffset=..., aMaskLayer=0x0)
    at ../../../gecko/gfx/layers/basic/BasicImageLayer.cpp:76
#9  0xb54c0044 in mozilla::layers::BasicLayerManager::PaintSelfOrChildren (this=this@entry=0xafef2d20, aPaintContext=..., 
    aGroupTarget=aGroupTarget@entry=0xb09d8240) at ../../../gecko/gfx/layers/basic/BasicLayerManager.cpp:807
#10 0xb54c028c in mozilla::layers::BasicLayerManager::PaintLayer (this=this@entry=0xafef2d20, aTarget=aTarget@entry=0xb09d8240, 
    aLayer=0xb2140000, aCallback=<optimized out>, aCallbackData=0xbefc9778)
Attachment #8579102 - Flags: review?(sotaro.ikeda.g)

Updated

4 years ago
Whiteboard: [CR 809989]
(Assignee)

Comment 61

4 years ago
Hmm, I can't reproduce a crash with my new patch. What test video are you using? When I record a video with the new patch, there's no crash. But if I force the code to take the lock_ycbcr path by removing the venus entry from the GrallocImage::sColorIdMap table, it crashes. AFAICT, the patch works as expected on flame kk..
Flags: needinfo?(mwu) → needinfo?(sotaro.ikeda.g)
(Assignee)

Comment 62

4 years ago
This patch replaces the old one, so make sure you have the venus entry in sColorIdMap.
(FWIW, pulling in just attachment 8579102 [details] [diff] [review] didn't reproduce the issue I noted in comment 54 on devices here)
Whiteboard: [CR 809989] → [caf priority: p2][CR 809989]
(In reply to Michael Wu [:mwu] from comment #61)
> Hmm, I can't reproduce a crash with my new patch. What test video are you
> using? When I record a video with the new patch, there's no crash. But if I
> force the code to take the lock_ycbcr path by removing the venus entry from
> the GrallocImage::sColorIdMap table, it crashes. AFAICT, the patch works as
> expected on flame kk..

Sorry, somehow, applying the patch failed to update the ROM on my environment :-( After removing the all intermediate files and build, the crash did not happen.
Flags: needinfo?(sotaro.ikeda.g)
Attachment #8579102 - Flags: review+

Updated

4 years ago
Whiteboard: [caf priority: p2][CR 809989] → [caf priority: p2][CR 798548]
Whiteboard: [caf priority: p2][CR 798548] → [b2g-crash][caf-crash 519][caf priority: p2][CR 798548]
Keywords: crash
https://hg.mozilla.org/mozilla-central/rev/8368cc78389e
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S8 (20mar)
please uplift to 2.2 since it is a 2.2 blocker. Thanks

NI myself for following verification.
Flags: needinfo?(hcheng)
Use below build to verify this bug. After recording, the thumbnail was created correctly and Video app can show thumbnail of each video file. Michael, could you please uplift to 2.2? Thanks.

* Env info:
Build ID               20150322160204
Gaia Revision          9b6f3024e4d0e62dd057231f4b14abe1782932ab
Gaia Date              2015-03-22 10:09:18
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/e730012260a4
Gecko Version          39.0a1
Device Name            hammerhead
Firmware(Release)      5.0
Firmware(Incremental)  eng.cltbld.20150322.191717
Firmware Date          Sun Mar 22 19:17:33 EDT 2015
Bootloader             HHZ12d
Flags: needinfo?(hcheng) → needinfo?(mwu)
(In reply to Hermes Cheng[:hermescheng] from comment #70)
> please uplift to 2.2 since it is a 2.2 blocker. Thanks
> 
> NI myself for following verification.

Micahel, could you help uplift?
(Assignee)

Comment 73

4 years ago
A sheriff will uplift once there is approval.
Flags: needinfo?(mwu)
(Assignee)

Comment 74

4 years ago
Comment on attachment 8577838 [details] [review]
Switch N5 to fork of hardware/qcom/display

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Not a regression.
User impact if declined: No thumbnails on videos in the video app on the N5 L port.
Testing completed: Verified on master.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.
Attachment #8577838 - Flags: approval-mozilla-b2g37?
(Assignee)

Comment 75

4 years ago
Comment on attachment 8579102 [details] [diff] [review]
Fallback on lock_ycbcr conversion

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Not a regression.
User impact if declined: No thumbnails on videos in the video app on the N5 L port.
Testing completed: Verified on master. Tested on Flame and N5, and m1 has also tested it on his device.
Risk to taking this patch (and alternatives if risky): Devices without an implementation of lock_ycbcr may crash. This crash indicates a real bug that needs to be fixed regardless though, so that's actually a good thing.
String or UUID changes made by this patch: None.
Attachment #8579102 - Flags: approval-mozilla-b2g37?
Attachment #8577838 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8579102 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
NI myself for 2.2 verification. It seems not included in today's pvt build.
Flags: needinfo?(hcheng)
2.2 verified:

Build ID               20150326164141
Gaia Revision          6d0174e28576f2f93e696a43d1ac3b03340117f6
Gaia Date              2015-03-26 21:47:33
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/820c2f2e817a
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.0
Firmware(Incremental)  eng.cltbld.20150326.200911
Firmware Date          Thu Mar 26 20:09:35 EDT 2015
Bootloader             HHZ12d
Status: RESOLVED → VERIFIED
Flags: needinfo?(hcheng)
See Also: → 1179162
You need to log in before you can comment on or make changes to this bug.