Use hw codec and camera hw in mediaserver process

RESOLVED FIXED in Firefox 22

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

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

Firefox Tracking Flags

(blocking-b2g:leo+, firefox20 wontfix, firefox21 wontfix, firefox22 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

Details

(Whiteboard: regression-risk [tech-bug])

Attachments

(10 attachments, 54 obsolete attachments)

4.00 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
3.43 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
54.63 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
2.96 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
45.80 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
1.24 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
630 bytes, patch
sotaro
: review+
Details | Diff | Splinter Review
2.58 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
45.12 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
1.19 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.4 (KHTML, like Gecko) Chrome/22.0.1229.94 Safari/537.4

Steps to reproduce:

FirefoxOS load camera hw within app process. This produce following problems.
 - request high privilege to app's process.
 - camera hw could be loaded from multiple app processes

I already found out a way to move hw codec to mediaserver process. If we could move camera hw to mediaserver process. We do not need a high privileged app process on FirefoxOS.
OS: Windows 7 → Gonk
Hardware: x86_64 → ARM
By this patch, we could move camera hw from app process to mediaserver process. And We do not need to apply a high privilege to camera app.

- GonkCameraHardware do not load camera hw directly, otherwise to use android::Camera class to access camera hw.
- change GonkNativeWindow to be drived from BnSurfaceTexture.
- addd GonkNativeWindowClient, it is derived from ANativeWindow
- do not apply high privilege to camera app
attachment 673138 [details] [diff] [review] is work in progress patch. 

There is a bug. After I launched camera app, preview video was shown, but soon after camera app process crashed. I am going to try to fix it next week.
In attachment 673138 [details] [diff] [review], GonkNativeWindow is split to GonkNativeWindow and GonkNativeWindowClient. They correspond to SurfaceTexture and SurfaceTextureClient in android.

It is necessary because, GonkNativeWindow need to communicate with SurfaceTextureClient in mediaserver process. Therefore I changed GonkNativeWindow and GonkNativeWindowClient as similar to SurfaceTexture and SurfaceTextureClient as possible.
GonkNativeWindowClient is used only in OmxDecoder. After applying attachment 673138 [details] [diff] [review], I confirmed that H.264 video could be played by using hw codec as before on my hardware.

I noticed that video app could sometimes be killed during seeking. But it also happen without the patch. This crush is because of incorrect Buffer freeing, I think.
> also happen without the patch. This crush is because of incorrect Buffer
> freeing, I think.

Correction, It seems that MeidaBuffer::release() is not called correctly during seeking.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Depends on: 796854
Start Binder ThreadPool to receive android Binder IPC from mediaserver.

Firefox OS already use Binder ipc to communicate with AudioFliner. But Binder ipc happens always from b2g to AudioFlinger synchronously. Binder ThreadPool is not necessary in this case. But b2g already could not receive Binder Died event from AudioFlinger if the ThreadPool is not present.
Disable to apply high privilege to app process that has "camera" or "deprecated-hwvideo" permission.
Get IOMX interface via OMXClient.

OMXClient get IOMX from mediaserver. OMX instance is in mediaserver process. Therefore the IOMX does not require high privilege even when IOMX handles a hw codec.
Get IOMX via OMXClient.

OMXClient get IOMX from mediaserver. OMX instance is in mediaserver process. 
Therefore the IOMX do not request high privilege even when IOMX handles hw codec.
Attachment #675067 - Attachment is patch: true
Attachment #675066 - Attachment is patch: true
- GonkCameraHardware do not load camera hw directly, otherwise to use android::Camera class to access camera hw.
- change GonkNativeWindow to be drived from BnSurfaceTexture.
- addd GonkNativeWindowClient, it is derived from ANativeWindow

android::Camera controls camera hardware via CameraService in mediaserver proces.
Therefore android::Camera does not request high privilege even when controling camera hw.

GonkNativeWindow is split to GonkNativeWindow and GonkNativeWindowClient. They correspond to SurfaceTexture and SurfaceTextureClient in android. GonkNativeWindow need to communicate with SurfaceTextureClient in mediaserver process. Therefore changed GonkNativeWindow and GonkNativeWindowClient as similar to SurfaceTexture and SurfaceTextureClient as possible.

But there are differences between GonkNativeWindow and SurfaceTexture. Some modifications to camera hw hal might be necessary for gonk hardwares.
some qcom's ics platform implementation request to implement NATIVE_WINDOW_SET_BUFFERS_SIZE. It is very platform dependent thing.

My phone requires it, then I created the patch. Other hardware do not need the patch.
Attachment #673138 - Attachment is obsolete: true
By using the patches, I confirmed that video play, camera preview and take photos.

Though still could not do recording. I do not have a time this week, I am going to try to fix it for my phone next week.
I do not have otoro nor unagi device. The patches should work also on them. Though some tweak to camera hw hal might be necessary.
Summary: move camera hw from app process to mediaserver process → Use hw codec and camera hw in mediaserver process
In attachment 675071 [details] [diff] [review], before to start preview, GonkCameraHardware try to free all buffers. It is not nice. It is better not to free buffer in this case.

>GonkCameraHardware::StartPreview()
>{
>  ReentrantMonitorAutoEnter mon(mReentrantMonitor);
>  mNativeWindow->freeAllBuffers();
>  mCamera->setPreviewTexture(mNativeWindow);
>  return mCamera->startPreview();
Posted file GonkNativeWindow.cpp (obsolete) —
supplement to attachment 675071 [details] [diff] [review]
It is not easy to understand the code only from it.
Posted file GonkNativeWindow.h (obsolete) —
supplement to attachment 675071 [details] [diff] [review]
It is not easy to understand the code only from it.
Posted file GonkNativeWindowClient.cpp (obsolete) —
supplement to attachment 675071 [details] [diff] [review]
It is not easy to understand the code only from it.
Posted file GonkNativeWindowClient.h (obsolete) —
supplement to attachment 675071 [details] [diff] [review]
It is not easy to understand the code only from it.
Video mode did not work at all on yesterday's source, because of bug 805377,  I assume.
Remove freeing all buffers before calling mCamera->startPreview() in GonkCameraHardware::StartPreview(). It is not necessary any more.

During development it was necessary to free all buffers before preview start. Otherwise CameraHardwareInterface try to dequeue GraphicBuffer by calling GonkNativeWindow::dequeueBuffer() until receive error. But the GonkNativeWindow did not return error. Then GonkNativeWindow::dequeueBuffer() was stucked forever.

Since implemented GonkNativeWindow correctly. GonkNativeWindow::dequeueBuffer() could return correct error code when CameraHardwareInterface try to dequeue too much.
Attachment #675071 - Attachment is obsolete: true
Posted file GonkNativeWindow.cpp (obsolete) —
update besed on attachment 676037 [details] [diff] [review]
Attachment #675079 - Attachment is obsolete: true
Posted file GonkNativeWindow.h (obsolete) —
update besed on attachment 676037 [details] [diff] [review]
Attachment #675080 - Attachment is obsolete: true
update & fix precondition check in GonkNativeWindow::setBufferCount()

when buffer is RENDERING stage, the buffer is server side, not client side. It is not necessary to check here. Within the function, GonkNativeWindow::freeAllBuffersLocked() is called. The function frees all buffers in GonkNativeWindow and increment buffer generation.


//<snip>
-    // Error out if the user has dequeued buffers or sent buffers to
-    // video stream
+    // Error out if the user has dequeued buffers
     for (int i=0 ; i<mBufferCount ; i++) {
-        if (mSlots[i].mBufferState == BufferSlot::DEQUEUED ||
-            mSlots[i].mBufferState == BufferSlot::RENDERING) {
+        if (mSlots[i].mBufferState == BufferSlot::DEQUEUED) {
             CNW_LOGE("setBufferCount: client owns some buffers");
             return -EINVAL;
         }
     }
Attachment #676037 - Attachment is obsolete: true
Posted file GonkNativeWindow.cpp (obsolete) —
update based on  attachment 676458 [details] [diff] [review]
Attachment #676038 - Attachment is obsolete: true
just update
Attachment #675064 - Attachment is obsolete: true
just update
Attachment #675074 - Attachment is obsolete: true
current gaia's camera app set 'cif' profile for recording. My phone do not support 'cif', then I change it to 'high'.

https://github.com/mozilla-b2g/gaia/blob/master/apps/camera/js/camera.js#L74

//<snip>

  _previewConfigVideo: {
    profile: 'cif',
    rotation: 90,
    width: 352,
    height: 288
  },
By applying all patches except part 2 attachment 676460 [details] [diff] [review], I confirmed following on my phone.
 - video recording
 - camera preview
 - camera(front/back) change during previewing
 - take photos
 - camera mode(picture/video) change
 - video playback by using hw codec

But when applying "Part 2 rev 2 - Disable app process's high privilege", video recording failes. It failed in video file creation in nsGonkCameraControl::StartRecordingImpl().

http://mxr.mozilla.org/mozilla-central/source/dom/camera/GonkCameraControl.cpp#737

It try to create the file by using posix open() function directly, then failed. It is a file system permission problem, not hw permissions problem. Better to be addressed in a different bug.
Hi Kan-Ru Chen, can I have a comment to this bug?
IMHO, it is good for FirefoxOS(gonk).

thanks
(In reply to Sotaro Ikeda [:sotaro] from comment #30)
> Hi Kan-Ru Chen, can I have a comment to this bug?
> IMHO, it is good for FirefoxOS(gonk).
> 
> thanks

I think moving Camera/OMXDecoder to a dedicated process is the right direction, however I'm not sure whether we want to reuse mediaserver or not. IIRC we only use mediaserver for audio output and we might kill it after all the dependencies are removed. You might also want mikeh to take a look at the camera changes.
Thank for the comment. I am also going to ask mikeh.

There are two reasons to have a dedicated process for media hardwares.
 - To separate buggy hardwares from b2g chrome process and b2g app process.
 - Not to request high priviledge to app process that handle media hardwares

From my experience on mobile, audio hw is also buggy, it should also be separated from b2g process.

It is ideal to construct the process on gecko framework. But it is very very difficult to do it right now. It is a future task. On the contrary, we can use android framework, and it is the only effective choice to do it right now, I think. If we use android framework, we should use the mediaserver for it. It's source code is stable and common. And we could share some sources between Fennec for android around them.
High priviledge of app process handling media hw is a big problem.

- normal web contents could not use the hardwares capability directly.
- an application needs to have "deprecated-hwvideo" or "camera" permission in manifest.webapp to use them.
- could not apply the privilege to an application durinng its running. It needs the process restart.
- camara hw might be loaded from multiple processes. In android, camera hw is loaded from only one process(mediaserver).
- application processes with deprecated-hwvideo/camera permission have too high privilege
Hi mikeh, can I have a comment about this bug?

thanks
In near future, FirefoxOS is going to implement WebRTC, I think. On WebRTC, normal web content need to use camera hw and hw codec capabilities if user grant it. The app process priviledge problem need be solved also for the WebRTC.
Depends on: 808907
re-base
Attachment #675059 - Attachment is obsolete: true
Attachment #676460 - Attachment is obsolete: true
re-base
Attachment #675066 - Attachment is obsolete: true
re-base
Attachment #675067 - Attachment is obsolete: true
re-base and some small updates
Attachment #676458 - Attachment is obsolete: true
Attachment #676461 - Attachment is obsolete: true
I confirmed that the patches works on unagi phone.
Attachment #703403 - Attachment is patch: true
attachment 703403 [details] [diff] [review] do not apply Bug 830995. It is necessary to apply the bug.
(In reply to Sotaro Ikeda [:sotaro] from comment #41)
> attachment 703403 [details] [diff] [review] do not apply Bug 830995. It is
> necessary to apply the bug.

also need to care about Bug 825687.
The patches do not work any more. It is necessary to update them. I locally updated the patch. But after applying them camera do not work correctly. After fixed the camera problem, I am going to update the patches.
re-base
Attachment #703398 - Attachment is obsolete: true
re-base
Attachment #703399 - Attachment is obsolete: true
re-base
Attachment #703401 - Attachment is obsolete: true
- apply Bug 830995 change
- apply Bug 835367 attachment 707604 [details] [diff] [review]
- fix as recording to work again
- fix nits
Attachment #703403 - Attachment is obsolete: true
Posted file GonkNativeWindow.h (obsolete) —
as reference
Attachment #676039 - Attachment is obsolete: true
Posted file GonkNativeWindow.cpp (obsolete) —
as reference
Attachment #676459 - Attachment is obsolete: true
I confirmed the patches works on unagi phone.
Assignee: nobody → sotaro.ikeda.g
Blocks: 793034
Sotaro, can you

 1. Generate a single rollup patch that we can use to easily test these changes

 2. Start getting these changes through review :).  I'm not sure who the right reviewers are, but they should all be CC'd now.
some patches become bitrotted. I am going to update them. Then create the rollup patch.
unbitrot
Attachment #708286 - Attachment is obsolete: true
unbitrot
Attachment #708288 - Attachment is obsolete: true
Posted patch rollup patch (obsolete) — Splinter Review
just to test the patches easily
The patches are made for b2g18, not for mc/mi.

(a73813aea20d0ad83569212cf98a4204e9e02df7)
The bug need to apply Bug 836782.
Depends on: 836782
I am going to split "Part 5" to get reviews easier.
Duplicate of this bug: 808907
Duplicate of this bug: 798773
Duplicate of this bug: 798202
No longer depends on: 740997, 798202, 798773, 808907
We critically need to fix this from the product perspective.  The fix for this bug is going to affect stability testing, so we need to get it in the review+land pipeline asap.
blocking-b2g: --- → leo?
Whiteboard: regression-risk
I update the patch soon.
split "Part 1 rev3" to get a review easily
Attachment #708285 - Attachment is obsolete: true
split "Part 1 rev3" to get a review easily
un-bitrot
Attachment #711040 - Attachment is obsolete: true
split "Part 5" to get a review
Attachment #675081 - Attachment is obsolete: true
Attachment #675083 - Attachment is obsolete: true
Attachment #708290 - Attachment is obsolete: true
Attachment #708291 - Attachment is obsolete: true
split "Part 5" to get a review
split "Part 5" to get a review
split "Part 5" to get a review
split "Part 5" to get a review
split "Part 5" to get a review
Attachment #711041 - Attachment is obsolete: true
Posted patch rollup patch rev2 (obsolete) — Splinter Review
update rollup patch
Attachment #711042 - Attachment is obsolete: true
Attachment #717581 - Flags: review?(jones.chris.g)
Attachment #717582 - Flags: review?(jones.chris.g)
Attachment #717583 - Flags: review?(chris.double)
Attachment #708287 - Flags: review?(mhabicher)
Attachment #717588 - Flags: review?(kchen)
Attachment #717589 - Flags: review?(chris.double)
Attachment #717590 - Flags: review?(mhabicher)
Attachment #717591 - Flags: review?(mhabicher)
Attachment #717592 - Flags: review?(mhabicher)
Attachment #717593 - Flags: review?(jones.chris.g)
Comment on attachment 717588 [details] [diff] [review]
Part 5a rev1 - Split GonkNativeWindow to GonkNativeWindow and GonkNativeWindowClient

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

::: dom/camera/GonkNativeWindow.cpp
@@ +193,5 @@
> +{
> +    CNW_LOGD("requestBuffer: slot=%d", slot);
> +    Mutex::Autolock lock(mMutex);
> +    if (mAbandoned) {
> +        CNW_LOGE("requestBuffer: SurfaceTexture has been abandoned!");

This is not SurfaceTexture anymore :)

@@ +255,5 @@
>                       * the consumer may still have pending reads of the
>                       * buffers in flight.
>                       */
> +                    bool isOlder = mSlots[i].mFrameNumber < mSlots[found].mFrameNumber;
> +                    if (found < 0 || isOlder) {

We need to check if (found < 0) first. This was fixed earlier.

@@ +310,5 @@
> +            format = mPixelFormat;
> +        }
> +
> +        // buffer is now in DEQUEUED (but can also be current at the same time,
> +        // if we're in synchronous mode)

Does this comment applies to our impl?
(In reply to Kan-Ru Chen [:kanru] from comment #74)
> Comment on attachment 717588 [details] [diff] [review]
> Part 5a rev1 - Split GonkNativeWindow to GonkNativeWindow and
> GonkNativeWindowClient
> 
> Review of attachment 717588 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/camera/GonkNativeWindow.cpp
> @@ +193,5 @@
> > +{
> > +    CNW_LOGD("requestBuffer: slot=%d", slot);
> > +    Mutex::Autolock lock(mMutex);
> > +    if (mAbandoned) {
> > +        CNW_LOGE("requestBuffer: SurfaceTexture has been abandoned!");
> 
> This is not SurfaceTexture anymore :)

Fixed

> 
> @@ +255,5 @@
> >                       * the consumer may still have pending reads of the
> >                       * buffers in flight.
> >                       */
> > +                    bool isOlder = mSlots[i].mFrameNumber < mSlots[found].mFrameNumber;
> > +                    if (found < 0 || isOlder) {
> 
> We need to check if (found < 0) first. This was fixed earlier.

Fixed

> 
> @@ +310,5 @@
> > +            format = mPixelFormat;
> > +        }
> > +
> > +        // buffer is now in DEQUEUED (but can also be current at the same time,
> > +        // if we're in synchronous mode)
> 
> Does this comment applies to our impl?

Do not apply to our impl. Remove the comment.
Apply comments.
Attachment #717588 - Attachment is obsolete: true
Attachment #717588 - Flags: review?(kchen)
Attachment #717886 - Flags: review?(kchen)
Attachment #717581 - Flags: review?(jones.chris.g) → review?(mwu)
Attachment #717582 - Flags: review?(jones.chris.g) → review+
Attachment #717593 - Flags: review?(jones.chris.g) → review?(mwu)
Comment on attachment 708287 [details] [diff] [review]
Part 4 rev3 - Use OmxClient in GonkRecorder

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

r+ with the noted items addressed.

::: dom/camera/GonkRecorder.cpp
@@ -51,5 @@
> -static sp<IOMX> sOMX = NULL;
> -static sp<IOMX> GetOMX() {
> -  if(sOMX.get() == NULL) {
> -    sOMX = new OMX;
> -    }

Formatting:
  if (sOMX.get() == NULL) {
    sOMX = new OMX;
  }

@@ +757,5 @@
>          encMeta->setInt32(kKeyTimeScale, mAudioTimeScale);
>      }
>  
> +    OMXClient client;
> +    CHECK_EQ(client.connect(), OK);

Can you add a comment here indicating that CHECK_EQ() in fact asserts/causes an abort if the two values are not equal?
Attachment #708287 - Flags: review?(mhabicher) → review+
(In reply to Mike Habicher [:mikeh] from comment #77)
> Comment on attachment 708287 [details] [diff] [review]
> Part 4 rev3 - Use OmxClient in GonkRecorder
> 
> Review of attachment 708287 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the noted items addressed.
> 
> ::: dom/camera/GonkRecorder.cpp
> @@ -51,5 @@
> > -static sp<IOMX> sOMX = NULL;
> > -static sp<IOMX> GetOMX() {
> > -  if(sOMX.get() == NULL) {
> > -    sOMX = new OMX;
> > -    }
> 
> Formatting:
>   if (sOMX.get() == NULL) {
>     sOMX = new OMX;
>   }

The code is removed by applying the patch.

> 
> @@ +757,5 @@
> >          encMeta->setInt32(kKeyTimeScale, mAudioTimeScale);
> >      }
> >  
> > +    OMXClient client;
> > +    CHECK_EQ(client.connect(), OK);
> 
> Can you add a comment here indicating that CHECK_EQ() in fact asserts/causes
> an abort if the two values are not equal?

Will add a comment in a next patch.
Comment on attachment 717591 [details] [diff] [review]
Part 5d rev1 - Cange GonkRecorder to use GonkCameraHardware

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

r+ with suggested changes.

::: dom/camera/GonkRecorder.cpp
@@ +54,5 @@
>        mVideoSource(VIDEO_SOURCE_LIST_END),
>        mStarted(false),
>        mDisableAudio(false) {
>  
> +    DOM_CAMERA_LOGI("Constructor");

Please use the format standardized elsewhere in the camera code:
DOM_CAMERA_LOGT("%s:%d : this=%p\n", __function__, __LINE__, this);

This including 'this' makes it easier to track down potential object leaks.

@@ +59,5 @@
>      reset();
>  }
>  
>  GonkRecorder::~GonkRecorder() {
> +    DOM_CAMERA_LOGI("Destructor");

As with the constructor.

@@ +64,5 @@
>      stop();
>  }
>  
>  status_t GonkRecorder::init() {
> +    DOM_CAMERA_LOGI("init");

Consider using _LOGT(); also please include either file or function context, so that we don't have to guess what a bare "init" line in logcat means.

@@ +69,5 @@
>      return OK;
>  }
>  
>  status_t GonkRecorder::setAudioSource(audio_source_t as) {
> +    DOM_CAMERA_LOGI("setAudioSource: %d", as);

"GonkRecorder::setAudioSource"

@@ +90,5 @@
>      return OK;
>  }
>  
>  status_t GonkRecorder::setVideoSource(video_source vs) {
> +    DOM_CAMERA_LOGI("setVideoSource: %d", vs);

"GonkRecorder::setVideoSource"

@@ +107,5 @@
>      return OK;
>  }
>  
>  status_t GonkRecorder::setOutputFormat(output_format of) {
> +    DOM_CAMERA_LOGI("setOutputFormat: %d", of);

As above, and please apply the same change to all of the functions below as well.

@@ +203,3 @@
>      // These don't make any sense, do they?
>      CHECK_EQ(offset, 0);
>      CHECK_EQ(length, 0);

Does it make sense to abort here? or would it be better to return an error code?

@@ +369,5 @@
>          return BAD_VALUE;
>      }
>  
>      if (bytes <= 100 * 1024) {
> +        DOM_CAMERA_LOGW("Target file size (%lld bytes) is too small to be respected", bytes);

Is the only handling of this message the logging of a warning?  Who will not be respecting this "too small" value?

@@ +779,5 @@
>              return BAD_VALUE;
>          }
>      } else {  // mOutputFormat must be OUTPUT_FORMAT_AMR_WB
>          if (mAudioEncoder != AUDIO_ENCODER_AMR_WB) {
> +            DOM_CAMERA_LOGE("Invlaid encoder %d used for AMRWB recording",

"Invlaid" --> "Invalid"

@@ -1590,5 @@
>      snprintf(buffer, SIZE, "     Source: %d\n", mVideoSource);
>      result.append(buffer);
>      snprintf(buffer, SIZE, "     Camera Id: %d\n", mCameraId);
>      result.append(buffer);
> -    snprintf(buffer, SIZE, "     Camera Handle: %d\n", mCameraHandle);

Log the camera object address instead?

::: dom/camera/GonkRecorder.h
@@ +27,1 @@
>  

Take out this blank line, please.
Attachment #717591 - Flags: review?(mhabicher) → review+
Attachment #717592 - Flags: review?(mhabicher) → review+
Comment on attachment 717590 [details] [diff] [review]
Part 5c rev1 - Cange GonkCamera to use android::Camera

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

I'm holding off on the r+ until we sort out ::Connect()'s return value lifetime.

::: dom/camera/GonkCameraControl.cpp
@@ +279,5 @@
>    DOM_CAMERA_LOGI(" - exposure compensation step:    %f\n", mExposureCompensationStep);
>    DOM_CAMERA_LOGI(" - maximum metering areas:        %d\n", mMaxMeteringAreas);
>    DOM_CAMERA_LOGI(" - maximum focus areas:           %d\n", mMaxFocusAreas);
>  
> +  return mCameraHw != 0 ? NS_OK : NS_ERROR_FAILURE;

mCameraHw != nullptr

::: dom/camera/GonkCameraHwMgr.cpp
@@ +75,5 @@
>  }
>  
>  // Android data callback
>  void
> +GonkCameraHardware::postData(int32_t aMsgType, const sp<IMemory>& aDataPtr, camera_frame_metadata_t *metadata)

"camera_frame_metadata_t *metadata" --> "camera_frame_metadata_t* metadata"

@@ +183,5 @@
> +sp<GonkCameraHardware>
> +GonkCameraHardware::Connect(mozilla::nsGonkCameraControl* aTarget, uint32_t aCameraId)
> +{
> +  sp<Camera> camera = Camera::connect(aCameraId);
> +  if (camera == 0) {

camera == nullptr

@@ +185,5 @@
> +{
> +  sp<Camera> camera = Camera::connect(aCameraId);
> +  if (camera == 0) {
> +    return nullptr;
> +   }

Align closing brace with 'if'

@@ +187,5 @@
> +  if (camera == 0) {
> +    return nullptr;
> +   }
> +  sp<GonkCameraHardware> cameraHardware = new GonkCameraHardware(aTarget, aCameraId, camera);
> +  return cameraHardware;

Is this going to work?  I think you need some extra syntatic sugar here to make sure that cameraHardware isn't destroyed when Connect()'s stack unwinds but before the return value is stuffed into the destination sp<>.

@@ +189,5 @@
> +   }
> +  sp<GonkCameraHardware> cameraHardware = new GonkCameraHardware(aTarget, aCameraId, camera);
> +  return cameraHardware;
> + }
> + 

Extra whitespace at end of line.

@@ +204,4 @@
>    }
> +  mCamera.clear();
> +  mNativeWindow.clear();
> + 

Extra whitespace at end of line.

@@ +267,2 @@
>  {
> +  //android::Camera do not provide this capability

Interesting.  I'm surprised that the lower layers provide this functionality, but the AOSP layers don't.

Please add a _LOGW() message here so it's obvious in logcat that this is a stub.

@@ +284,5 @@
>  
>  int
>  GonkCameraHardware::StartPreview()
>  {
> +  DOM_CAMERA_LOGI("%s\n", __func__);

DOM_CAMERA_LOGT("%s:%d : this=%p\n", __function__, __LINE__, this);

@@ +291,5 @@
>  
> +void
> +GonkCameraHardware::StopPreview()
> +{
> +  DOM_CAMERA_LOGI("%s\n", __func__);

DOM_CAMERA_LOGT("%s:%d : this=%p\n", __function__, __LINE__, this);

@@ +301,2 @@
>  {
> +  DOM_CAMERA_LOGI("%s\n", __func__);

DOM_CAMERA_LOGT("%s:%d : this=%p\n", __function__, __LINE__, this);

@@ +314,2 @@
>  {
> +  DOM_CAMERA_LOGI("%s\n", __func__);

DOM_CAMERA_LOGT("%s:%d : this=%p\n", __function__, __LINE__, this);

::: dom/camera/GonkCameraHwMgr.h
@@ +54,2 @@
>  
> +  //drived from GonkNativeWindowNewFrameCallback

// derived ...

@@ +56,5 @@
>    virtual void    OnNewFrame() MOZ_OVERRIDE;
>  
> +  //derived from CameraListener
> +  virtual void notify(int32_t aMsgType, int32_t ext1, int32_t ext2);
> +  virtual void postData(int32_t aMsgType, const sp<IMemory> &aDataPtr, camera_frame_metadata_t *metadata);

"const sp<IMemory> &aDataPtr" --> "const sp<IMemory>& aDataPtr"
"camera_frame_metadata_t *metadata" --> "camera_frame_metadata_t* metadata"
Apply a comment. Carry "mhabicher: review+"
Attachment #708287 - Attachment is obsolete: true
Attachment #717971 - Flags: review+
Apply the comment. Carry "mhabicher: review+".
Attachment #717591 - Attachment is obsolete: true
Attachment #718063 - Flags: review+
Comment on attachment 717583 [details] [diff] [review]
Part 2 rev5 - Use OmxClient in OmxDecoder

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

::: content/media/omx/OmxDecoder.cpp
@@ +233,5 @@
> +  OMXClient client;
> +  status_t err = client.connect();
> +  NS_ASSERTION(err == OK, "Failed to connect to OMX in mediaserver.");
> +  sp<IOMX> omx = client.interface();
> +

Is a matching 'disconnect' somewhere needed? In media/omx-plugin/OmxPlugin.cpp we disconnect in the destructor.
Attachment #717583 - Flags: review?(chris.double) → review+
Attachment #717589 - Flags: review?(chris.double) → review+
(In reply to Mike Habicher [:mikeh] from comment #79)
> Comment on attachment 717591 [details] [diff] [review]
> Part 5d rev1 - Cange GonkRecorder to use GonkCameraHardware
> 
> Review of attachment 717591 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +203,3 @@
> >      // These don't make any sense, do they?
> >      CHECK_EQ(offset, 0);
> >      CHECK_EQ(length, 0);
> 
> Does it make sense to abort here? or would it be better to return an error
> code?

it make sense to abort here. In current use cases(mp4/3gpp), it is necessary to start recording from a top of the file.

> @@ +369,5 @@
> >          return BAD_VALUE;
> >      }
> >  
> >      if (bytes <= 100 * 1024) {
> > +        DOM_CAMERA_LOGW("Target file size (%lld bytes) is too small to be respected", bytes);
> 
> Is the only handling of this message the logging of a warning?  Who will not
> be respecting this "too small" value?

Yes, I think so. 100K byte is always very small as MaxFileSizeByte of media files. It do not happen in normal use cases.
(In reply to Chris Double (:doublec) from comment #83)
> Comment on attachment 717583 [details] [diff] [review]
> Part 2 rev5 - Use OmxClient in OmxDecoder
> 
> Review of attachment 717583 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/omx/OmxDecoder.cpp
> @@ +233,5 @@
> > +  OMXClient client;
> > +  status_t err = client.connect();
> > +  NS_ASSERTION(err == OK, "Failed to connect to OMX in mediaserver.");
> > +  sp<IOMX> omx = client.interface();
> > +
> 
> Is a matching 'disconnect' somewhere needed? In
> media/omx-plugin/OmxPlugin.cpp we disconnect in the destructor.

It is not necessary to call 'disconnect' in this case. OMXClient is instantiated on the stack. And a reference pointer to IOMX is automatically freed when the function exit.
Apply the comment.
Attachment #717590 - Attachment is obsolete: true
Attachment #717590 - Flags: review?(mhabicher)
(In reply to Mike Habicher [:mikeh] from comment #80)
> Comment on attachment 717590 [details] [diff] [review]
> Part 5c rev1 - Cange GonkCamera to use android::Camera
> 
> Review of attachment 717590 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm holding off on the r+ until we sort out ::Connect()'s return value
> lifetime.
//snip
> 
> @@ +187,5 @@
> > +  if (camera == 0) {
> > +    return nullptr;
> > +   }
> > +  sp<GonkCameraHardware> cameraHardware = new GonkCameraHardware(aTarget, aCameraId, camera);
> > +  return cameraHardware;
> 
> Is this going to work?  I think you need some extra syntatic sugar here to
> make sure that cameraHardware isn't destroyed when Connect()'s stack unwinds
> but before the return value is stuffed into the destination sp<>.

It works. It is a very common pattern in android like following.
http://androidxref.com/4.0.4/xref/frameworks/base/media/libstagefright/AACExtractor.cpp#188

The function's return type "sp<GonkCameraHardware>" is the point. While return from the function, the reference count gets increased by copy constructor & operator = .

I found same question in the URL.
https://groups.google.com/forum/?fromgroups#!topic/android-framework/92ucp0Rpq8A
Attachment #718136 - Flags: review?(mhabicher)
Per comment #62, blocking 1.1.
blocking-b2g: leo? → leo+
Comment on attachment 717886 [details] [diff] [review]
Part 5a rev2 - Split GonkNativeWindow to GonkNativeWindow and GonkNativeWindowClient

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

Sorry for the second round. r=me with comments addressed.

::: dom/camera/GonkNativeWindow.cpp
@@ +34,5 @@
>  
>  using namespace android;
>  using namespace mozilla::layers;
>  
> +GonkNativeWindow::GonkNativeWindow() : 

Trailing white space

@@ +313,5 @@
>          mSlots[buf].mBufferState = BufferSlot::DEQUEUED;
>  
>          const sp<GraphicBuffer>& gbuf(mSlots[buf].mGraphicBuffer);
>          alloc = (gbuf == NULL);
> +        if ((gbuf!=NULL) && 

Trailing white space

@@ +370,5 @@
>                  mSlots[buf].mGraphicBuffer = graphicBuffer;
>                  mSlots[buf].mSurfaceDescriptor = desc;
>                  mSlots[buf].mSurfaceDescriptor.get_SurfaceDescriptorGralloc().external() = true;
> +                mSlots[buf].mRequestBufferCalled = false;
> +            

Extra white spaces at the beginning of line.

@@ +496,2 @@
>    }
> +  

Extra white spaces at the beginning of line.

::: dom/camera/GonkNativeWindow.h
@@ +73,5 @@
> +    // calling this all buffer slots are owned by the GonkNativeWindow object
> +    // (i.e. they are not owned by the client).
> +    virtual status_t setBufferCount(int bufferCount);
> +
> +    virtual status_t requestBuffer(int slot, sp<GraphicBuffer>* buf);

Add some comments.
Attachment #717886 - Flags: review?(kchen) → review+
Needed to make important user-visible content work.
Whiteboard: regression-risk → regression-risk [tech-bug]
Apply the comment. Carry "kchen: review+".
Attachment #717886 - Attachment is obsolete: true
Attachment #718424 - Flags: review+
Carry "kchen: review+".
update just to to fix the patch that I have forgotton to add GonkNativeWindowClient.
Attachment #718424 - Attachment is obsolete: true
Attachment #718466 - Flags: review+
add "CAMERA_MSG_SHUTTER" to the argument of  mCamera->takePicture(). Without it, GonkCameraHardware can not receive CAMERA_MSG_SHUTTER message.

Change part is following.

> +  return mCamera->takePicture(CAMERA_MSG_SHUTTER | CAMERA_MSG_COMPRESSED_IMAGE);
Attachment #718136 - Attachment is obsolete: true
Attachment #718136 - Flags: review?(mhabicher)
Attachment #718469 - Flags: review?(mhabicher)
Comment on attachment 718136 [details] [diff] [review]
Part 5c rev2 - Cange GonkCamera to use android::Camera

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

Just a couple of nits.  r=me with the changes below.

::: dom/camera/GonkCameraHwMgr.cpp
@@ +267,2 @@
>  {
> +  DOM_CAMERA_LOGW("android::Camera do not provide this capability\n");

Please explicitly mention 'CancelTakePicture' in the warning so that it's possible to identify the source of this warning in the logcat.

::: dom/camera/GonkCameraHwMgr.h
@@ +54,2 @@
>  
> +  // drived from GonkNativeWindowNewFrameCallback

Typo: "drived" --> "derived"
                     ^
Attachment #718136 - Attachment is obsolete: false
Apply the comment. Carry "mhabicher: review+".
Attachment #718136 - Attachment is obsolete: true
Attachment #718469 - Attachment is obsolete: true
Attachment #718469 - Flags: review?(mhabicher)
Attachment #718481 - Flags: review+
Posted patch rollup patch rev3 (obsolete) — Splinter Review
update the patch
Attachment #717595 - Attachment is obsolete: true
mwu, do you have a time to review?
Attachment #717593 - Flags: review?(mwu) → review+
Comment on attachment 717581 [details] [diff] [review]
Part 1a rev1 - Enable Binder ThreadPool

I don't like where we're putting these calls, but there doesn't seem to be any other options that are significantly better.

I just have a suggestion for the comment:

This creates a ThreadPool for binder ipc. A ThreadPool is necessary to receive binder calls, though not necessary to send binder calls. ProcessState::Self() also needs to be called once on the main thread to register the main thread with the binder driver.
Attachment #717581 - Flags: review?(mwu) → review+
A patch for commit.
Apply the comment. Carry "mwu: review+".
Attachment #717581 - Attachment is obsolete: true
Attachment #722268 - Flags: review+
A patch for commit. Carry "cjones: review+".
Attachment #722272 - Flags: review+
Attachment #717582 - Attachment is obsolete: true
A patch for commit. Carry "chris.double: review+".
Attachment #717583 - Attachment is obsolete: true
Attachment #722274 - Flags: review+
Comment on attachment 722268 [details] [diff] [review]
Part 1a rev2 - Enable Binder ThreadPool

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

::: ipc/app/MozillaRuntimeMain.cpp
@@ +33,5 @@
> +  // This creates a ThreadPool for binder ipc. A ThreadPool is necessary to
> +  // receive binder calls, though not necessary to send binder calls.
> +  // ProcessState::Self() also needs to be called once on the main thread to
> +  // register the main thread with the binder driver.
> +  android::ProcessState::self()->startThreadPool();

One thing I forgot to mention - the indentation here is wrong for this file.
A patch for commit. Carry "mhabicher: review+".
Attachment #717971 - Attachment is obsolete: true
Attachment #722275 - Flags: review+
A patch for commit. Carry "kchen: review+".
Attachment #718466 - Attachment is obsolete: true
Attachment #722277 - Flags: review+
A patch for commit. Carry "chris.double: review+".
Attachment #717589 - Attachment is obsolete: true
Attachment #722280 - Flags: review+
A patch for commit. Carry "mhabicher: review+".
Attachment #718481 - Attachment is obsolete: true
Attachment #722283 - Flags: review+
A patch for commit. Carry "mhabicher: review+".
Attachment #718063 - Attachment is obsolete: true
Attachment #722286 - Flags: review+
A patch for commit. Carry "mhabicher: review+".
Attachment #717592 - Attachment is obsolete: true
Attachment #722290 - Flags: review+
A patch for commit. Carry "mwu: review+".
Attachment #717593 - Attachment is obsolete: true
Attachment #722292 - Flags: review+
Attachment #718584 - Attachment is obsolete: true
(In reply to Sotaro Ikeda [:sotaro] from comment #110)
> https://tbpl.mozilla.org/?tree=Try&rev=b77a34a4f40c

try busted. need to address.
A patch for commit.
Fix indent. Carry "mwu: review+".
Attachment #722268 - Attachment is obsolete: true
Attachment #722443 - Flags: review+
Fix bustage in linux build. Carry "cjones: review+".
Attachment #722272 - Attachment is obsolete: true
Attachment #722446 - Flags: review+
Fix bustage in B2G Arm debug build. Carry "mhabicher: review+".
Attachment #722283 - Attachment is obsolete: true
Attachment #722448 - Flags: review+
update nit. Carry "cjones: review+".
Attachment #722446 - Attachment is obsolete: true
Attachment #722457 - Flags: review+
Attachment #722448 - Attachment description: Part 5c rev6 - Cange GonkCamera to use android::Camera → Part 5c rev6 - Change GonkCamera to use android::Camera
Attachment #722286 - Attachment description: Part 5d rev3 - Cange GonkRecorder to use GonkCameraHardware → Part 5d rev3 - Change GonkRecorder to use GonkCameraHardware
(In reply to Sotaro Ikeda [:sotaro] from comment #116)
> https://tbpl.mozilla.org/?tree=Try&rev=567fe2b65270

There is one orange in Fedora debug. It is not related to this bug. Because the changes are enable only on gonk.
Keywords: checkin-needed
Blocks: 849330
Blocks: 831747
By applying patches, I confirmed that browser can play H.264 video on unagi phone by using hw codec. The following url have same content by H.264 and webm. H.254 video playback is smooth than webm by using hw codec.

http://www.bluishcoder.co.nz/2012/06/02/h264-aac-mp3-support-for-b2g.html
(In reply to Sotaro Ikeda [:sotaro] from comment #119)
> By applying patches, I confirmed that browser can play H.264 video on unagi
> phone by using hw codec. The following url have same content by H.264 and
> webm. H.254 video playback is smooth than webm by using hw codec.
> 
> http://www.bluishcoder.co.nz/2012/06/02/h264-aac-mp3-support-for-b2g.html

By unpriviled app can play H.264 video, the app continue to grab hw codec. It is going to be addressed in Bug 831747.

And the url's playback stopped soon. It seems another problem. There might be a problem around caching, it is going to be handled in a new bug.
(In reply to Sotaro Ikeda [:sotaro] from comment #120)
> (In reply to Sotaro Ikeda [:sotaro] from comment #119)
>
> And the url's playback stopped soon. It seems another problem. There might
> be a problem around caching, it is going to be handled in a new bug.

Sometimes the H.264 video is playback until end, sometimes not. It seems that it relats networking and caching.
Following is better to play H.264 videos in web browser in FirefoxOS.
http://people.mozilla.org/~cpeterson/videos/
No longer blocks: 831747
Depends on: 831747
I confirmed the bug is fixed on b2g18 by using unagi.
Blocks: 839007
Depends on: 851809
Duplicate of this bug: 850209
Depends on: 853977
Flags: in-moztrap?
Naoki said he was going to take this for creating the moztrap test cases here, so I'm flagging him here.
Flags: in-moztrap? → in-moztrap?(nhirata.bugzilla)
Comment on attachment 722443 [details] [diff] [review]
Part 1a rev3 - Enable Binder ThreadPool

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 868932 - All channel volume is not updated. 
User impact if declined: the volume can't be adjust after media server crashed.
Testing completed: kill media server and can't reproduce Bug 868932
Risk to taking this patch (and alternatives if risky): N/A
String or UUID changes made by this patch:
Attachment #722443 - Flags: approval-mozilla-b2g18?
Comment on attachment 722457 [details] [diff] [review]
Part 1b rev4 - change Makefile.in for Enable Binder ThreadPool

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 868932 - All channel volume is not updated. 
User impact if declined: the volume can't be adjusted after media server crashed.
Testing completed: kill media server and can't reproduce Bug 868932
Risk to taking this patch (and alternatives if risky): N/A
String or UUID changes made by this patch:
Attachment #722457 - Flags: approval-mozilla-b2g18?
There is no b2g18-v1.0.1 flag, so I use the b2g18 and uplift to v1.0.1.
(In reply to Randy Lin [:rlin] from comment #128)
> Is possible to phase-in those two patch
> https://bugzilla.mozilla.org/attachment.cgi?id=722443
> https://bugzilla.mozilla.org/attachment.cgi?id=722457
> because it depends
> https://bugzilla.mozilla.org/show_bug.cgi?id=868932

No problem to apply patches to v1.0.1.
Flags: needinfo?(sotaro.ikeda.g)
Can we file a new tef+ clone of bug 868932 rather than reusing this one for these two patches? a=akeybl for the landings, but I want to make sure that the bug trail makes sense.
Flags: needinfo?(rlin)
Hi Alex, 
I already had another one for this purpose. 
https://bugzilla.mozilla.org/show_bug.cgi?id=873350
Do I need to copy those two patches on that bug?
Flags: needinfo?(rlin)
Attachment #722443 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Attachment #722457 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Flags: needinfo?(jhammink)
Flags: in-moztrap?(nhirata.bugzilla)
Removing NI as there is now a testcase covering it.
Flags: needinfo?(jhammink)
You need to log in before you can comment on or make changes to this bug.