Sotaro Ikeda [:sotaro] Assignee
	Description • 12 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 1 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 2 • 12 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 3 • 12 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 4 • 12 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 5 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 6 • 12 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 7 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 8 • 12 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 9 • 12 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 10 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 11 • 12 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 12 • 12 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 13 • 12 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 14 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 15 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 16 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 17 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 18 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 19 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 20 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 21 • 12 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 22 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 23 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 24 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 25 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 26 • 12 years ago

just update

	Sotaro Ikeda [:sotaro] Assignee
	Comment 27 • 12 years ago

just update

	Sotaro Ikeda [:sotaro] Assignee
	Comment 28 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 29 • 12 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 30 • 12 years ago

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

thanks

	Kan-Ru Chen [:kanru] (UTC+9)
	Comment 31 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 32 • 12 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 33 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 34 • 12 years ago

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

thanks

	Sotaro Ikeda [:sotaro] Assignee
	Comment 35 • 12 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 36 • 12 years ago

re-base

	Sotaro Ikeda [:sotaro] Assignee
	Comment 37 • 12 years ago

re-base

	Sotaro Ikeda [:sotaro] Assignee
	Comment 38 • 12 years ago

re-base

	Sotaro Ikeda [:sotaro] Assignee
	Comment 39 • 12 years ago

re-base and some small updates

	Sotaro Ikeda [:sotaro] Assignee
	Comment 40 • 12 years ago

I confirmed that the patches works on unagi phone.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 41 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 42 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 43 • 12 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 44 • 12 years ago

re-base

	Sotaro Ikeda [:sotaro] Assignee
	Comment 45 • 12 years ago

re-base

	Sotaro Ikeda [:sotaro] Assignee
	Comment 46 • 12 years ago

re-base

	Sotaro Ikeda [:sotaro] Assignee
	Comment 47 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 48 • 12 years ago

as reference

	Sotaro Ikeda [:sotaro] Assignee
	Comment 49 • 12 years ago

as reference

	Sotaro Ikeda [:sotaro] Assignee
	Comment 50 • 12 years ago

I confirmed the patches works on unagi phone.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 51 • 12 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 52 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 53 • 12 years ago

unbitrot

	Sotaro Ikeda [:sotaro] Assignee
	Comment 54 • 12 years ago

unbitrot

	Sotaro Ikeda [:sotaro] Assignee
	Comment 55 • 12 years ago

just to test the patches easily

	Sotaro Ikeda [:sotaro] Assignee
	Comment 56 • 12 years ago

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

(a73813aea20d0ad83569212cf98a4204e9e02df7)

	Sotaro Ikeda [:sotaro] Assignee
	Comment 57 • 12 years ago

The bug need to apply Bug 836782.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 58 • 12 years ago

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

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 62 • 12 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 63 • 12 years ago

I update the patch soon.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 64 • 12 years ago

split "Part 1 rev3" to get a review easily

	Sotaro Ikeda [:sotaro] Assignee
	Comment 65 • 12 years ago

split "Part 1 rev3" to get a review easily

	Sotaro Ikeda [:sotaro] Assignee
	Comment 66 • 12 years ago

un-bitrot

	Sotaro Ikeda [:sotaro] Assignee
	Comment 67 • 12 years ago

split "Part 5" to get a review

	Sotaro Ikeda [:sotaro] Assignee
	Comment 68 • 12 years ago

split "Part 5" to get a review

	Sotaro Ikeda [:sotaro] Assignee
	Comment 69 • 12 years ago

split "Part 5" to get a review

	Sotaro Ikeda [:sotaro] Assignee
	Comment 70 • 12 years ago

split "Part 5" to get a review

	Sotaro Ikeda [:sotaro] Assignee
	Comment 71 • 12 years ago

split "Part 5" to get a review

	Sotaro Ikeda [:sotaro] Assignee
	Comment 72 • 12 years ago

split "Part 5" to get a review

	Sotaro Ikeda [:sotaro] Assignee
	Comment 73 • 12 years ago

update rollup patch

	Kan-Ru Chen [:kanru] (UTC+9)
	Comment 74 • 12 years ago

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?

	Sotaro Ikeda [:sotaro] Assignee
	Comment 75 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 76 • 12 years ago

Apply comments.

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 77 • 12 years ago

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?

	Sotaro Ikeda [:sotaro] Assignee
	Comment 78 • 12 years ago

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

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 79 • 12 years ago

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.

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 80 • 12 years ago

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"

	Sotaro Ikeda [:sotaro] Assignee
	Comment 81 • 12 years ago

Apply a comment. Carry "mhabicher: review+"

	Sotaro Ikeda [:sotaro] Assignee
	Comment 82 • 12 years ago

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

	cajbir (:cajbir)
	Comment 83 • 12 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 84 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 85 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 86 • 12 years ago

Apply the comment.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 87 • 12 years ago

(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

	Dietrich Ayala (:dietrich)
	Comment 88 • 12 years ago

Per comment #62, blocking 1.1.

	Kan-Ru Chen [:kanru] (UTC+9)
	Comment 89 • 12 years ago

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.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 90 • 12 years ago

Needed to make important user-visible content work.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 91 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 92 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 93 • 12 years ago

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

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 94 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 95 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 96 • 12 years ago

update the patch

	Sotaro Ikeda [:sotaro] Assignee
	Comment 97 • 12 years ago

mwu, do you have a time to review?

	Michael Wu [:mwu]
	Comment 98 • 12 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 99 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 100 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 101 • 12 years ago

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

	Michael Wu [:mwu]
	Comment 102 • 12 years ago

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.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 103 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 104 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 105 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 106 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 107 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 108 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 109 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 110 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 111 • 12 years ago

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

try busted. need to address.

	Sotaro Ikeda [:sotaro] Assignee
	Comment 112 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 113 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 114 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 115 • 12 years ago

update nit. Carry "cjones: review+".

	Sotaro Ikeda [:sotaro] Assignee
	Comment 116 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 117 • 12 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 118 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 119 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 120 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 121 • 12 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 122 • 12 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 123 • 12 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 124 • 12 years ago

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*

	Sotaro Ikeda [:sotaro] Assignee
	Comment 125 • 12 years ago

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

	Jason Smith [:jsmith]
	Comment 127 • 11 years ago

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

	Randy Lin [:rlin] - Be Mozillian on 2015/01/26
	Comment 128 • 11 years ago

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

	Randy Lin [:rlin] - Be Mozillian on 2015/01/26
	Comment 129 • 11 years ago

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:

	Randy Lin [:rlin] - Be Mozillian on 2015/01/26
	Comment 130 • 11 years ago

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:

	Randy Lin [:rlin] - Be Mozillian on 2015/01/26
	Comment 131 • 11 years ago

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

	Sotaro Ikeda [:sotaro] Assignee
	Comment 132 • 11 years ago

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

	Alex Keybl [:akeybl]
	Comment 133 • 11 years ago

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.

	Randy Lin [:rlin] - Be Mozillian on 2015/01/26
	Comment 134 • 11 years ago

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?