All users were logged out of Bugzilla on October 13th, 2018

Support GonkNativeWindow on gonk-JB (Android 4.3)

RESOLVED FIXED in Firefox 28

Status

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Dafeng.Xu, Assigned: sotaro)

Tracking

unspecified
1.2 FC (16sep)
All
Gonk (Firefox OS)
Dependency tree / graph
Bug Flags:
in-moztrap -

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox28 fixed)

Details

(Whiteboard: burirun1)

Attachments

(2 attachments, 31 obsolete attachments)

129.93 KB, patch
Details | Diff | Splinter Review
208.28 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
In spreadtrum Android 4.1(7710 for wcdma),the camera module can not work.

Updated

6 years ago
Duplicate of this bug: 871361

Comment 2

6 years ago
Dafeng, please provide logs or more information for analysis.

Comment 3

6 years ago
There are no plans for Android 4.1 support.

Updated

6 years ago
Component: General → DOM: Device Interfaces
Product: Boot2Gecko → Core

Comment 4

6 years ago
Morphing this bug to cover camera support on 4.2.

Updated

6 years ago
Blocks: 784227
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: [tara] Gecko's Camera module can not run in Android 4.1 → Support camera on gonk-JB (Android 4.2)

Updated

6 years ago
Hardware: ARM → All
(Assignee)

Updated

6 years ago
Assignee: nobody → sotaro.ikeda.g
(Reporter)

Comment 5

5 years ago
Created attachment 756320 [details] [diff] [review]
the patch for camera in sp7710 Android 4.1

when apply this patch, you can take pictures in Android 4.1 (sp7710), but the video recorder can not be used.

Comment 6

5 years ago
Dafeng, can you attach the error log file as well?

Comment 7

5 years ago
> Dafeng, can you attach the error log file as well?
Please also provide us a recent build to flash our device to 4.1. And we can replace gecko to investigate the problem.
(Reporter)

Comment 8

5 years ago
Hi Ben, you can ask Keven Kuo to get the source code of sp8810, Android4.1.
Keven Kuo will give you 7710 device, source code and image.

Updated

5 years ago
blocking-b2g: --- → koi?
(Reporter)

Comment 10

5 years ago
Created attachment 763936 [details]
the video recorder can work well

when you apply this patch in dom/camera/GonkCameraSource.cpp, the video recorder can work well.
(Reporter)

Comment 11

