803471 - Use hw codec and camera hw in mediaserver process

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Sotaro Ikeda [:sotaro]]

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.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: change to use CameraService in mediaserver to access camera

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

Comment 2

[Sotaro Ikeda [:sotaro]]

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.

Comment 3

[Sotaro Ikeda [:sotaro]]

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.

Comment 4

[Sotaro Ikeda [:sotaro]]

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.

Comment 5

[Sotaro Ikeda [:sotaro]]

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

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: Part 1 - Enable Binder ThreadPool

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.

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: Part 2 - Disable app process's high privilege

Disable to apply high privilege to app process that has "camera" or "deprecated-hwvideo" permission.

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: Part 3 - Use OmxClient in OmxDecoder

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.

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: Part 4 - Use OmxClient in GonkRecorder

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.

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5 - Use android::Camera to control camera hw

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

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: Part 6 - Implement NATIVE_WINDOW_SET_BUFFERS_SIZE for some qcom ics 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.

Comment 12

[Sotaro Ikeda [:sotaro]]

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.

Comment 13

[Sotaro Ikeda [:sotaro]]

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.

Comment 14

[Sotaro Ikeda [:sotaro]]

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

Comment 15

[Sotaro Ikeda [:sotaro]]

Attachment: GonkNativeWindow.cpp

supplement to attachment 675071 [details] [diff] [review]
It is not easy to understand the code only from it.

Comment 16

[Sotaro Ikeda [:sotaro]]

Attachment: GonkNativeWindow.h

supplement to attachment 675071 [details] [diff] [review]
It is not easy to understand the code only from it.

Comment 17

[Sotaro Ikeda [:sotaro]]

Attachment: GonkNativeWindowClient.cpp

supplement to attachment 675071 [details] [diff] [review]
It is not easy to understand the code only from it.

Comment 18

[Sotaro Ikeda [:sotaro]]

Attachment: GonkNativeWindowClient.h

supplement to attachment 675071 [details] [diff] [review]
It is not easy to understand the code only from it.

Comment 19

[Sotaro Ikeda [:sotaro]]

Video mode did not work at all on yesterday's source, because of bug 805377,  I assume.

Comment 20

[Sotaro Ikeda [:sotaro]]

Created attachment 675074 [details] [diff] [review] is created based on following code. otoro and unagi device uses ics_chocolate_rb4.2. Therefore it is not necessary. It is for ics_strawberry_rb5.3.

https://www.codeaurora.org/gitweb/quic/la/?p=platform/frameworks/base.git;a=blob;f=libs/gui/SurfaceTexture.cpp;h=e2333fd8257e21ae07e8cc9471980c0b476fef7b;hb=ics_strawberry_rb5.3#l769

https://www.codeaurora.org/gitweb/quic/la/?p=platform/frameworks/base.git;a=blob;f=libs/gui/SurfaceTextureClient.cpp;h=edd3038e6f687f5193e1e3ba2eb1130cd28639f7;hb=ics_strawberry_rb5.3#l432

Comment 21

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5 rev2 - Use android::Camera to control camera hw

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.

Comment 22

[Sotaro Ikeda [:sotaro]]

Attachment: GonkNativeWindow.cpp

update besed on attachment 676037 [details] [diff] [review]

Comment 23

[Sotaro Ikeda [:sotaro]]

Attachment: GonkNativeWindow.h

update besed on attachment 676037 [details] [diff] [review]

Comment 24

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5 rev3 - Use android::Camera to control camera hw

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

Comment 25

[Sotaro Ikeda [:sotaro]]

Attachment: GonkNativeWindow.cpp

update based on  attachment 676458 [details] [diff] [review]

Comment 26

[Sotaro Ikeda [:sotaro]]

Attachment: Part 2 rev 2 - Disable app process's high privilege

just update

Comment 27

[Sotaro Ikeda [:sotaro]]

Attachment: Part 6 rev2 - Implement NATIVE_WINDOW_SET_BUFFERS_SIZE for some qcom ics hardwares

just update

Comment 28

[Sotaro Ikeda [:sotaro]]

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

Comment 29

[Sotaro Ikeda [:sotaro]]

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.

Comment 30

[Sotaro Ikeda [:sotaro]]

Hi Kan-Ru Chen, can I have a comment to this bug?
IMHO, it is good for FirefoxOS(gonk).

thanks

Comment 31

