Support GonkNativeWindow on gonk-JB (Android 4.3)

RESOLVED FIXED in Firefox 28

Status

defect
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
In spreadtrum Android 4.1(7710 for wcdma),the camera module can not work.
Duplicate of this bug: 871361
Dafeng, please provide logs or more information for analysis.
There are no plans for Android 4.1 support.
Component: General → DOM: Device Interfaces
Product: Boot2Gecko → Core
Morphing this bug to cover camera support on 4.2.
Blocks: gonk-jb
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)
Hardware: ARM → All
Assignee: nobody → sotaro.ikeda.g
when apply this patch, you can take pictures in Android 4.1 (sp7710), but the video recorder can not be used.
Dafeng, can you attach the error log file as well?
> 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.
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.
blocking-b2g: --- → koi?
Posted file the video recorder can work well (obsolete) —
when you apply this patch in dom/camera/GonkCameraSource.cpp, the video recorder can work well.
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...)
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.
(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.
(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?
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
Posted patch WIP for 4.2 camera/camcorder (obsolete) — Splinter Review
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)
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.
Posted file logcat on N4 (obsolete) —
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
(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.
> 
> 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 :)
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.
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.
I hope the preview problem will be fixed by Bug 858914.
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: mvines → sotaro.ikeda.g
> 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.
Attachment #772983 - Flags: feedback?(sotaro.ikeda.g) → feedback-
Attachment #772983 - Flags: feedback-
Attachment #774038 - Flags: feedback?(mvines)
- 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
Nexus4 Hw decoder's output video color format was 0x7FA30C03.
It is HAL_PIXEL_FORMAT_YCbCr_420_SP_TILED / 
OMX_QCOM_COLOR_FormatYUV420PackedSemiPlanar64x32Tile2m8ka
Confirmed video decoding via wifi web browsing by the method at Bug 887485 comment #3.
fix nit in TextureTargetForAndroidPixelFormat(android::PixelFormat aFormat)
Attachment #774281 - Attachment is obsolete: true
On my current nexus 4 hw composer is not used for rendering. The rendering problem is around OpenGL.
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.
One possible cause of attachment 774321 [details] is that correct shader is not chosen.
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
Duplicate of this bug: 878108
Summary: Support camera on gonk-JB (Android 4.2) → Support GonkNativeWindow on gonk-JB (Android 4.2)
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
(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,
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 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.
video rendering problem might be Bug 894891.
Depends on: 894891
(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.
Camera preview stops soon after preview start. Need to fix it.
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.
Depends on: 894262, 858914
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?
(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.
Summary: Support GonkNativeWindow on gonk-JB (Android 4.2) → Support GonkNativeWindow on gonk-JB (Android 4.3)
(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 ?
(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.
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.
Duplicate of this bug: 902255
Attachment #756320 - Attachment is obsolete: true
Attachment #763936 - Attachment is obsolete: true
Attachment #772983 - Attachment is obsolete: true
Component: DOM: Device Interfaces → General
Product: Core → Boot2Gecko
Attachment #773379 - Attachment is obsolete: true
Attachment #774321 - Attachment is obsolete: true
Depends on: 903174
Just moving current GonkNativeWindow under widget will be done in Bug 903174.
Depends on: 906888
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
Depends on: 907745
Posted patch let-camera-preview-works (obsolete) — Splinter Review
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.
(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!
No longer blocks: 909222
Duplicate of this bug: 909222
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.
Blocks: 909243
Depends on: 901224
No longer depends on: 858914
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
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
Depends on: 910498
(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?
Depends on: 909746
(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
> 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)
Add oem specific color formats.
Attachment #796443 - Attachment is obsolete: true
Depends on: 911518
Add oem specific color format to GrallocImage.
Attachment #798204 - Attachment is obsolete: true
Bug 910498 comment #21, it became clear that the video recording will become OK when Bug 910498 is fixed.
(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.
Depends on: 911548
Blocks: 911518
No longer depends on: 911518
Blocks: 911560
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
Update GonkCameraSource and GonkRecorder by android AOSP JB 4.3 source.
Attachment #798666 - Flags: review?(chris.double)
Attachment #798667 - Flags: review?(mhabicher)
Attachment #798668 - Flags: review?(mhabicher)
Attachment #798666 - Flags: review?(chris.double) → review+
Attachment #798670 - Flags: review?(mwu)
Attachment #798670 - Flags: review?(mwu) → review+
(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.
(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.
Attachment #798664 - Flags: review?(vladimir)
Attachment #798665 - Flags: review?(vladimir)
Move OEM specific pixel format into GrallocImage class.
Attachment #798669 - Attachment is obsolete: true
Attachment #798989 - Flags: review?(jmuizelaar)
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]
Attachment #799027 - Attachment is patch: true
Attachment #799027 - Attachment mime type: text/x-patch → text/plain
Attachment #799034 - Attachment is obsolete: true
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
Attachment #799036 - Flags: review?(vladimir)
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+
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)
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)
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
Re-created the patch depend on attachment 799088 [details] [diff] [review].
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)
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-
Remove "/widget/gonk/nativewindow/moz.build" from the patch. It is part2.
Attachment #799098 - Attachment is obsolete: true
Attachment #799098 - Flags: review?(vladimir)
Attachment #799401 - Flags: review?(vladimir)
>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)
> >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
Temporary patch to see the amount of changes in Makefile.in/
Blocks: 913279
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)
Depends on: 913821
Attachment #799663 - Attachment is obsolete: true
Apply Comment 91 and Comment 97.
Attachment #798670 - Attachment is obsolete: true
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)
:vlad, :mikeh, review ping.
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";
(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.
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 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+
Attachment #799088 - Flags: review?(vladimir) → review+
Attachment #798665 - Flags: review?(vladimir) → review+
> 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.
Apply the comments.
Attachment #798667 - Attachment is obsolete: true
Comment on attachment 802890 [details] [diff] [review]
patch part 4 v2 - Change to /dom/camera/

Carry "mhabicher: review+".
Attachment #802890 - Flags: review+
(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.
Apply the comments.
Attachment #799401 - Attachment is obsolete: true
Comment on attachment 802902 [details] [diff] [review]
patch part 1 v4 - GonkNativeWindow for JB

Carry "mwu: review+".
Attachment #802902 - Flags: review+
Attachment #798665 - Attachment is obsolete: true
Attachment #798666 - Attachment is obsolete: true
Attachment #798668 - Attachment is obsolete: true
Attachment #798989 - Attachment is obsolete: true
Attachment #799088 - Attachment is obsolete: true
Attachment #801263 - Attachment is obsolete: true
Attachment #802890 - Attachment is obsolete: true
Attachment #802902 - Attachment is obsolete: true
Committable patch. Base source of JB's GonkNativeWindow. Need to be hg blame as a reference information.
Rolled up committable patch. This patch depends on part 0.
Attachment #803333 - Attachment description: patch part0 - part7 rolled up patch → patch part1 - part7 rolled up patch
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4ab45ab4f80b
https://hg.mozilla.org/mozilla-central/rev/91d005745401
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2 FC (16sep)
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?
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)
Thanks for the response. On August, I just heard that 1.3 was going to support JB.
Duplicate of this bug: 806766
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.