5 years ago
If you apply the patchs in attachments, the camera can work well in Android 4.1, sp7710.
This patch is for spreadtrum sp7710 only, base on Android4.1.
(:sotaro -- just for FYI I've been hacking on this and should have something to show here soon.  Happy to take this bug for now...)

Comment 14

5 years ago
I can offer some information based on current status. Currently we didn't enable MOZ_B2G_CAMERA so the whole GonkXXX didn't been involved in build scope. Please see the below statement in gecko/dom/camera/moz.build

if CONFIG['MOZ_B2G_CAMERA']:
    CPP_SOURCES += [
        'GonkCameraManager.cpp',
        'GonkCameraControl.cpp',
        'GonkCameraHwMgr.cpp',
        'GonkNativeWindow.cpp',
        'GonkNativeWindowClient.cpp',
        'GonkRecorder.cpp',
        'GonkCameraSource.cpp',
        'AudioParameter.cpp',
        'GonkRecorderProfiles.cpp',
    ]

Once it enabled, lots of build fail messages showed up. Some of them are the location change of header file and the others are the change of class name and argument. Now I can build pass the environment. I can attach the patch file for the modification I did for build fail for sharing. For these patches, GonkNativeWindow changed function arguments so I filtered out some function calling in GonkNativeWindow.cpp. If I found more, I will leave info on comment in this bug for discussion.
So I have camera building and working on 4.2.  I hope to make time to start work on camcorder tomorrow.  My plan was to get camcorder working before sharing a WIP patch.  At that point we'd have a better idea of the differences between ICS and JB and can discuss how to clean up the patch and get it into a landable state.

Comment 16

5 years ago
(In reply to Michael Vines [:m1] [:evilmachines] from comment #15)
> So I have camera building and working on 4.2.  I hope to make time to start
> work on camcorder tomorrow.  My plan was to get camcorder working before
> sharing a WIP patch.  At that point we'd have a better idea of the
> differences between ICS and JB and can discuss how to clean up the patch and
> get it into a landable state.

As I said in Comment 14, GonkNativeWindow and GonkNativeWindowClient seems have many changes in their API between ICS and JB. When I checked the code, the GonkNativeWindow and GonkNativeWindowClient seems matched the SurfaceTexture layer in Android frameworks. I think it would be a better way to clean up this part from comparing both side.
(Assignee)

Comment 17

5 years ago
(In reply to Michael Vines [:m1] [:evilmachines] from comment #13)
> (:sotaro -- just for FYI I've been hacking on this and should have something
> to show here soon.  Happy to take this bug for now...)

Thanks. Can you take the bug?
(Assignee)

Comment 18

5 years ago
I feel that it is better to keep GonkNativeWindow and GonkNativeWindowClient for ICS and create new classes corresponding to them for JB.
Assignee: sotaro.ikeda.g → mvines
Created attachment 772983 [details] [diff] [review]
WIP for 4.2 camera/camcorder

This is a total WIP.  It will not work on ICS, but on 4.2 gets camera/camcorder working for me.  I'm curious how this patch works for the Nexus 4 folks.  I agree with Comment 18, but for now the raw ICS->JB diff that this patch presents should be more helpful for comprehension.
Attachment #772983 - Flags: feedback?(sotaro.ikeda.g)

Comment 20

5 years ago
MediaDebug.h isn't available on AOSP 4.2.2_r1. Seems like it was replaced by media/stagefright/foundation/ADebug.h.

I fixed that part up and it built. The camera apps doesn't work after building, though it was able to enumerate the a number of hardware features. I will attach some relevant looking logs from logcat.

Comment 21

5 years ago
Created attachment 773379 [details]
logcat on N4

Comment 22

5 years ago
Though for some reason, if you look at the preview in the task switcher, the preview does show up. While the camera app is actually loaded, the preview is black.
(In reply to Michael Wu [:mwu] from comment #22)
>
> Though for some reason, if you look at the preview in the task switcher, the
> preview does show up. While the camera app is actually loaded, the preview
> is black.

That sounds like the camera is working; according to bug 880780 comment 47, the GP/Peak has a black viewfinder problem as well. I wonder if this is related to bug 885345.
Sounds like Inder was seeing the same thing on one of our devices.  I'm still on a mozilla-central from July 1st (Happy Canada Day :mikeh!) and the patch is working great so we've picked up some regressions since it seems.
Depends on: 885345
(Assignee)

Comment 25

5 years ago
(In reply to Michael Vines [:m1] [:evilmachines] from comment #19)
> Created attachment 772983 [details] [diff] [review]
> WIP for 4.2 camera/camcorder
> 
> This is a total WIP.  It will not work on ICS, but on 4.2 gets
> camera/camcorder working for me.  I'm curious how this patch works for the
> Nexus 4 folks.  I agree with Comment 18, but for now the raw ICS->JB diff
> that this patch presents should be more helpful for comprehension.

It is very nice that attachment 772983 [details] [diff] [review] work camera on 4.2! 
For a production code, it might be better to use a way to keep as sync as possible to recent android code.
(Assignee)

Comment 26

5 years ago
> 
> It is very nice that attachment 772983 [details] [diff] [review] work camera
> on 4.2! 
> For a production code, it might be better to use a way to keep as sync as
> possible to recent android code.

I am going to implement a patch applying the above way.
Ah, cool.  Do you want to take this bug back then?  I'm not overly attached to it, I mainly just wanted to get the camera working :)
(Assignee)

Comment 28

5 years ago
Created attachment 774038 [details] [diff] [review]
WIP patch - update NativeWindow to Android 4.2

The patch work same level as comment #22 in Nexus 4. Black preview, but preview shows up in the task switcher.
<change>
-[1] Move GonkNativeWindow to widget/gonk/nativewindow
-[2] Add new GonkNativeWindow for JB as GonkNativeWindowJB.h/.cpp
     made by using 4.2.
-[3] Keep old GonkNativeWindow's implementation as GonkNativeWindowICS.h/.cpp
-[4] Update GonkRecorder and GonkCameraSource by using 4.2

I change the place of GonkNativeWindow from dom/camera to widget/gonk/nativewindow, because GonkNativeWindow is general platform dependent graphic thing. Should not be under dom/camera. I chose the place by talking with :jrmuizel.
(Assignee)

Comment 29

5 years ago
attachment 774038 [details] [diff] [review] can not built on b2g18. Though it is easier to fix because GonkNativeWindow is different between for b2g18 and for master.
(Assignee)

Comment 30

5 years ago
I hope the preview problem will be fixed by Bug 858914.
(Assignee)

Comment 31

5 years ago
Comment on attachment 774038 [details] [diff] [review]
WIP patch - update NativeWindow to Android 4.2

I think the way of attachment 774038 [details] [diff] [review] could reduce the problem in actual production. How do you think about it?
Attachment #774038 - Flags: feedback?(mvines)
(Assignee)

Updated

5 years ago
Assignee: mvines → sotaro.ikeda.g
(Assignee)

Comment 32

5 years ago
> Ah, cool.  Do you want to take this bug back then?  I'm not overly attached to
> it, I mainly just wanted to get the camera working :)

I take the bug again from comment by :m1. I am not sure why the feedback comment is not on bugzilla.
(Assignee)

Updated

5 years ago
Attachment #772983 - Flags: feedback?(sotaro.ikeda.g) → feedback-
(Assignee)

Updated

5 years ago
Attachment #772983 - Flags: feedback-
(Assignee)

Updated

5 years ago
Attachment #774038 - Flags: feedback?(mvines)
(Assignee)

Comment 33

5 years ago
Created attachment 774281 [details] [diff] [review]
patch v2 - update NativeWindow to Android 4.2

- enable MOZ_OMX_DECODER
- fix nit bugs

By applying the patch, confirmed hw codec works in nexus4. But videos were not rendered same as camera preview.
Attachment #774038 - Attachment is obsolete: true
(Assignee)

Comment 34

5 years ago
Nexus4 Hw decoder's output video color format was 0x7FA30C03.
It is HAL_PIXEL_FORMAT_YCbCr_420_SP_TILED / 
OMX_QCOM_COLOR_FormatYUV420PackedSemiPlanar64x32Tile2m8ka
(Assignee)

Comment 35

5 years ago
Confirmed video decoding via wifi web browsing by the method at Bug 887485 comment #3.
(Assignee)

Comment 36

5 years ago
Created attachment 774316 [details] [diff] [review]
patch v3 - update NativeWindow to Android 4.2

fix nit in TextureTargetForAndroidPixelFormat(android::PixelFormat aFormat)
Attachment #774281 - Attachment is obsolete: true
(Assignee)

Comment 37

5 years ago
On my current nexus 4 hw composer is not used for rendering. The rendering problem is around OpenGL.
(Assignee)

Comment 38

5 years ago
Created attachment 774321 [details]
A screenshot of youtube video playback

At first I thought that video is not rendered at all. But it is not correct. During youtube video playback, I recognized that some part of application area is renderd in video area. It seems that the texture is not bounded correctly to the buffer.
(Assignee)

Comment 39

5 years ago
One possible cause of attachment 774321 [details] is that correct shader is not chosen.

Comment 40

5 years ago
Sotaro: FYI, Your patch doesn't apply cleanly on our builds. It's missing support of these functions:
[1] setBuffersSize() and [2] updateBuffersGeometry().
[1] https://www.codeaurora.org/cgit/quic/la/platform/frameworks/native/tree/include/gui/ISurfaceTexture.h?h=b2g/jb_mr1_rb2.17#n173
[2] https://www.codeaurora.org/cgit/quic/la/platform/frameworks/native/tree/include/gui/ISurfaceTexture.h?h=b2g/jb_mr1_rb2.17#n177
(Assignee)

Updated

5 years ago
Duplicate of this bug: 878108
(Assignee)

Updated

5 years ago
Summary: Support camera on gonk-JB (Android 4.2) → Support GonkNativeWindow on gonk-JB (Android 4.2)

Comment 42

5 years ago
After fixing Comment 40. I am getting "unhandled program type" [1] when i try to play a recorded video.
[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/Effects.h#259

Crash reason:  SIGSEGV
Crash address: 0x0

Thread 21 (crashed)
 0  libxul.so!mozilla::layers::CreateTexturedEffect(mozilla::layers::TextureHost*, mozilla::layers::TextureHost*, mozilla::gfx::Filter const&) [Effects.h : 259 + 0x4]
     r4 = 0x47cff274    r5 = 0x46efeee0    r6 = 0x00000000    r7 = 0x3f800000
     r8 = 0x00000000    r9 = 0x47cff234   r10 = 0x47cff31c    fp = 0x47cff340
     sp = 0x47cff230    lr = 0x41658d63    pc = 0x41658ea4
    Found by: given as instruction pointer in context
 1  libxul.so!mozilla::layers::ImageHostSingle::Composite(mozilla::layers::EffectChain&, float, mozilla::gfx::Matrix4x4 const&, mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::Filter const&, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const&, nsIntRegion const*, mozilla::layers::TiledLayerProperties*) [Effects.h : 269 + 0xb]
     r4 = 0x494a10c0    r5 = 0x4b8d5c00    r6 = 0x00000000    r7 = 0x47cff3cc
     r8 = 0x494a10c0    r9 = 0x485ebbe0   r10 = 0x47cff320    fp = 0x47cff340
     sp = 0x47cff258    pc = 0x4165db2d
    Found by: call frame info
 2  libxul.so!mozilla::layers::ImageLayerComposite::RenderLayer(nsIntPoint const&, nsIntRect const&) [ImageLayerComposite.cpp : 98 + 0x13]
     r4 = 0x4165dae9    r5 = 0x4b8d5c00    r6 = 0x00000000    r7 = 0x47cff3cc
     r8 = 0x494a10c0    r9 = 0x485ebbe0   r10 = 0x485ebbe0    fp = 0x00000000
     sp = 0x47cff300    pc = 0x4165dfcb
    Found by: call frame info
 3  libxul.so!void mozilla::layers::ContainerRender<mozilla::layers::ContainerLayerComposite>(mozilla::layers::ContainerLayerComposite*, nsIntPoint const&, mozilla::layers::LayerManagerComposite*, nsIntRect const&) [ContainerLayerComposite.cpp : 122 + 0xb]
     r4 = 0x4b8d5d90    r5 = 0x47cff600    r6 = 0x494c4000    r7 = 0x470d4150
     r8 = 0x47cff534    r9 = 0x485ebbe0   r10 = 0x485ebbe0    fp = 0x00000000
     sp = 0x47cff398    pc = 0x41656285
    Found by: call frame info
 4  libxul.so!void mozilla::layers::ContainerRender<mozilla::layers::ContainerLayerComposite>(mozilla::layers::ContainerLayerComposite*, nsIntPoint const&, mozilla::layers::LayerManagerComposite*, nsIntRect const&) [ContainerLayerComposite.cpp : 122 + 0xb]
     r4 = 0x494c4210    r5 = 0x47cff768    r6 = 0x478c8400    r7 = 0x470d4150
     r8 = 0x47cff69c    r9 = 0x485ebbe0   r10 = 0x485ebbe0    fp = 0x00000000
     sp = 0x47cff500    pc = 0x41656285
    Found by: call frame info
 5  libxul.so!void mozilla::layers::ContainerRender<mozilla::layers::ContainerLayerComposite>(mozilla::layers::ContainerLayerComposite*, nsIntPoint const&, mozilla::layers::LayerManagerComposite*, nsIntRect const&) [ContainerLayerComposite.cpp : 122 + 0xb]
     r4 = 0x478c8610    r5 = 0x47cff8d0    r6 = 0x493ca000    r7 = 0x470d4150
     r8 = 0x47cff804    r9 = 0x485ebbe0   r10 = 0x485ebbe0    fp = 0x00000001
     sp = 0x47cff668    pc = 0x41656285
    Found by: call frame info
(Assignee)

Comment 43

5 years ago
(In reply to Inder from comment #42)
> After fixing Comment 40. I am getting "unhandled program type" [1] when i
> try to play a recorded video.
> [1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/Effects.h#259
> 

It seems that you need to add a color format of hw codec video in following.
/gfx/layers/opengl/TextureHostOGL.cpp
- SurfaceFormatForAndroidPixelFormat()
- TextureTargetForAndroidPixelFormat()

I added following to the patch.
- HAL_PIXEL_FORMAT_YCbCr_420_SP_TILED     = 0x7FA30C03,

Comment 44

5 years ago
Thanks for the pointer Sotaro. We were missing:
HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS     = 0x7FA30C04,

After adding this it doesn't crash but still no audio/video.
(Assignee)

Comment 45

5 years ago
(In reply to Inder from comment #44)
> Thanks for the pointer Sotaro. We were missing:
> HAL_PIXEL_FORMAT_YCbCr_420_SP_VENUS     = 0x7FA30C04,
> 
> After adding this it doesn't crash but still no audio/video.

In nexus4, I confirm that no video rendering but audio out is enabled.
(Assignee)

Comment 46

5 years ago
video rendering problem might be Bug 894891.
(Assignee)

Updated

5 years ago
Depends on: 894891
(Assignee)

Comment 47

5 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #46)
> video rendering problem might be Bug 894891.

Confirmed that attachment 777293 [details] [diff] [review] in Bug 894891 fixes "the camera preview and video playback not shown" problem.
(Assignee)

Comment 48

5 years ago
Camera preview stops soon after preview start. Need to fix it.
(Assignee)

Comment 49

5 years ago
It might be better to implement master version of "Bug 879297 - ImageLayerOGL keeps lock on external buffers". But it need to wait Bug 858914 fixed.
(Assignee)

Updated

5 years ago
Depends on: 894262, 858914

Comment 50

5 years ago
Hi Sotaro,

May I know the status of this bug now? Thanks.

Q1: are patches ready for review and blocked by other bugs?

Q2: Since the Bug 898898 changed the build system to know SDK version 18 (4.3) only, it means that even your patches are ready and reviewed you still needs to port them into 4.3 too. So may I know your plan on porting to 4.3?
(Assignee)

Comment 51

5 years ago
(In reply to Marco Chen [:mchen] from comment #50)
> Hi Sotaro,
> 
> May I know the status of this bug now? Thanks.
> 
> Q1: are patches ready for review and blocked by other bugs?

Not ready for review. I did not polish the current patch because until 858914 is fix, we can not fix all problem related to porting. Current master already have a problem for video playback.

> 
> Q2: Since the Bug 898898 changed the build system to know SDK version 18
> (4.3) only, it means that even your patches are ready and reviewed you still
> needs to port them into 4.3 too. So may I know your plan on porting to 4.3?

I am going to do following order.

-[1] reconsider how to implement GonkNativeWindow on 4.3
   + Implementation will becomes similar to the pach for 4.2.
   + check consistency to new camera api.
-[2] create a patch for 4.3
-[3] fix bugs recognized on 4.2 and other bugs related to camera and video. Need to fix them after Bug 858914 is fixed.
   + Camera preview is stopped soon after the preview start.
   + Rendered video frame order is not correct during video playback.
-[4] Get review of the patch
-[5] Commit.
(Assignee)

Updated

5 years ago
Summary: Support GonkNativeWindow on gonk-JB (Android 4.2) → Support GonkNativeWindow on gonk-JB (Android 4.3)

Comment 52

5 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #51)
> (In reply to Marco Chen [:mchen] from comment #50)
> > Hi Sotaro,
> > 
> > May I know the status of this bug now? Thanks.
> > 
> > Q1: are patches ready for review and blocked by other bugs?
> 
> Not ready for review. I did not polish the current patch because until
> 858914 is fix, we can not fix all problem related to porting. Current master
> already have a problem for video playback.
> 
> > 
> > Q2: Since the Bug 898898 changed the build system to know SDK version 18
> > (4.3) only, it means that even your patches are ready and reviewed you still
> > needs to port them into 4.3 too. So may I know your plan on porting to 4.3?
> 
> I am going to do following order.
> 
> -[1] reconsider how to implement GonkNativeWindow on 4.3
>    + Implementation will becomes similar to the pach for 4.2.
>    + check consistency to new camera api.
> -[2] create a patch for 4.3

Will you do this based on current attachment(patch v3 - update NativeWindow to Android 4.2) in this bug ?

ISurfaceTexture is removed on 4.3 while IGraphicBufferProducer and Surface(inherits ANativeWindow directly now) are introduced instead. 

Besides, did you aware of other changes influence GonkNativeWindow ?

> -[3] fix bugs recognized on 4.2 and other bugs related to camera and video.
> Need to fix them after Bug 858914 is fixed.
>    + Camera preview is stopped soon after the preview start.
>    + Rendered video frame order is not correct during video playback.
> -[4] Get review of the patch
> -[5] Commit.

Seems GonkNativeWindow on both 4.2 and 4.3 can't really be verified and landed on Master before Bug 858914 is fixed ?
(Assignee)

Comment 53

5 years ago
(In reply to vlin from comment #52)
> 
> Will you do this based on current attachment(patch v3 - update NativeWindow
> to Android 4.2) in this bug ?

I am thinking about it now. For 4.2 JB's GonkNativeWindow, I used SurfaceTexture(GLConsumer) for it. This class is not ideal, because GonkNativeWindow never bind to OpenGL and EGL. Therefore I changed a lot to create GonkNativeWindow. To create GonkNativeWindow, use BufferItemConsumer might be a good way to go. But I do not decide it yet.
 
> ISurfaceTexture is removed on 4.3 while IGraphicBufferProducer and
> Surface(inherits ANativeWindow directly now) are introduced instead. 
> 
> Besides, did you aware of other changes influence GonkNativeWindow ?

Following pixel format is added. They are used for gralloc buffer.
- HAL_PIXEL_FORMAT_Y8
- HAL_PIXEL_FORMAT_Y16
- HAL_PIXEL_FORMAT_YCbCr_420_888

I am not sure we need to render these format in Firefox OS v1.2. To lock HAL_PIXEL_FORMAT_YCbCr_420_888 buffer, we need to use GraphicBuffer::lockYCbCr() instead of GraphicBuffer::lock().

We do not support ProCamera in v1.2. ProCamera is going to pass CpuConsumer directly. We need think about how to handle CpuConsumer until v1.3. During 1.2, we should understand at least how CpuConsumer is used.
 
> Seems GonkNativeWindow on both 4.2 and 4.3 can't really be verified and
> landed on Master before Bug 858914 is fixed ?

Yes. Until Bug 858914 is fixed, gralloc buffer handling is incorrect in gfx layers. And current gfx layer is very unstable like Bug 900012. I think Bug 858914 is going to reach to final soon.
(Assignee)

Comment 54

5 years ago
I created 2 diagrams around JB graphics. I am going to add more.
https://github.com/sotaroikeda/android-diagrams/wiki/Android-Diagrams
(In reply to Sotaro Ikeda [:sotaro] from comment #53)
> (In reply to vlin from comment #52)
> > 
> > Will you do this based on current attachment(patch v3 - update NativeWindow
> > to Android 4.2) in this bug ?
> 
> I am thinking about it now. For 4.2 JB's GonkNativeWindow, I used
> SurfaceTexture(GLConsumer) for it. This class is not ideal, because
> GonkNativeWindow never bind to OpenGL and EGL. Therefore I changed a lot to
> create GonkNativeWindow. To create GonkNativeWindow, use BufferItemConsumer
> might be a good way to go. But I do not decide it yet.
>  
ProCamera is using CpuConsumer and it looks like a special camera client.

In my opinion, video and normal camera client should use BufferItemConsumer because no needs for CPU lock or EGL binding stuffs.

https://android.googlesource.com/platform/frameworks/av/+/android-4.3_r2/camera/ICameraService.cpp Line 145.

> > ISurfaceTexture is removed on 4.3 while IGraphicBufferProducer and
> > Surface(inherits ANativeWindow directly now) are introduced instead. 
> > 
> > Besides, did you aware of other changes influence GonkNativeWindow ?
> 
> Following pixel format is added. They are used for gralloc buffer.
> - HAL_PIXEL_FORMAT_Y8
> - HAL_PIXEL_FORMAT_Y16
> - HAL_PIXEL_FORMAT_YCbCr_420_888
> 
> I am not sure we need to render these format in Firefox OS v1.2. To lock
> HAL_PIXEL_FORMAT_YCbCr_420_888 buffer, we need to use
> GraphicBuffer::lockYCbCr() instead of GraphicBuffer::lock().
> 
For android camera device API 3.0, we need to support flexible yuv(HAL_PIXEL_FORMAT_YCbCr_420_888). But android camera device API 2.0, we could support either flexible yuv or YV12 + NV21.

But I'm not sure what is current camera API version for b2g 1.2.

https://android.googlesource.com/platform/hardware/libhardware/+/4c543a1456cd34a94e2c3a09879aa65ed8cd2f3a/tests/camera2/CameraMetadataTests.cpp Line 134.

> We do not support ProCamera in v1.2. ProCamera is going to pass CpuConsumer
> directly. We need think about how to handle CpuConsumer until v1.3. During
> 1.2, we should understand at least how CpuConsumer is used.
>  
> > Seems GonkNativeWindow on both 4.2 and 4.3 can't really be verified and
> > landed on Master before Bug 858914 is fixed ?
> 
> Yes. Until Bug 858914 is fixed, gralloc buffer handling is incorrect in gfx
> layers. And current gfx layer is very unstable like Bug 900012. I think Bug
> 858914 is going to reach to final soon.

Updated

5 years ago
Duplicate of this bug: 902255
(Assignee)

Updated

5 years ago
Attachment #756320 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #763936 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #772983 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Component: DOM: Device Interfaces → General
Product: Core → Boot2Gecko
(Assignee)

Updated

5 years ago
Attachment #773379 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #774321 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 903174
(Assignee)

Comment 57

5 years ago
Just moving current GonkNativeWindow under widget will be done in Bug 903174.
(Assignee)

Updated

5 years ago
Depends on: 906888
(Assignee)

Comment 58

5 years ago
Created attachment 792579 [details] [diff] [review]
WIP patch v4 - update NativeWindow to Android 4.3

Work in prograss patch. Apply the patch depend on Bug 906888.
Following problems are recognized.
- Camera preview works, but stopps soon. Might be effect of Bug 901107/Bug901219 .
- Video playback with hw video codec works. But Bug 901224 is present.

This bug is still strongly affected by Bug 858914.
Attachment #774316 - Attachment is obsolete: true

Updated

5 years ago
Depends on: 907745
Created attachment 795260 [details] [diff] [review]
let-camera-preview-works

I found state of buffer reset to FREE but mAcquireCalled didn't reset to false. This would cause get NULL buffer at GonkBufferQueue::acquireBuffer funtion and then camera stop. So I set mAcquireCalled to false at dequeueBuffer and camera preview works now.
(Assignee)

Comment 60

5 years ago
(In reply to Morris Tseng [:mtseng] from comment #59)
> Created attachment 795260 [details] [diff] [review]
> let-camera-preview-works
> 
> I found state of buffer reset to FREE but mAcquireCalled didn't reset to
> false. This would cause get NULL buffer at GonkBufferQueue::acquireBuffer
> funtion and then camera stop. So I set mAcquireCalled to false at
> dequeueBuffer and camera preview works now.

Thanks!
Blocks: 909222

Updated

5 years ago
No longer blocks: 909222
Duplicate of this bug: 909222
(Assignee)

Comment 62

5 years ago
I think about attachment 795260 [details] [diff] [review]. mAcquireCalled is used to show the buffer owner ship(BufferQueue or Consumer). So it is not good to change it. Then I am going to change the code of GonkBufferQueue::acquireBuffer() to the following. Change the mSurfaceDescriptor not to related to mAcquireCalled. By this change, camera preview works normally.

---------------------------------------
    // check if queue is empty
    // In asynchronous mode the list is guaranteed to be one buffer
    // deep, while in synchronous mode we use the oldest buffer.
    if (!mQueue.empty()) {
        Fifo::iterator front(mQueue.begin());
        int buf = *front;

        if (mSlots[buf].mAcquireCalled) {
            buffer->mGraphicBuffer = NULL;
        } else {
            buffer->mGraphicBuffer = mSlots[buf].mGraphicBuffer;
        }
        buffer->mSurfaceDescriptor = mSlots[buf].mSurfaceDescriptor;
(In reply to Sotaro Ikeda [:sotaro] from comment #62)
> I think about attachment 795260 [details] [diff] [review]. mAcquireCalled is
> used to show the buffer owner ship(BufferQueue or Consumer). So it is not
> good to change it. Then I am going to change the code of

I agree with you.

> GonkBufferQueue::acquireBuffer() to the following. Change the
> mSurfaceDescriptor not to related to mAcquireCalled. By this change, camera
> preview works normally.
> 
> ---------------------------------------
>     // check if queue is empty
>     // In asynchronous mode the list is guaranteed to be one buffer
>     // deep, while in synchronous mode we use the oldest buffer.
>     if (!mQueue.empty()) {
>         Fifo::iterator front(mQueue.begin());
>         int buf = *front;
> 
>         if (mSlots[buf].mAcquireCalled) {
>             buffer->mGraphicBuffer = NULL;
>         } else {
>             buffer->mGraphicBuffer = mSlots[buf].mGraphicBuffer;
>         }
>         buffer->mSurfaceDescriptor = mSlots[buf].mSurfaceDescriptor;

I tried this change and my camera preview worked normally as well! Thanks.
(Assignee)

Updated

5 years ago
Blocks: 909243
(Assignee)

Updated

5 years ago
Depends on: 901224
No longer depends on: 858914
(Assignee)

Comment 64

5 years ago
Created attachment 796443 [details] [diff] [review]
WIP patch v5 - update NativeWindow to Android 4.3

The patch is depends on Bug 901224, Bug 907745 and fix of EGLMemory leak in a patch of Bug 907745. Confirmed camera preview, take a picture and video playback on nexus-4. But still failed video recording.
Attachment #792579 - Attachment is obsolete: true
Attachment #795260 - Attachment is obsolete: true
(Assignee)

Comment 65

5 years ago
Start Video recording was failed at nsVolumeService::GetVolumeByPath(). On nexus 4, sd card is not present and added internal storage by Bug 890144. It might affected to the failure. I will create a bug for it.

The failed file path was following.
- /storage/emulated/legacy/DCIM/100MZLLA/VID_0001.3gp
(Assignee)

Updated

5 years ago
Depends on: 910498

Comment 66

5 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #64)
> Created attachment 796443 [details] [diff] [review]
> WIP patch v5 - update NativeWindow to Android 4.3
> 
> The patch is depends on Bug 901224, Bug 907745 and fix of EGLMemory leak in
> a patch of Bug 907745. Confirmed camera preview, take a picture and video
> playback on nexus-4. But still failed video recording.

I'd tried the patch you offered but camera preview still not working. May I have chance to know your last commit id in Gecko you used?
(Assignee)

Updated

5 years ago
Depends on: 909746
(Assignee)

Comment 67

5 years ago
(In reply to Vincent Liu[:vliu] from comment #66)
>
> I'd tried the patch you offered but camera preview still not working. May I
> have chance to know your last commit id in Gecko you used?

Does it related to Bug 909746? .
I used the following. A little bit old. I am going to check the patches by using recent gecko
- 55d8cf7a8393916f8ff525640ca533263351212f
(Assignee)

Comment 68

5 years ago
> I'd tried the patch you offered but camera preview still not working. May I
> have chance to know your last commit id in Gecko you used?

nexus4/camera was did not work these days. Cause of the problems were different each days and nexus4/camera becomes to works again. I confirmed the camera by the following.
-[1] Update b2g src to latest
-[2] Set "layers.use-deprecated-textures=false" as in Bug 907745 comment #58
-[3] Apply attachment 797907 [details] [diff] [review] in Bug 910921.
-[4] Apply attachment 798141 [details] [diff] [review] in Bug 901224.
-[5] Apply attachment 796443 [details] [diff] [review] (this bug's patch)
(Assignee)

Comment 69

5 years ago
Created attachment 798204 [details] [diff] [review]
WIP patch v6 - update NativeWindow to Android 4.3

Add oem specific color formats.
Attachment #796443 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 911518
(Assignee)

Comment 70

5 years ago
Created attachment 798240 [details] [diff] [review]
WIP patch v7 - update NativeWindow to Android 4.3

Add oem specific color format to GrallocImage.
Attachment #798204 - Attachment is obsolete: true
(Assignee)

Comment 71

5 years ago
Bug 910498 comment #21, it became clear that the video recording will become OK when Bug 910498 is fixed.

Comment 72

5 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #67)
> (In reply to Vincent Liu[:vliu] from comment #66)
> >
> > I'd tried the patch you offered but camera preview still not working. May I
> > have chance to know your last commit id in Gecko you used?
> 
> Does it related to Bug 909746? .
> I used the following. A little bit old. I am going to check the patches by
> using recent gecko
> - 55d8cf7a8393916f8ff525640ca533263351212f

Yes, I think it is the bug I missed. Thanks for you great help.
(Assignee)

Updated

5 years ago
Depends on: 911548
(Assignee)

Updated

5 years ago
Blocks: 911518
No longer depends on: 911518
(Assignee)

Updated

5 years ago
Blocks: 911560
(Assignee)

Comment 73

5 years ago
Created attachment 798664 [details] [diff] [review]
patch part 1 - GonkNativeWindow for JB

made classes from the following android classes.
android::BufferItemConsumer -> GonkNativeWindow
android::BufferQueue        -> GonkBufferQueue
android::ConsumerBase       -> GonkConsumerBase
android::Surface            -> GonkNativeWindowClient
Attachment #798240 - Attachment is obsolete: true
(Assignee)

Comment 74

5 years ago
Created attachment 798665 [details] [diff] [review]
patch part 2 - Change to /widget/gonk/nativewindow/moz.build
(Assignee)

Comment 75

5 years ago
Created attachment 798666 [details] [diff] [review]
patch part 3 - Change to /content/media/omx/
(Assignee)

Comment 76

5 years ago
Created attachment 798667 [details] [diff] [review]
patch part 4 - Change to /dom/camera/

Update GonkCameraSource and GonkRecorder by android AOSP JB 4.3 source.
(Assignee)

Comment 77

5 years ago
Created attachment 798668 [details] [diff] [review]
patch part 5 - Change to /dom/camera/moz.build
(Assignee)

Comment 78

5 years ago
Created attachment 798669 [details] [diff] [review]
patch part 6 - Add OEM specific HAL formats
(Assignee)

Comment 79

5 years ago
Created attachment 798670 [details] [diff] [review]
patch part 7 - Change to configure.in
(Assignee)

Updated

5 years ago
Attachment #798666 - Flags: review?(chris.double)
(Assignee)

Updated

5 years ago
Attachment #798667 - Flags: review?(mhabicher)
(Assignee)

Updated

5 years ago
Attachment #798668 - Flags: review?(mhabicher)

Updated

5 years ago
Attachment #798666 - Flags: review?(chris.double) → review+
(Assignee)

Updated

5 years ago
Attachment #798670 - Flags: review?(mwu)

Updated

5 years ago
Attachment #798670 - Flags: review?(mwu) → review+

Comment 80

5 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #73)
> Created attachment 798664 [details] [diff] [review]
> patch part 1 - GonkNativeWindow for JB
> 
> made classes from the following android classes.
> android::BufferItemConsumer -> GonkNativeWindow
> android::BufferQueue        -> GonkBufferQueue
> android::ConsumerBase       -> GonkConsumerBase
> android::Surface            -> GonkNativeWindowClient


May I ask a question?
Why you people prefer copying and pasting source code into gecko source tree from android source tree 
rather than using android source tree directly?
I think it will be great convenient when updating/patching the source code from android.
(Assignee)

Comment 81

5 years ago
(In reply to ying.xu from comment #80)
> May I ask a question?
> Why you people prefer copying and pasting source code into gecko source tree
> from android source tree 
> rather than using android source tree directly?
> I think it will be great convenient when updating/patching the source code
> from android.

If a change is trivial and do not depends on gecko's capability, I prefer patching to android source tree. If it is not, choose to copy and modify. In the GonkNativeWindow's case, The way of gralloc buffer allocation/free is changed a lot from original.
(Assignee)

Updated

5 years ago
Attachment #798664 - Flags: review?(vladimir)
(Assignee)

Updated

5 years ago
Attachment #798665 - Flags: review?(vladimir)
(Assignee)

Comment 82

5 years ago
Created attachment 798989 [details] [diff] [review]
patch part 6 v2 - Add OEM specific HAL formats

Move OEM specific pixel format into GrallocImage class.
Attachment #798669 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #798989 - Flags: review?(jmuizelaar)
(Assignee)

Comment 83

5 years ago
Created attachment 799027 [details] [diff] [review]
patch - actual change from android' one to GonkNativeWindow

I compared following to make clear the actual changes around GonkNativeWindow. They are made from android classes.
- [1] Just change file names and classes names from android's ones
   +android::BufferItemConsumer -> GonkNativeWindow
   +android::BufferQueue        -> GonkBufferQueue
   +android::ConsumerBase       -> GonkConsumerBase
   +android::Surface            -> GonkNativeWindowClient
- [2] GonkNativeWindow related classes in attachment 798664 [details] [diff] [review]
(Assignee)

Updated

5 years ago
Attachment #799027 - Attachment is patch: true
Attachment #799027 - Attachment mime type: text/x-patch → text/plain
(Assignee)

Comment 84

5 years ago
Created attachment 799034 [details]
patch - actual change from android' one to GonkNativeWindow
(Assignee)

Updated

5 years ago
Attachment #799034 - Attachment is obsolete: true
(Assignee)

Comment 85

5 years ago
Created attachment 799036 [details] [diff] [review]
patch v2 - actual change from android' one to GonkNativeWindow

Update the patch. To clarify more the actual change from attachment 799027 [details] [diff] [review], change the name BufferQueue to GonkBufferQueue in GonkConsumerBase.h/cpp in base source side.
Attachment #799027 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #799036 - Flags: review?(vladimir)
(Assignee)

Comment 86

5 years ago
Comment on attachment 798670 [details] [diff] [review]
patch part 7 - Change to configure.in

Mike, can you review the patch? Can you review it soon? Thanks.
Attachment #798670 - Flags: review?(mh+mozilla)
Attachment #798989 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 87

5 years ago
Comment on attachment 799036 [details] [diff] [review]
patch v2 - actual change from android' one to GonkNativeWindow

I am going to update the patch.
Attachment #799036 - Attachment is obsolete: true
Attachment #799036 - Flags: review?(vladimir)
(Assignee)

Comment 88

5 years ago
Comment on attachment 798664 [details] [diff] [review]
patch part 1 - GonkNativeWindow for JB

I am going to update the patch. Set obsolete.
Attachment #798664 - Attachment is obsolete: true
Attachment #798664 - Flags: review?(vladimir)
(Assignee)

Comment 89

5 years ago
Created attachment 799088 [details] [diff] [review]
patch part 0 - Add base source for GonkNativeWindow

GonkNativeWindow for JB is made from android's classes. This patch's classes is made just by changing the files' names and classes' names like the following. The source do not pass the build.
   +android::BufferItemConsumer -> GonkNativeWindow
   +android::BufferQueue        -> GonkBufferQueue
   +android::ConsumerBase       -> GonkConsumerBase
   +android::Surface            -> GonkNativeWindowClient
(Assignee)

Comment 90

5 years ago
Created attachment 799098 [details] [diff] [review]
patch part 1 v2 - GonkNativeWindow for JB

Re-created the patch depend on attachment 799088 [details] [diff] [review].
(Assignee)

Updated

5 years ago
Attachment #799088 - Attachment description: patch part 0 - add base source for GonkNativeWindow → patch part 0 - Add base source for GonkNativeWindow
Attachment #799088 - Flags: review?(vladimir)
(Assignee)

Updated

5 years ago
Attachment #799098 - Flags: review?(vladimir)
Comment on attachment 798670 [details] [diff] [review]
patch part 7 - Change to configure.in

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

::: configure.in
@@ +214,5 @@
>          AC_DEFINE(MOZ_OMX_DECODER)
>          AC_SUBST(MOZ_OMX_DECODER)
>          ;;
>      18)
> +        GONK_INCLUDES="-I$gonkdir/frameworks/native/include -I$gonkdir/frameworks/av/include -I$gonkdir/frameworks/av/include/media -I$gonkdir/frameworks/av/include/camera -I$gonkdir/frameworks/native/include/media/openmax -I$gonkdir/frameworks/av/media/libstagefright/include"

While you're here, I doubt you need these includes in the entire tree. In fact, I suspect you don't need them outside content/media. Could you change that to use LOCAL_INCLUDES there instead? (and do the corresponding change to GONK_INCLUDES above)

@@ +221,5 @@
>              MOZ_B2G_BT=1
>          fi
> +        MOZ_B2G_CAMERA=1
> +        MOZ_OMX_DECODER=1
> +        AC_DEFINE(MOZ_OMX_DECODER)

There are too few places using that define. Please just add:
ifdef MOZ_OMX_DECODER
DEFINES += -DMOZ_OMX_DECODER
endif

to the relevant Makefile.in files, and remove the AC_DEFINE here and above.
Attachment #798670 - Flags: review?(mh+mozilla) → review-
(Assignee)

Comment 92

5 years ago
Created attachment 799401 [details] [diff] [review]
patch part 1 v3 - GonkNativeWindow for JB

Remove "/widget/gonk/nativewindow/moz.build" from the patch. It is part2.
Attachment #799098 - Attachment is obsolete: true
Attachment #799098 - Flags: review?(vladimir)
(Assignee)

Updated

5 years ago
Attachment #799401 - Flags: review?(vladimir)
(Assignee)

Comment 93

5 years ago
>While you're here, I doubt you need these includes in the entire tree.
>In fact, I suspect you don't need them outside content/media.
>Could you change that to use LOCAL_INCLUDES there instead?
>(and do the corresponding change to GONK_INCLUDES above)

I did it locally and find out we need to add includes to following Makefile.in.
It seems code maintainance becomes more difficult. Is it OK to keep them in configure.in?
- /content/media/Makefile.in
- /content/media/omx/Makefile.in
- /content/media/omx/mediaresourcemanager/Makefile.in
- /content/media/webrtc/Makefile.in
- /dom/camera/Makefile.in
- /dom/media/Makefile.in
- /media/webrtc/Makefile.in
- /widget/gonk/Makefile.in

>There are too few places using that define. Please just add:
>ifdef MOZ_OMX_DECODER
>DEFINES += -DMOZ_OMX_DECODER
>endif

MOZ_OMX_DECODER is also referenced from /layout/build/Makefile.in.
  http://mxr.mozilla.org/mozilla-central/source/layout/build/Makefile.in

The Makefile.in do not have DEFINES, even when it regerences a lot of libraries.
Is it OK to add following to the /layout/build/Makefile.in?

> ifdef MOZ_OMX_DECODER
> DEFINES += -DMOZ_OMX_DECODER
> endif
Flags: needinfo?(mh+mozilla)
(Assignee)

Comment 94

5 years ago
> >There are too few places using that define. Please just add:
> >ifdef MOZ_OMX_DECODER
> >DEFINES += -DMOZ_OMX_DECODER
> >endif
> 
> MOZ_OMX_DECODER is also referenced from /layout/build/Makefile.in.
>   http://mxr.mozilla.org/mozilla-central/source/layout/build/Makefile.in

Actually here.
http://mxr.mozilla.org/mozilla-central/source/layout/build/Makefile.in#115
(Assignee)

Comment 95

5 years ago
Created attachment 799663 [details] [diff] [review]
patch - remove includes from configure.in

Temporary patch to see the amount of changes in Makefile.in/
(Assignee)

Updated

5 years ago
Blocks: 913279
(Assignee)

Comment 96

5 years ago
I tried to test the patch on recent master b2g code. But even the default recent b2g master does not work correctly... I can not even do unlock the phone.
(In reply to Sotaro Ikeda [:sotaro] from comment #93)
> >While you're here, I doubt you need these includes in the entire tree.
> >In fact, I suspect you don't need them outside content/media.
> >Could you change that to use LOCAL_INCLUDES there instead?
> >(and do the corresponding change to GONK_INCLUDES above)
> 
> I did it locally and find out we need to add includes to following
> Makefile.in.
> It seems code maintainance becomes more difficult. Is it OK to keep them in
> configure.in?
> - /content/media/Makefile.in
> - /content/media/omx/Makefile.in
> - /content/media/omx/mediaresourcemanager/Makefile.in
> - /content/media/webrtc/Makefile.in
> - /dom/camera/Makefile.in
> - /dom/media/Makefile.in
> - /media/webrtc/Makefile.in
> - /widget/gonk/Makefile.in

Skip that then.

> >There are too few places using that define. Please just add:
> >ifdef MOZ_OMX_DECODER
> >DEFINES += -DMOZ_OMX_DECODER
> >endif
> 
> MOZ_OMX_DECODER is also referenced from /layout/build/Makefile.in.
>   http://mxr.mozilla.org/mozilla-central/source/layout/build/Makefile.in
> 
> The Makefile.in do not have DEFINES, even when it regerences a lot of
> libraries.
> Is it OK to add following to the /layout/build/Makefile.in?
> 
> > ifdef MOZ_OMX_DECODER
> > DEFINES += -DMOZ_OMX_DECODER
> > endif

You only need that when there are C++ files built that do use it.
Flags: needinfo?(mh+mozilla)
(Assignee)

Updated

5 years ago
Depends on: 913821
(Assignee)

Updated

5 years ago
Attachment #799663 - Attachment is obsolete: true
(Assignee)

Comment 98

5 years ago
Created attachment 801263 [details] [diff] [review]
patch part 7 v2 - Change to configure.in and Makefile.in

Apply Comment 91 and Comment 97.
Attachment #798670 - Attachment is obsolete: true
(Assignee)

Comment 99

5 years ago
Comment on attachment 801263 [details] [diff] [review]
patch part 7 v2 - Change to configure.in and Makefile.in

Mike, can you review the patch again?
Attachment #801263 - Flags: review?(mh+mozilla)
(Assignee)

Comment 100

5 years ago
:vlad, :mikeh, review ping.

Comment 101

5 years ago
With these patches Video recorder is not working. It is because of two reasons. 
1) nsVolumeService::GetVolumeByPath checks for real path and in nexus-4 value name is not point to real path (/storage/emulated/legacy) this causes GetVolumeByPath always fails. Because of that Video recorder fail to start. 
You Can fix this by following solution. in /dom/system/gonk/nsVolumeService.cpp b/dom/system/gonk/nsVolumeService.cpp
     nsRefPtr<nsVolume> vol = mVolumeArray[volIndex];
     NS_ConvertUTF16toUTF8 volMountPointSlash(vol->MountPoint());
+    char volPathBuf[PATH_MAX];
+    realpath(volMountPointSlash.get(), volPathBuf);
+    volMountPointSlash.Assign(volPathBuf);

2) After the above change, Video recorder execution moves forward and it fails at open/creating the video file. The line is shown below.
  ScopedClose fd(open(nativeFilename.get(), O_RDWR | O_CREAT, 0644));
The reason for fail is "(13) Permission denied".
If hardcoded path is passed to open command like below and change the path permissions then recorder works fine.
nativeFilename = "/data/media/0/DCIM/100MZLLA/VID_0001.3gp";
(Assignee)

Comment 102

5 years ago
(In reply to Sridhar Ravipati from comment #101)
> With these patches Video recorder is not working. It is because of two
> reasons. 

Thanks for the comment. The volume problem is handled in Bug 910498.

Updated

5 years ago
Whiteboard: burirun1
Attachment #801263 - Flags: review?(mh+mozilla) → review+
Comment on attachment 798667 [details] [diff] [review]
patch part 4 - Change to /dom/camera/

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

r+ with my comments addressed, especially the copyright notice change!

::: dom/camera/GonkCameraHwMgr.cpp
@@ +194,5 @@
>  sp<GonkCameraHardware>
>  GonkCameraHardware::Connect(mozilla::nsGonkCameraControl* aTarget, uint32_t aCameraId)
>  {
> +#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 18
> +  sp<Camera> camera = Camera::connect(aCameraId, /*clientPackageName*/String16("gonk.camera"), Camera::USE_CALLING_UID);

Format the comment like this:
/*<space>clientPackagename<space>*/<space>...

::: dom/camera/GonkCameraSource.cpp
@@ +104,5 @@
>      }
>  }
>  
>  static int32_t getColorFormat(const char* colorFormat) {
> +    return OMX_COLOR_FormatYUV420SemiPlanar; //XXX TODO

Either remove this comment (and possibly this code), or describe very specifically what needs to get done to remove the TODO.

@@ +139,3 @@
>           "GonkCameraSource::getColorFormat", colorFormat);
>  
> +    CHECK(!"Unknown color format");

Really? This is AOSP's notation?

@@ +423,5 @@
>      return OK;
>  }
>  
>  /*
> + * Initialize the GonkCameraSource to so that it becomes

Since you're changing this, please remove the extra "to".

@@ +740,4 @@
>      return mIsMetaDataStoredInVideoBuffers;
>  }
>  
> +

Remove this extra blank line.

::: dom/camera/GonkRecorder.cpp
@@ +1,3 @@
>  /*
>   * Copyright (C) 2009 The Android Open Source Project
> + * Copyright (C) 2013 Mozilla Foundation

Why did you remove the CAF copyright notice?

@@ -373,5 @@
>      if (bytes <= 100 * 1024) {
> -        DOM_CAMERA_LOGW("Target file size (%lld bytes) is too small to be respected", bytes);
> -    }
> -
> -    if (bytes >= 0xffffffffLL) {

We can't remove this test, unless mMaxFileSizeBytes has become an int64_t somewhere that I can't find.

@@ +770,5 @@
>      // OMXClient::connect() always returns OK and abort's fatally if
>      // it can't connect.
>      OMXClient client;
>      // CHECK_EQ causes an abort if the given condition fails.
> +    CHECK_EQ(client.connect(), (status_t)OK);

Is the typecast required? If so, please make it use C++ style.

@@ +781,5 @@
>  }
>  
> +#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 18
> +status_t GonkRecorder::startAACRecording() {
> +    // FIXME:

If this is still a valid FIXME, please file a bug for it.

@@ +970,5 @@
>      }
>  }
>  
>  status_t GonkRecorder::checkVideoEncoderCapabilities() {
> +    //if (!mCaptureTimeLapse) {

Remove these commented out lines.

@@ +1253,5 @@
> +    // OMXClient::connect() always returns OK and abort's fatally if
> +    // it can't connect.
> +    OMXClient client;
> +    // CHECK_EQ causes an abort if the given condition fails.
> +    CHECK_EQ(client.connect(), (status_t)OK);

C++ style typecast here as well.

@@ +1263,5 @@
>  
> +    // Do not wait for all the input buffers to become available.
> +    // This give timelapse video recording faster response in
> +    // receiving output from video encoder component.
> +    //if (mCaptureTimeLapse) {

If this block should still be commented out, remove it and the associated comment.

::: dom/camera/GonkRecorder.h
@@ +180,5 @@
>      void clipAudioSampleRate();
>      void clipNumberOfAudioChannels();
>      void setDefaultProfileIfNecessary();
>  
> +

Remove this extra blank line.
Attachment #798667 - Flags: review?(mhabicher) → review+
Attachment #798668 - Flags: review?(mhabicher) → review+

Comment 104

5 years ago
Comment on attachment 799401 [details] [diff] [review]
patch part 1 v3 - GonkNativeWindow for JB

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

I think there's a bit more change here than strictly necessary - we have a proper stub which fixes the ATRACE stuff in widget/gonk/libui/cutils_trace.h. A bunch of the logging changes don't seem necessary. The EGL handling seemed to be largely NOP and didn't need to be removed. Sounds like the general architecture is going to be redone soon, however, so I won't be too picky. The core changes make sense to me.

::: widget/gonk/nativewindow/GonkConsumerBase.cpp
@@ +33,5 @@
> +#define CB_LOGV(...) __android_log_print(ANDROID_LOG_VERBOSE, LOG_TAG, __VA_ARGS__)
> +#define CB_LOGD(...) __android_log_print(ANDROID_LOG_DEBUG, LOG_TAG, __VA_ARGS__)
> +#define CB_LOGI(...) __android_log_print(ANDROID_LOG_INFO, LOG_TAG, __VA_ARGS__)
> +#define CB_LOGW(...) __android_log_print(ANDROID_LOG_WARN, LOG_TAG, __VA_ARGS__)
> +#define CB_LOGE(...) __android_log_print(ANDROID_LOG_ERROR, LOG_TAG, __VA_ARGS__)

Do we need to replace the logging here?

::: widget/gonk/nativewindow/GonkNativeWindowClientJB.h
@@ +19,5 @@
>  #define NATIVEWINDOW_GONKNATIVEWINDOWCLIENT_JB_H
>  
>  #include <gui/IGraphicBufferProducer.h>
> +//#include <gui/GLConsumer.h>
> +//#include <gui/BufferQueue.h>

Just delete these lines.

::: widget/gonk/nativewindow/GonkNativeWindowJB.cpp
@@ +26,5 @@
> +#define BI_LOGV(...) __android_log_print(ANDROID_LOG_VERBOSE, LOG_TAG, __VA_ARGS__)
> +#define BI_LOGD(...) __android_log_print(ANDROID_LOG_DEBUG, LOG_TAG, __VA_ARGS__)
> +#define BI_LOGI(...) __android_log_print(ANDROID_LOG_INFO, LOG_TAG, __VA_ARGS__)
> +#define BI_LOGW(...) __android_log_print(ANDROID_LOG_WARN, LOG_TAG, __VA_ARGS__)
> +#define BI_LOGE(...) __android_log_print(ANDROID_LOG_ERROR, LOG_TAG, __VA_ARGS__)

Do we need to replace the logging here?

@@ +38,3 @@
>  {
> +    //mBufferQueue->setConsumerUsageBits(consumerUsage);
> +    //mBufferQueue->setSynchronousMode(synchronousMode);

Delete these lines.
Attachment #799401 - Flags: review?(vladimir) → review+

Updated

5 years ago
Attachment #799088 - Flags: review?(vladimir) → review+

Updated

5 years ago
Attachment #798665 - Flags: review?(vladimir) → review+
(Assignee)

Comment 105

5 years ago
> r+ with my comments addressed, especially the copyright notice change!

ICS's GonkRecorder was made from codeaurora's StagefrightRecorder. JB's GonkRecorder was made from AOSP's StagefrightRecorder. Then codeaurora's copy right notice is not necessary.

> ::: dom/camera/GonkCameraHwMgr.cpp
> @@ +194,5 @@
> >  sp<GonkCameraHardware>
> >  GonkCameraHardware::Connect(mozilla::nsGonkCameraControl* aTarget, uint32_t aCameraId)
> >  {
> > +#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 18
> > +  sp<Camera> camera = Camera::connect(aCameraId, /*clientPackageName*/String16("gonk.camera"), Camera::USE_CALLING_UID);
> 
> Format the comment like this:
> /*<space>clientPackagename<space>*/<space>...

Will be fixed in next version

> ::: dom/camera/GonkCameraSource.cpp
> @@ +104,5 @@
> >      }
> >  }
> >  
> >  static int32_t getColorFormat(const char* colorFormat) {
> > +    return OMX_COLOR_FormatYUV420SemiPlanar; //XXX TODO
> 
> Either remove this comment (and possibly this code), or describe very
> specifically what needs to get done to remove the TODO.

Will update in next patch.

> @@ +139,3 @@
> >           "GonkCameraSource::getColorFormat", colorFormat);
> >  
> > +    CHECK(!"Unknown color format");
> 
> Really? This is AOSP's notation?

It's from AOSP.

> 
> @@ +423,5 @@
> >      return OK;
> >  }
> >  
> >  /*
> > + * Initialize the GonkCameraSource to so that it becomes
> Since you're changing this, please remove the extra "to".

It comes from AOSP. But the phrase is not correct.
Will be fixed in next patch.

> @@ +740,4 @@
> >      return mIsMetaDataStoredInVideoBuffers;
> >  }
> >  
> > +
> 
> Remove this extra blank line.

Will be fixed in next patch.

> ::: dom/camera/GonkRecorder.cpp
> @@ +1,3 @@
> >  /*
> >   * Copyright (C) 2009 The Android Open Source Project
> > + * Copyright (C) 2013 Mozilla Foundation
> 
> Why did you remove the CAF copyright notice?

ICS's GonkRecorder was made from codeaurora's StagefrightRecorder. JB's GonkRecorder was made from AOSP's StagefrightRecorder. Then codeaurora's copy right notice is not necessary.

> @@ -373,5 @@
> >      if (bytes <= 100 * 1024) {
> > -        DOM_CAMERA_LOGW("Target file size (%lld bytes) is too small to be respected", bytes);
> > -    }
> > -
> > -    if (bytes >= 0xffffffffLL) {
> 
> We can't remove this test, unless mMaxFileSizeBytes has become an int64_t
> somewhere that I can't find.

As I talked with you mMaxFileSizeBytes is 64 bits.

> @@ +770,5 @@
> >      // OMXClient::connect() always returns OK and abort's fatally if
> >      // it can't connect.
> >      OMXClient client;
> >      // CHECK_EQ causes an abort if the given condition fails.
> > +    CHECK_EQ(client.connect(), (status_t)OK);
> 
> Is the typecast required? If so, please make it use C++ style.

It is AOSP's implementation.

> 
> @@ +781,5 @@
> >  }
> >  
> > +#if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 18
> > +status_t GonkRecorder::startAACRecording() {
> > +    // FIXME:
> 
> If this is still a valid FIXME, please file a bug for it.

It's AOSP's comment.

> >  status_t GonkRecorder::checkVideoEncoderCapabilities() {
> > +    //if (!mCaptureTimeLapse) {

> 
> Remove these commented out lines.

Will update in next patch.

> 
> @@ +1253,5 @@
> > +    // OMXClient::connect() always returns OK and abort's fatally if
> > +    // it can't connect.
> > +    OMXClient client;
> > +    // CHECK_EQ causes an abort if the given condition fails.
> > +    CHECK_EQ(client.connect(), (status_t)OK);
> 
> C++ style typecast here as well.

Come from AOSP.

> @@ +1263,5 @@
> >  
> > +    // Do not wait for all the input buffers to become available.
> > +    // This give timelapse video recording faster response in
> > +    // receiving output from video encoder component.
> > +    //if (mCaptureTimeLapse) {
> 
> If this block should still be commented out, remove it and the associated
> comment.

Will be updated in next patch.

> 
> ::: dom/camera/GonkRecorder.h
> @@ +180,5 @@
> >      void clipAudioSampleRate();
> >      void clipNumberOfAudioChannels();
> >      void setDefaultProfileIfNecessary();
> >  
> > +
> 
> Remove this extra blank line.

Will be updated in next patch.
(Assignee)

Comment 106

5 years ago
Created attachment 802890 [details] [diff] [review]
patch part 4 v2 - Change to /dom/camera/

Apply the comments.
Attachment #798667 - Attachment is obsolete: true
(Assignee)

Comment 107

5 years ago
Comment on attachment 802890 [details] [diff] [review]
patch part 4 v2 - Change to /dom/camera/

Carry "mhabicher: review+".
Attachment #802890 - Flags: review+
(Assignee)

Comment 108

5 years ago
(In reply to Michael Wu [:mwu] from comment #104)
> Comment on attachment 799401 [details] [diff] [review]
> patch part 1 v3 - GonkNativeWindow for JB
> 
> Review of attachment 799401 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think there's a bit more change here than strictly necessary - we have a
> proper stub which fixes the ATRACE stuff in
> widget/gonk/libui/cutils_trace.h. A bunch of the logging changes don't seem
> necessary. The EGL handling seemed to be largely NOP and didn't need to be
> removed. Sounds like the general architecture is going to be redone soon,
> however, so I won't be too picky. The core changes make sense to me.

GonkNativeWindow is going to be updated again in near future. The above will be fixed at that time.

> ::: widget/gonk/nativewindow/GonkConsumerBase.cpp
> @@ +33,5 @@
> > +#define CB_LOGV(...) __android_log_print(ANDROID_LOG_VERBOSE, LOG_TAG, __VA_ARGS__)
> > +#define CB_LOGD(...) __android_log_print(ANDROID_LOG_DEBUG, LOG_TAG, __VA_ARGS__)
> > +#define CB_LOGI(...) __android_log_print(ANDROID_LOG_INFO, LOG_TAG, __VA_ARGS__)
> > +#define CB_LOGW(...) __android_log_print(ANDROID_LOG_WARN, LOG_TAG, __VA_ARGS__)
> > +#define CB_LOGE(...) __android_log_print(ANDROID_LOG_ERROR, LOG_TAG, __VA_ARGS__)
> 
> Do we need to replace the logging here?

Some how default log MACRO can not pass build. Then just change it to pass a build.
An the next GonkNativeWindow update, I am going to fix about it.

> ::: widget/gonk/nativewindow/GonkNativeWindowClientJB.h
> @@ +19,5 @@
> >  #define NATIVEWINDOW_GONKNATIVEWINDOWCLIENT_JB_H
> >  
> >  #include <gui/IGraphicBufferProducer.h>
> > +//#include <gui/GLConsumer.h>
> > +//#include <gui/BufferQueue.h>
> 
> Just delete these lines.

Will be changed in next patch.

> 
> ::: widget/gonk/nativewindow/GonkNativeWindowJB.cpp
> @@ +26,5 @@
> > +#define BI_LOGV(...) __android_log_print(ANDROID_LOG_VERBOSE, LOG_TAG, __VA_ARGS__)
> > +#define BI_LOGD(...) __android_log_print(ANDROID_LOG_DEBUG, LOG_TAG, __VA_ARGS__)
> > +#define BI_LOGI(...) __android_log_print(ANDROID_LOG_INFO, LOG_TAG, __VA_ARGS__)
> > +#define BI_LOGW(...) __android_log_print(ANDROID_LOG_WARN, LOG_TAG, __VA_ARGS__)
> > +#define BI_LOGE(...) __android_log_print(ANDROID_LOG_ERROR, LOG_TAG, __VA_ARGS__)
> 
> Do we need to replace the logging here?

Some how default log MACRO can not pass build. Then just change it to pass a build.
An the next GonkNativeWindow update, I am going to fix about it.

> 
> @@ +38,3 @@
> >  {
> > +    //mBufferQueue->setConsumerUsageBits(consumerUsage);
> > +    //mBufferQueue->setSynchronousMode(synchronousMode);
> 

Will be fixed in next patch.
> Delete these lines.
(Assignee)

Comment 109

5 years ago
Created attachment 802902 [details] [diff] [review]
patch part 1 v4 - GonkNativeWindow for JB

Apply the comments.
Attachment #799401 - Attachment is obsolete: true
(Assignee)

Comment 110

5 years ago
Comment on attachment 802902 [details] [diff] [review]
patch part 1 v4 - GonkNativeWindow for JB

Carry "mwu: review+".
Attachment #802902 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #798665 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #798666 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #798668 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #798989 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #799088 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #801263 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #802890 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #802902 - Attachment is obsolete: true
(Assignee)

Comment 112

5 years ago
Created attachment 803330 [details] [diff] [review]
patch part 0 v2 - Add base source for GonkNativeWindow

Committable patch. Base source of JB's GonkNativeWindow. Need to be hg blame as a reference information.
(Assignee)

Comment 113

5 years ago
Created attachment 803333 [details] [diff] [review]
patch part1 - part7 rolled up patch

Rolled up committable patch. This patch depends on part 0.
(Assignee)

Updated

5 years ago
Attachment #803333 - Attachment description: patch part0 - part7 rolled up patch → patch part1 - part7 rolled up patch
(Assignee)

Comment 114

5 years ago
The patchs are rolled up to two patches. part 0 is needed to remain as hg blame as a helpful information.
- patch part 0 v2 - Add base source for GonkNativeWindow (attachment 803330 [details] [diff] [review])
- patch part1 - part7 rolled up patch (attachment 803333 [details] [diff] [review] )
Keywords: checkin-needed

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4ab45ab4f80b
https://hg.mozilla.org/mozilla-central/rev/91d005745401
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2 FC (16sep)

Comment 117

5 years ago
After taking the patch (part0 and part1), i have an other issue which i have seen on Nexus-4 (mako).
After taking the picture, if the photo is opened from the thumbnail icons (present on the top of the same screen), the image is not seen in full; only the top left corner portion of the image is zoomed-in and displayed. After which we can not zoom-out to see the full image. 
Note : No issues in viewing the full image from Gallery.
blocking-b2g: koi? → 1.3?
(Assignee)

Comment 118

5 years ago
milan, why do you change the flag to '1.3?'?
Flags: needinfo?(milan)
It's JellyBean - that's 1.3 requirement.  I guess it went into m-c before the 1.2 branch, so it is in 1.2?  That should be fine.  If this stays fixed, it doesn't make a difference, but if it regresses, just tagging it that it's only needed for 1.3
blocking-b2g: 1.3? → 1.3+
Flags: needinfo?(milan)
(Assignee)

Comment 120

5 years ago
Thanks for the response. On August, I just heard that 1.3 was going to support JB.
status-firefox28: --- → fixed
(Assignee)

Updated

5 years ago
Duplicate of this bug: 806766

Updated

5 years ago
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.