[Kan-Ru Chen [:kanru] (UTC+9)]

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

Comment 32

[Sotaro Ikeda [:sotaro]]

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.

Comment 33

[Sotaro Ikeda [:sotaro]]

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

Comment 34

[Sotaro Ikeda [:sotaro]]

Hi mikeh, can I have a comment about this bug?

thanks

Comment 35

[Sotaro Ikeda [:sotaro]]

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.

Comment 36

[Sotaro Ikeda [:sotaro]]

Attachment: Part 1 rev2 - Enable Binder ThreadPool

re-base

Comment 37

[Sotaro Ikeda [:sotaro]]

Attachment: Part 2 rev2 - Use OmxClient in OmxDecoder

re-base

Comment 38

[Sotaro Ikeda [:sotaro]]

Attachment: Part 4 rev2 - Use OmxClient in GonkRecorder

re-base

Comment 39

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5 rev4 - Use android::Camera to control camera hw

re-base and some small updates

Comment 40

[Sotaro Ikeda [:sotaro]]

I confirmed that the patches works on unagi phone.

Comment 41

[Sotaro Ikeda [:sotaro]]

attachment 703403 [details] [diff] [review] do not apply Bug 830995. It is necessary to apply the bug.

Comment 42

[Sotaro Ikeda [:sotaro]]

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

Comment 43

[Sotaro Ikeda [:sotaro]]

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.

Comment 44

[Sotaro Ikeda [:sotaro]]

Attachment: Part 1 rev3 - Enable Binder ThreadPool

re-base

Comment 45

[Sotaro Ikeda [:sotaro]]

Attachment: Part 2 rev3 - Use OmxClient in OmxDecoder

re-base

Comment 46

[Sotaro Ikeda [:sotaro]]

Attachment: Part 4 rev3 - Use OmxClient in GonkRecorder

re-base

Comment 47

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5 rev5 - Use android::Camera to control camera hw

- apply Bug 830995 change
- apply Bug 835367 attachment 707604 [details] [diff] [review]
- fix as recording to work again
- fix nits

Comment 48

[Sotaro Ikeda [:sotaro]]

Attachment: GonkNativeWindow.h

as reference

Comment 49

[Sotaro Ikeda [:sotaro]]

Attachment: GonkNativeWindow.cpp

as reference

Comment 50

[Sotaro Ikeda [:sotaro]]

I confirmed the patches works on unagi phone.

Comment 51

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 52

[Sotaro Ikeda [:sotaro]]

some patches become bitrotted. I am going to update them. Then create the rollup patch.

Comment 53

[Sotaro Ikeda [:sotaro]]

Attachment: Part 2 rev4 - Use OmxClient in OmxDecoder

unbitrot

Comment 54

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5 rev6 - Use android::Camera to control camera hw

unbitrot

Comment 55

[Sotaro Ikeda [:sotaro]]

Attachment: rollup patch

just to test the patches easily

Comment 56

[Sotaro Ikeda [:sotaro]]

The patches are made for b2g18, not for mc/mi.

(a73813aea20d0ad83569212cf98a4204e9e02df7)

Comment 57

[Sotaro Ikeda [:sotaro]]

The bug need to apply Bug 836782.

Comment 58

[Sotaro Ikeda [:sotaro]]

I am going to split "Part 5" to get reviews easier.

Comment 62

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 63

[Sotaro Ikeda [:sotaro]]

I update the patch soon.

Comment 64

[Sotaro Ikeda [:sotaro]]

Attachment: Part 1a rev1 - Enable Binder ThreadPool

split "Part 1 rev3" to get a review easily

Comment 65

[Sotaro Ikeda [:sotaro]]

Attachment: Part 1b rev1 - change Makefile.in for Enable Binder ThreadPool

split "Part 1 rev3" to get a review easily

Comment 66

[Sotaro Ikeda [:sotaro]]

Attachment: Part 2 rev5 - Use OmxClient in OmxDecoder

un-bitrot

Comment 67

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5a rev1 - Split GonkNativeWindow to GonkNativeWindow and GonkNativeWindowClient

split "Part 5" to get a review

Comment 68

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5b rev1 - Change OmxDecode to use GonkNativeWindowClient

split "Part 5" to get a review

Comment 69

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5c rev1 - Cange GonkCamera to use android::Camera

split "Part 5" to get a review

Comment 70

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5d rev1 - Cange GonkRecorder to use GonkCameraHardware

