871364 - Support GonkNativeWindow on gonk-JB (Android 4.3)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Dafeng Xu]

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

Comment 2

[Ben Tian (inactive)]

Dafeng, please provide logs or more information for analysis.

Comment 3

[Michael Wu [:mwu]]

There are no plans for Android 4.1 support.

Comment 4

[Michael Wu [:mwu]]

Morphing this bug to cover camera support on 4.2.

Comment 5

[Dafeng Xu]

Attachment: 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

[Ben Tian (inactive)]

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

Comment 7

[Ben Tian (inactive)]

> 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.

Comment 8

[Dafeng Xu]

Hi Ben, you can ask Keven Kuo to get the source code of sp8810, Android4.1.

Comment 9

[James Zhang (Spreadtrum)]

Keven Kuo will give you 7710 device, source code and image.

Comment 10

[Dafeng Xu]

Attachment: the video recorder can work well

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

Comment 11

[Dafeng Xu]

If you apply the patchs in attachments, the camera can work well in Android 4.1, sp7710.

Comment 12

[James Zhang (Spreadtrum)]

This patch is for spreadtrum sp7710 only, base on Android4.1.

Comment 13

[Michael Vines [:m1] [:evilmachines]]

(: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

[Vincent Liu[:vliu]]

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.

Comment 15

[Michael Vines [:m1] [:evilmachines]]

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

[Vincent Liu[:vliu]]

(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.

Comment 17

[Sotaro Ikeda [:sotaro]]

(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?

Comment 18

[Sotaro Ikeda [:sotaro]]

I feel that it is better to keep GonkNativeWindow and GonkNativeWindowClient for ICS and create new classes corresponding to them for JB.

Comment 19

[Michael Vines [:m1] [:evilmachines]]

Attachment: 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.

Comment 20

[Michael Wu [:mwu]]

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

[Michael Wu [:mwu]]

Attachment: logcat on N4

Comment 22

[Michael Wu [:mwu]]

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.

Comment 23

[Mike Habicher [:mikeh] (high bugzilla latency)]

(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.

Comment 24

[Michael Vines [:m1] [:evilmachines]]

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.

Comment 25

[Sotaro Ikeda [:sotaro]]

(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.

Comment 26

[Sotaro Ikeda [:sotaro]]

> 
> 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.

Comment 27

[Michael Vines [:m1] [:evilmachines]]

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 :)

Comment 28

[Sotaro Ikeda [:sotaro]]

Attachment: 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.

Comment 29

[Sotaro Ikeda [:sotaro]]

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.

Comment 30

[Sotaro Ikeda [:sotaro]]

I hope the preview problem will be fixed by Bug 858914.

Comment 31

[Sotaro Ikeda [:sotaro]]

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?

Comment 32

[Sotaro Ikeda [:sotaro]]

> 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.

Comment 33

[Sotaro Ikeda [:sotaro]]

Attachment: 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.

Comment 34

[Sotaro Ikeda [:sotaro]]

Nexus4 Hw decoder's output video color format was 0x7FA30C03.
It is HAL_PIXEL_FORMAT_YCbCr_420_SP_TILED / 
OMX_QCOM_COLOR_FormatYUV420PackedSemiPlanar64x32Tile2m8ka

Comment 35

[Sotaro Ikeda [:sotaro]]

Confirmed video decoding via wifi web browsing by the method at Bug 887485 comment #3.

Comment 36

[Sotaro Ikeda [:sotaro]]

Attachment: patch v3 - update NativeWindow to Android 4.2

fix nit in TextureTargetForAndroidPixelFormat(android::PixelFormat aFormat)

Comment 37

[Sotaro Ikeda [:sotaro]]

On my current nexus 4 hw composer is not used for rendering. The rendering problem is around OpenGL.

Comment 38

[Sotaro Ikeda [:sotaro]]

Attachment: 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.

Comment 39

[Sotaro Ikeda [:sotaro]]

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

Comment 40

[Inder]

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

Comment 42

[Inder]

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

Comment 43

[Sotaro Ikeda [:sotaro]]

(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

[Inder]

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.

Comment 45

[Sotaro Ikeda [:sotaro]]

(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.

Comment 46

[Sotaro Ikeda [:sotaro]]

video rendering problem might be Bug 894891.

Comment 47

[Sotaro Ikeda [:sotaro]]

(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.

Comment 48

[Sotaro Ikeda [:sotaro]]

Camera preview stops soon after preview start. Need to fix it.

Comment 49

[Sotaro Ikeda [:sotaro]]

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.

Comment 50

[Marco Chen [:mchen]]

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?

Comment 51

[Sotaro Ikeda [:sotaro]]

(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.

Comment 52

[Vincent Lin[:vilin]]

(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 ?

Comment 53

[Sotaro Ikeda [:sotaro]]

(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.

Comment 54

[Sotaro Ikeda [:sotaro]]

I created 2 diagrams around JB graphics. I am going to add more.
https://github.com/sotaroikeda/android-diagrams/wiki/Android-Diagrams

Comment 55

[Peter Chang[:pchang]]

(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.

Comment 57

[Sotaro Ikeda [:sotaro]]

Just moving current GonkNativeWindow under widget will be done in Bug 903174.

Comment 58

[Sotaro Ikeda [:sotaro]]

Attachment: 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.

Comment 59

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: 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.

Comment 60

[Sotaro Ikeda [:sotaro]]

(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!

Comment 62

[Sotaro Ikeda [:sotaro]]

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;

Comment 63

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(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.

Comment 64

[Sotaro Ikeda [:sotaro]]

Attachment: 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.

Comment 65

[Sotaro Ikeda [:sotaro]]

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

Comment 66

[Vincent Liu[:vliu]]

(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?

Comment 67

[Sotaro Ikeda [:sotaro]]

(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

Comment 68

[Sotaro Ikeda [:sotaro]]

> 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)

Comment 69

[Sotaro Ikeda [:sotaro]]

Attachment: WIP patch v6 - update NativeWindow to Android 4.3

Add oem specific color formats.

Comment 70

[Sotaro Ikeda [:sotaro]]

Attachment: WIP patch v7 - update NativeWindow to Android 4.3

Add oem specific color format to GrallocImage.

Comment 71

[Sotaro Ikeda [:sotaro]]

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

Comment 72

[Vincent Liu[:vliu]]

(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.

Comment 73

[Sotaro Ikeda [:sotaro]]

Attachment: 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

Comment 74

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Change to /widget/gonk/nativewindow/moz.build

Comment 75

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Change to /content/media/omx/

Comment 76

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 - Change to /dom/camera/

Update GonkCameraSource and GonkRecorder by android AOSP JB 4.3 source.

Comment 77

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 5 - Change to /dom/camera/moz.build

Comment 78

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 6 - Add OEM specific HAL formats

Comment 79

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 7 - Change to configure.in

Comment 80

[ying.xu]

(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.

Comment 81

[Sotaro Ikeda [:sotaro]]

(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.

Comment 82

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 6 v2 - Add OEM specific HAL formats

Move OEM specific pixel format into GrallocImage class.

Comment 83

[Sotaro Ikeda [:sotaro]]

Attachment: 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]

Comment 84

[Sotaro Ikeda [:sotaro]]

Attachment: patch - actual change from android' one to GonkNativeWindow

Comment 85

[Sotaro Ikeda [:sotaro]]

Attachment: 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.

Comment 86

[Sotaro Ikeda [:sotaro]]

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.

Comment 87

[Sotaro Ikeda [:sotaro]]

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

I am going to update the patch.

Comment 88

[Sotaro Ikeda [:sotaro]]

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

I am going to update the patch. Set obsolete.

Comment 89

[Sotaro Ikeda [:sotaro]]

Attachment: 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

Comment 90

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 v2 - GonkNativeWindow for JB

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

Comment 91

[Mike Hommey [:glandium]]

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.

Comment 92

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 v3 - GonkNativeWindow for JB

Remove "/widget/gonk/nativewindow/moz.build" from the patch. It is part2.

Comment 93

[Sotaro Ikeda [:sotaro]]

>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

Comment 94

[Sotaro Ikeda [:sotaro]]

> >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

Comment 95

[Sotaro Ikeda [:sotaro]]

Attachment: patch - remove includes from configure.in

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

Comment 96

[Sotaro Ikeda [:sotaro]]

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.

Comment 97

[Mike Hommey [:glandium]]

(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.

Comment 98

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 7 v2 - Change to configure.in and Makefile.in

Apply Comment 91 and Comment 97.

Comment 99

[Sotaro Ikeda [:sotaro]]

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?

Comment 100

[Sotaro Ikeda [:sotaro]]

:vlad, :mikeh, review ping.

Comment 101

[Sridhar Ravipati]

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";

Comment 102

[Sotaro Ikeda [:sotaro]]

(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.

Comment 103

[Mike Habicher [:mikeh] (high bugzilla latency)]

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.

Comment 104

[Michael Wu [:mwu]]

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.

Comment 105

[Sotaro Ikeda [:sotaro]]

> 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.

Comment 106

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 v2 - Change to /dom/camera/

Apply the comments.

Comment 107

[Sotaro Ikeda [:sotaro]]

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

Carry "mhabicher: review+".

Comment 108

[Sotaro Ikeda [:sotaro]]

(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.

Comment 109

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 v4 - GonkNativeWindow for JB

Apply the comments.

Comment 110

[Sotaro Ikeda [:sotaro]]

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

Carry "mwu: review+".

Comment 111

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=6fd777d1d778

Comment 112

[Sotaro Ikeda [:sotaro]]

Attachment: 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.

Comment 113

[Sotaro Ikeda [:sotaro]]

Attachment: patch part1 - part7 rolled up patch

Rolled up committable patch. This patch depends on part 0.

Comment 114

[Sotaro Ikeda [:sotaro]]

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] )

Comment 115

[Michael Wu [:mwu]]

I removed two hunks from the second patch which were *.orig backup files.

https://hg.mozilla.org/integration/b2g-inbound/rev/4ab45ab4f80b
https://hg.mozilla.org/integration/b2g-inbound/rev/91d005745401

Comment 116

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4ab45ab4f80b
https://hg.mozilla.org/mozilla-central/rev/91d005745401

Comment 117

[Brijesh]

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.

Comment 118

[Sotaro Ikeda [:sotaro]]

milan, why do you change the flag to '1.3?'?

Comment 119

[Milan Sreckovic [:milan] (needinfo for best results)]

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

Comment 120

[Sotaro Ikeda [:sotaro]]

Thanks for the response. On August, I just heard that 1.3 was going to support JB.