split "Part 5" to get a review

Comment 71

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5e rev1 - Change Makefile.in to add GonkNativeWindowClient.cpp

split "Part 5" to get a review

Comment 72

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5f rev1 - Change Makefile.in to add libgui

split "Part 5" to get a review

Comment 73

[Sotaro Ikeda [:sotaro]]

Attachment: rollup patch rev2

update rollup patch

Comment 74

[Kan-Ru Chen [:kanru] (UTC+9)]

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?

Comment 75

[Sotaro Ikeda [:sotaro]]

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

Comment 76

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5a rev2 - Split GonkNativeWindow to GonkNativeWindow and GonkNativeWindowClient

Apply comments.

Comment 77

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

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?

Comment 78

[Sotaro Ikeda [:sotaro]]

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

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

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.

Comment 80

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

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"

Comment 81

[Sotaro Ikeda [:sotaro]]

Attachment: Part 4 rev4 - Use OmxClient in GonkRecorder

Apply a comment. Carry "mhabicher: review+"

Comment 82

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5d rev2 - Cange GonkRecorder to use GonkCameraHardware

Apply the comment. Carry "mhabicher: review+".

Comment 83

[cajbir (:cajbir)]

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.

Comment 84

[Sotaro Ikeda [:sotaro]]

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

Comment 85

[Sotaro Ikeda [:sotaro]]

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

Comment 86

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5c rev2 - Cange GonkCamera to use android::Camera

Apply the comment.

Comment 87

[Sotaro Ikeda [:sotaro]]

(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

Comment 88

[Dietrich Ayala (:dietrich)]

Per comment #62, blocking 1.1.

Comment 89

[Kan-Ru Chen [:kanru] (UTC+9)]

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.

Comment 90

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Needed to make important user-visible content work.

Comment 91

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5a rev3 - Split GonkNativeWindow to GonkNativeWindow and GonkNativeWindowClient

Apply the comment. Carry "kchen: review+".

Comment 92

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5a rev4 - Split GonkNativeWindow to GonkNativeWindow and GonkNativeWindowClient

Carry "kchen: review+".
update just to to fix the patch that I have forgotton to add GonkNativeWindowClient.

Comment 93

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5c rev3 - Cange GonkCamera to use android::Camera

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

Comment 94

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

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

Comment 95

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5c rev4 - Cange GonkCamera to use android::Camera

Apply the comment. Carry "mhabicher: review+".

Comment 96

[Sotaro Ikeda [:sotaro]]

Attachment: rollup patch rev3

update the patch

Comment 97

[Sotaro Ikeda [:sotaro]]

mwu, do you have a time to review?

Comment 98

[Michael Wu [:mwu]]

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.

Comment 99

[Sotaro Ikeda [:sotaro]]

Attachment: Part 1a rev2 - Enable Binder ThreadPool

A patch for commit.
Apply the comment. Carry "mwu: review+".

Comment 100

[Sotaro Ikeda [:sotaro]]

Attachment: Part 1b rev2 - change Makefile.in for Enable Binder ThreadPool

A patch for commit. Carry "cjones: review+".

Comment 101

[Sotaro Ikeda [:sotaro]]

Attachment: Part 2 rev6 - Use OmxClient in OmxDecoder

A patch for commit. Carry "chris.double: review+".

Comment 102

[Michael Wu [:mwu]]

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.

Comment 103

[Sotaro Ikeda [:sotaro]]

Attachment: Part 4 rev5 - Use OmxClient in GonkRecorder

A patch for commit. Carry "mhabicher: review+".

Comment 104

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5a rev5 - Split GonkNativeWindow to GonkNativeWindow and GonkNativeWindowClient

A patch for commit. Carry "kchen: review+".

Comment 105

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5b rev2 - Change OmxDecode to use GonkNativeWindowClient

A patch for commit. Carry "chris.double: review+".

Comment 106

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5c rev5 - Cange GonkCamera to use android::Camera

A patch for commit. Carry "mhabicher: review+".

Comment 107

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5d rev3 - Change GonkRecorder to use GonkCameraHardware

A patch for commit. Carry "mhabicher: review+".

Comment 108

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5e rev2 - Change Makefile.in to add GonkNativeWindowClient.cpp

A patch for commit. Carry "mhabicher: review+".

Comment 109

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5f rev12 - Change Makefile.in to add libgui

A patch for commit. Carry "mwu: review+".

Comment 110

[Sotaro Ikeda [:sotaro]]

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

Comment 111

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #110)
> https://tbpl.mozilla.org/?tree=Try&rev=b77a34a4f40c

try busted. need to address.

Comment 112

[Sotaro Ikeda [:sotaro]]

Attachment: Part 1a rev3 - Enable Binder ThreadPool

A patch for commit.
Fix indent. Carry "mwu: review+".

Comment 113

[Sotaro Ikeda [:sotaro]]

Attachment: Part 1b rev3 - change Makefile.in for Enable Binder ThreadPool

Fix bustage in linux build. Carry "cjones: review+".

Comment 114

[Sotaro Ikeda [:sotaro]]

Attachment: Part 5c rev6 - Change GonkCamera to use android::Camera

Fix bustage in B2G Arm debug build. Carry "mhabicher: review+".

Comment 115

[Sotaro Ikeda [:sotaro]]

Attachment: Part 1b rev4 - change Makefile.in for Enable Binder ThreadPool

update nit. Carry "cjones: review+".

Comment 116

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=567fe2b65270

Comment 117

[Sotaro Ikeda [:sotaro]]

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

Comment 118

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d477e8934f1c
https://hg.mozilla.org/integration/mozilla-inbound/rev/2422115b7400
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a29e43d47af
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc9d903311b7
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e45de4653f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fb64985500c
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7f37db7d738
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f0a8b55cd68
https://hg.mozilla.org/integration/mozilla-inbound/rev/855c69ddfc87
https://hg.mozilla.org/integration/mozilla-inbound/rev/a19b312bd4f5

Comment 119

[Sotaro Ikeda [:sotaro]]

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

Comment 120

[Sotaro Ikeda [:sotaro]]

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

Comment 121

[Sotaro Ikeda [:sotaro]]

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

Comment 122

[Sotaro Ikeda [:sotaro]]

Following is better to play H.264 videos in web browser in FirefoxOS.
http://people.mozilla.org/~cpeterson/videos/

Comment 123

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d477e8934f1c
https://hg.mozilla.org/mozilla-central/rev/2422115b7400
https://hg.mozilla.org/mozilla-central/rev/8a29e43d47af
https://hg.mozilla.org/mozilla-central/rev/bc9d903311b7
https://hg.mozilla.org/mozilla-central/rev/1e45de4653f3
https://hg.mozilla.org/mozilla-central/rev/6fb64985500c
https://hg.mozilla.org/mozilla-central/rev/c7f37db7d738
https://hg.mozilla.org/mozilla-central/rev/7f0a8b55cd68
https://hg.mozilla.org/mozilla-central/rev/855c69ddfc87
https://hg.mozilla.org/mozilla-central/rev/a19b312bd4f5

Comment 124

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/d031aae8da48
https://hg.mozilla.org/releases/mozilla-b2g18/rev/aaca95d502f5
https://hg.mozilla.org/releases/mozilla-b2g18/rev/b32ffb4cbf92
https://hg.mozilla.org/releases/mozilla-b2g18/rev/24505d927c2e
https://hg.mozilla.org/releases/mozilla-b2g18/rev/46283b144f44
https://hg.mozilla.org/releases/mozilla-b2g18/rev/ea270327e0fb
https://hg.mozilla.org/releases/mozilla-b2g18/rev/68d02c25635b
https://hg.mozilla.org/releases/mozilla-b2g18/rev/937c9560600d
https://hg.mozilla.org/releases/mozilla-b2g18/rev/62082ec34e4b
https://hg.mozilla.org/releases/mozilla-b2g18/rev/472d7bc16303

*fingers crossed*

Comment 125

[Sotaro Ikeda [:sotaro]]

I confirmed the bug is fixed on b2g18 by using unagi.

Comment 127

[Jason Smith [:jsmith]]

Naoki said he was going to take this for creating the moztrap test cases here, so I'm flagging him here.

Comment 128

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Hi Sotaro, 
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

Comment 129

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

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:

Comment 130

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

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:

Comment 131

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

There is no b2g18-v1.0.1 flag, so I use the b2g18 and uplift to v1.0.1.

Comment 132

[Sotaro Ikeda [:sotaro]]

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

Comment 133

[Alex Keybl [:akeybl]]

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.

Comment 134

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

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?

Comment 135

[John Hammink]

Removing NI as there is now a testcase covering it.