825110 - [meta] Porting WebRTC video_module for B2G

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Comment 1

[Chiajung Hung [:chiajung]]

Attachment: [WIP] V1

Here is a work-in-process patch for B2G WebRTC video_module.

The implementation basically porting from Android/Linux version. And it /*depends*/ on Android HAL and part of framework classes.

It is a test patch for anyone interest in this feature on B2G. It is expected that some function may not work correctly. I will need more detail comparison between other platform implementation to mature it.

Any feedback is welcome!

Comment 2

[Chiajung Hung [:chiajung]]

Attachment: [WIP] V1 part#2

This patch change some common audio_module code. 

WARNING: This patch will cause build break now. We need a complete B2G WebRTC build related change to build it correctly. (See Bug#818843)

Comment 3

[Chiajung Hung [:chiajung]]

Oh...I found I uploaded the patch part#2 to wrong bug.
It is for 825112.

Comment 4

[Chiajung Hung [:chiajung]]

Attachment: [WIP] V2

Minor clean up to exclude Make system related files, and clean out some log spam.

Comment 5

[Chiajung Hung [:chiajung]]

Attachment: Patch V1

Hi dmose,

I would like you to review my patch for enable WebRTC video capture module on B2G. 

Here is some change from last patch:
  - Remove the thread. Since all camera related HAL function call do not block the calling thread, we can use the MediaThread directly.
  - Remove flooding logs
  - Some format clean up

Thanks

Comment 6

[[:fabrice] Fabrice Desré]

I have a couple of questions about these patches:
- are they e10s ready?
- don't we duplicate some of the code written for the camera api?

Comment 7

[Chiajung Hung [:chiajung]]

(In reply to Fabrice Desré [:fabrice] from comment #6)
> I have a couple of questions about these patches:
> - are they e10s ready?
Do you mean OOP/permission related problem? I think they are handled by MediaManager and this patch do not touch that part.

> - don't we duplicate some of the code written for the camera api?
Yes, I think most code may be similar (but much simpler). I implemented it this way since I want to keep the architecture similar with most other platforms. It is ok for me to write video_device as a wrapper of dom/camera. If you think we should do it that way then I can try to implement it later.

Comment 8

[[:fabrice] Fabrice Desré]

(In reply to Chiajung Hung [:chiajung] from comment #7)
> (In reply to Fabrice Desré [:fabrice] from comment #6)
> > I have a couple of questions about these patches:
> > - are they e10s ready?
> Do you mean OOP/permission related problem? I think they are handled by
> MediaManager and this patch do not touch that part.

I'm pretty sure that the MediaManager code doesn't take into account OOP apps. We need to get that right for b2g.

> > - don't we duplicate some of the code written for the camera api?
> Yes, I think most code may be similar (but much simpler). I implemented it
> this way since I want to keep the architecture similar with most other
> platforms. It is ok for me to write video_device as a wrapper of dom/camera.
> If you think we should do it that way then I can try to implement it later.

Yes, I would prefer that if we can avoid code duplication.

Comment 9

[Chiajung Hung [:chiajung]]

(In reply to Fabrice Desré [:fabrice] from comment #8)
> (In reply to Chiajung Hung [:chiajung] from comment #7)
> > (In reply to Fabrice Desré [:fabrice] from comment #6)
> > > I have a couple of questions about these patches:
> > > - are they e10s ready?
> > Do you mean OOP/permission related problem? I think they are handled by
> > MediaManager and this patch do not touch that part.
> 
> I'm pretty sure that the MediaManager code doesn't take into account OOP
> apps. We need to get that right for b2g.
OK, I will file a new bug for that part later.
> 
> > > - don't we duplicate some of the code written for the camera api?
> > Yes, I think most code may be similar (but much simpler). I implemented it
> > this way since I want to keep the architecture similar with most other
> > platforms. It is ok for me to write video_device as a wrapper of dom/camera.
> > If you think we should do it that way then I can try to implement it later.
> 
> Yes, I would prefer that if we can avoid code duplication.
Then there are 2 possible solutions to use dom/camera code:

1. Wrap dom/camera in MediaEngine
   - Let MediaEngine manipulate CameraControl on B2G, bypass whole video_engine (and maybe MediaEngineWebRTCVideo)
   - Pros: Better performance if we do not alter the frame
   - Cons: If we have to manipulate each frame (e.g. denoising, stablization, encoding, etc.), we have to figure out how to use WebRTC's code or implement our version. 

2. Wrap dom/camera in video_module
   - Let VideoModuleGonk manipulate CameraControl on B2G, memcpy each frame for WebRTC engine
   - Pros: We can use all other code in video_module
   - Cons: The performance is as bad as current implementation or worse.

I prefer solution 1. How about you? :)

Comment 10

[[:fabrice] Fabrice Desré]

2. is unacceptable for obvious performance reasons. Let's go with 1. !

Comment 11

[Chiajung Hung [:chiajung]]

(In reply to Fabrice Desré [:fabrice] from comment #10)
> 2. is unacceptable for obvious performance reasons. Let's go with 1. !

OK! Let me sync with StevenLee about how peer connection request VP8 encoded frame now first, then start to implement new solution. :)

Comment 12

[Chiajung Hung [:chiajung]]

(In reply to Chiajung Hung [:chiajung] from comment #11)
> (In reply to Fabrice Desré [:fabrice] from comment #10)
> > 2. is unacceptable for obvious performance reasons. Let's go with 1. !
> 
> OK! Let me sync with StevenLee about how peer connection request VP8 encoded
> frame now first, then start to implement new solution. :)

The low level network module will create a video_engine to encode video frames into VP8 format in current WebRTC framework. We may have to handle that part, too. 

And I reviewed some part of dom/camera implementation, which contains many JS related stuff that is redundant for WebRTC video usage. For example, WebRTC enumerate device and get device name separately in Media thread, but dom/camera do them in one function in main thread. If I try to call it directly, I will have to handle unnecessary JS stuffs in pure C++ code. In such case, I may need refactor part of dom/camera codes for better code share. :S

Comment 13

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

Comment on attachment 704393 [details] [diff] [review]
Patch V1

It doesn't look like you're calling takePicture() on the camera, so just FYI: bug 830995 landed a patch for GonkNativeWindow to fix a potential deadlock.

Comment 14

[Chiajung Hung [:chiajung]]

(In reply to Mike Habicher [:mikeh] from comment #13)
> Comment on attachment 704393 [details] [diff] [review]
> Patch V1
> 
> It doesn't look like you're calling takePicture() on the camera, so just
> FYI: bug 830995 landed a patch for GonkNativeWindow to fix a potential
> deadlock.

In fact, I am going to abandon this patch to use dom/camera directly. But I have no enough time to do that now :S

After I can switch to new architecture, I think we can avoid such problems as you guys fixed them already :P

Comment 15

[Chiajung Hung [:chiajung]]

Attachment: Patch V2 WIP

This version utilize dom/camera

Main feature:
- Utilize dom/camera code
  - Reduce frame-by-frame memcpy (reduces 2 times per frame)
- Fix MediaPipeline related part to handle GonkIOSurfaceImage

I will start to clean this patch.

Comment 16

[Chiajung Hung [:chiajung]]

Attachment: Patch V2

This patch complete the video module rewrite to utilize dom/camera API. And this one handled all threading related stuff and enable takePicture from previous WIP patch.

Since dom/camera can avoid memory copy 2 times per frame, this should enhance the WebRTC performance on B2G. (I think we can further improve it by avoid frame-by-frame color space convertion, later.)

Comment 17

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

Comment on attachment 718850 [details] [diff] [review]
Patch V2

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

Drive-by review of the dom/camera files:
- bug 803471 changed how we access the camera, which will affect a number of calls (that I have noted) in GonkCameraManager.cpp;
- please make sure than any updated patches use -U8 for more context (e.g. git diff -U8).

::: dom/camera/DOMCameraManager.cpp
@@ +82,4 @@
>    uint32_t permission = nsIPermissionManager::DENY_ACTION;
>    permMgr->TestPermissionFromWindow(aWindow, "camera", &permission);
>    if (permission != nsIPermissionManager::ALLOW_ACTION) {
> +    __android_log_print(ANDROID_LOG_INFO, "DOMCameraManger", "Permission denied");

Consider replacing with DOM_CAMERA_LOGI().

::: dom/camera/DOMCameraManager.h
@@ +46,4 @@
>    void Register(mozilla::nsDOMCameraControl* aDOMCameraControl);
>    void OnNavigation(uint64_t aWindowId);
>  
> +  nsresult GetCameraNumber(uint32_t &deviceCount);

'aDeviceCount'

@@ +46,5 @@
>    void Register(mozilla::nsDOMCameraControl* aDOMCameraControl);
>    void OnNavigation(uint64_t aWindowId);
>  
> +  nsresult GetCameraNumber(uint32_t &deviceCount);
> +  nsresult GetCameraName(uint32_t deviceNum, nsCString& deviceName);

'aDeviceNum' and 'aDeviceName'

::: dom/camera/FallbackCameraManager.cpp
@@ +5,5 @@
>  #include "DOMCameraManager.h"
>  
>  // From nsDOMCameraManager.
> +nsresult
> +nsDOMCameraManager::GetCameraNumber(uint32_t &deviceCount)

'aDeviceCount'

@@ +11,5 @@
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +};
> +
> +nsresult
> +nsDOMCameraManager::GetCameraName(uint32_t deviceNum, nsCString& deviceName)

'aDeviceNum' and 'aDeviceName'

::: dom/camera/GonkCameraManager.cpp
@@ +21,5 @@
>  #include "CameraCommon.h"
>  
>  // From nsDOMCameraManager, but gonk-specific!
> +nsresult
> +nsDOMCameraManager::GetCameraNumber(uint32_t& deviceCount)

Please rename this to GetNumberOfCameras().

Also, parameter name should be 'aDeviceCount'.

@@ +24,5 @@
> +nsresult
> +nsDOMCameraManager::GetCameraNumber(uint32_t& deviceCount)
> +{
> +  camera_module_t* module;
> +  if (hw_get_module(CAMERA_HARDWARE_MODULE_ID, (const hw_module_t**)&module) < 0) {

The changes in bug 803471 will (likely) break this.

See https://bugzilla.mozilla.org/attachment.cgi?id=722448&action=diff#a/dom/camera/GonkCameraManager.cpp_sec2

@@ +29,5 @@
> +    DOM_CAMERA_LOGE("GetCameraNumber : Could not load camera HAL module");
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +
> +  deviceCount = module->get_number_of_cameras();

The changes in bug 803471 will (likely) break this.

@@ +34,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +nsDOMCameraManager::GetCameraName(uint32_t deviceNum, nsCString& deviceName)

'aDeviceNum' and 'aDeviceName'.

@@ +37,5 @@
> +nsresult
> +nsDOMCameraManager::GetCameraName(uint32_t deviceNum, nsCString& deviceName)
> +{
> +  camera_module_t* module;
> +  if (hw_get_module(CAMERA_HARDWARE_MODULE_ID, (const hw_module_t**)&module) < 0) {

The changes in bug 803471 will (likely) break this.

@@ +42,5 @@
> +	DOM_CAMERA_LOGE("GetCameraName : Could not load camera HAL module");
> +	return NS_ERROR_NOT_AVAILABLE;
> +  }
> +
> +  uint32_t count = module->get_number_of_cameras();

The changes in bug 803471 will (likely) break this.

@@ +50,5 @@
> +	return NS_ERROR_NOT_AVAILABLE;
> +  }
> +
> +  struct camera_info info;
> +  int rv = module->get_camera_info(deviceNum, &info);

The changes in bug 803471 will (likely) break this.

@@ +55,5 @@
> +  if (rv != 0) {
> +    DOM_CAMERA_LOGE("GetCameraName : get_camera_info(%d) failed: %d\n", deviceNum, rv);
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +

Remove this extra blank line.

@@ +58,5 @@
> +  }
> +
> +
> +  switch (info.facing) {
> +	case CAMERA_FACING_BACK:

Please indent this two-spaces only:

  switch (info.facing) {
    case ...
      // code
      break
  }

@@ +67,5 @@
> +	  deviceName.Assign("front");
> +	  break;
> +
> +	default:
> +	  {

Please remove this redundant scope.

@@ +81,5 @@
>  NS_IMETHODIMP
>  nsDOMCameraManager::GetListOfCameras(JSContext* cx, JS::Value* _retval)
>  {
>    JSObject* a = JS_NewArrayObject(cx, 0, nullptr);
> +  //camera_module_t* module;

Remove this line.

@@ +100,1 @@
>    DOM_CAMERA_LOGI("getListOfCameras : get_number_of_cameras() returned %d\n", count);

Update the text in this logging statement.

Comment 18

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

Comment on attachment 718850 [details] [diff] [review]
Patch V2

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

Just a quick review of non-dom/camera stuff.  I'm not terribly familiar with the WebRTC code base, but noticed a few things.

If possible, we should try to eliminate duplication of the permissions-check between the camera API and WebRTC.

::: content/media/webrtc/MediaEngineWebRTC.cpp
@@ +37,5 @@
>  MediaEngineWebRTC::EnumerateVideoDevices(nsTArray<nsRefPtr<MediaEngineVideoSource> >* aVSources)
>  {
> +#ifdef MOZ_WIDGET_GONK
> +  MutexAutoLock lock(mMutex);
> +  if (!mCameraManager) {

This looks like a duplicate of nsDOMCameraManager::CheckPermissionAndCreateInstance().  Can we refactor that code to avoid this duplication?

@@ +81,5 @@
> +    }
> +  }
> +
> +  return;
> +

nit: remove extra blank line here.

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +89,5 @@
> +    , mMinFps(aMinFps)
> +    , mInitDone(false)
> +    , mInSnapshotMode(false)
> +    , mSnapshotPath(NULL)
> +	, mPreviewStream(nullptr) {

Please make sure 'mPreviewStream' lines up with the other initializers, and put the opening brace on a new line.

@@ +91,5 @@
> +    , mInSnapshotMode(false)
> +    , mSnapshotPath(NULL)
> +	, mPreviewStream(nullptr) {
> +    mState = kReleased;
> +    NS_NewThread(getter_AddRefs(mCameraThread), nullptr);

I think it would be better to refactor DOMCameraManager so that we don't need to duplicate mCameraThread here.  What do you think?

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +396,5 @@
> +  cameraPosition.longitude = NAN;
> +  cameraPosition.altitude = NAN;
> +  cameraPosition.timestamp = NAN;
> +
> +  TimeDuration duration = TimeStamp::Now() - mStartTime;

This is used to populate the date/time field in the EXIF header of the resulting image/jpeg blob; it should be the number of seconds since January 1, 1970 UTC.  I'm not sure what you're doing here.

@@ +406,5 @@
> +  // The camera return nsDOMMemoryFile indeed, and the inheritance tree is:
> +  // nsIDOMBlob <- nsIDOMFile <- nsDOMFileBase <- nsDOMFile <- nsDOMMemoryFile
> +  *aFile = static_cast<nsIDOMFile*>(mLastCapture.get());
> +  return NS_OK;
> +

Can probably remove this extra blank line.

@@ +553,5 @@
>  
> +#ifdef MOZ_WIDGET_GONK
> +// nsICameraGetCameraCallback
> +nsresult
> +MediaEngineWebRTCVideoSource::HandleEvent(nsICameraControl *camera) {

nit: the style for this file seems to be:
nsICameraControl* camera

@@ +593,5 @@
> +}
> +
> +// nsICameraErrorCallback
> +nsresult
> +MediaEngineWebRTCVideoSource::HandleEvent(const nsAString & error) {

nit: the style for this file seems to be:
'const nsAString& error'

@@ +606,5 @@
> +                                                            TrackTicks aTrackOffset,
> +                                                            uint32_t aTrackEvents,
> +                                                            const MediaSegment& aQueuedMedia) {
> +  ReentrantMonitorAutoEnter enter(mMonitor);
> +  MediaSegment *mediaSegment = const_cast<MediaSegment*>(&aQueuedMedia);

nit: the style for this file seems to be:
'MediaSegment* mediaSegment'

::: dom/media/MediaManager.cpp
@@ +991,5 @@
>  
> +#ifdef MOZ_WIDGET_GONK
> +  /*if (picture)*/ {
> +    // ShowFilePickerForMimeType() must run on the Main Thread! (on Android)
> +	mMediaThread->Dispatch(gUMRunnable, NS_DISPATCH_NORMAL);

Please fix indentation.

::: media/webrtc/Makefile.in
@@ +20,2 @@
>    trunk/testing \
> +	$(NULL)

nit: indent with spaces instead of tabs

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +786,5 @@
> +    void *basePtr;
> +    android::sp<android::GraphicBuffer> graphicBuffer = layers::GrallocBufferActor::GetFrom(grallocHandle);
> +    graphicBuffer->lock(android::GraphicBuffer::USAGE_SW_READ_MASK, (void**)&basePtr);
> +    conduit->SendVideoFrame((unsigned char*)basePtr,
> +    		                graphicBuffer->getWidth() * graphicBuffer->getHeight() * 3 / 2,

nit: line up the start of these parameters with the opening bracket of '(unsigned char*)...'

Comment 19

[Chiajung Hung [:chiajung]]

Hi Mike,

For dom/camera part:
I will update and rebase to see how to handle them.

For webrtc part:
(1) For Permission duplications, I will double check later, and give you feedback then.

(2) For CameraThread problem, I have no idea about how to make it better for both CameraAPI/WebRTC usage. Currently, WebRTC would like to use GonkCameraControl directly (since it is JSContext free), and use its functions as blocking call (to be able to check problem, and fire event if problem occurred), but GonkCameraControl will run asyncly and fire it event to main thread, thus we can not make it run on MainThread. And since MediaThread need to know the camera manipulation result, we can not use MediaThread as CameraThread directly. That's why I create a CameraThread here and Monitor a mutex for those call...

    The best possible I can think is make GonkCameraControl thread free (make the function call become blocking, and return status to caller, and make the called fire event as needed). But I am not sure whether it is feasible, and I think such change may over the scope of this bug. :S

(3) For Picture timestamp...I just have no clue about how to fill the value correctly from GonkCameraControl's comment. I will fix it later :P

Let me fix them and upload new version! Thanks for review!

Comment 20

[Chiajung Hung [:chiajung]]

Attachment: Patch V3

Changelog:
  - Fix various indention problem
  - Fix function naming problem
  - Move MediaEngineWebRTC::mCameraManager initiation from EnumerateVideoDevices to constructor

By the way, the nsDOMCameraManager::CheckPermissionAndCreateInstance(window) call in MediaEngineWebRTC is not duplicated permission check I think. I check it here to make sure we have camera permission when we access camera from GetUserMedia. And I think there is no other code in current codebase check for this.

For CameraThread problem, I think we can file another follow up bug to discuss it. I think I should not change them in this patch :P

Comment 21

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

Comment on attachment 726529 [details] [diff] [review]
Patch V3

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

** PLEASE use provide 8 lines of context in all future patches (e.g. specify git diff -U8, or hg diff -U8). **

A few style issues remaining; r- because of the incorrect use of PR_Now().

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +84,5 @@
> +    , mCallbackMonitor("WebRTCCamera.CallbackMonitor")
> +    , mInitDone(false)
> +    , mInSnapshotMode(false)
> +    , mSnapshotPath(NULL)
> +    , mPreviewStream(nullptr) {

style nit:
  MediaEngineWebRTCVideoSource(nsRefPtr<nsDOMCameraManager> aCameraManager,
    int aIndex, uint64_t aWindowId)
      ...
    , mPreviewStream(nullptr)
  {
     ...
  }

@@ +311,5 @@
>    , mVideoEngine(NULL)
>    , mVoiceEngine(NULL)
>    , mVideoEngineInit(false)
>    , mAudioEngineInit(false)
> +  , mWindowId(aWindowId)

style nit: the above six lines should all be indented 2 spaces.

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +424,5 @@
> +  cameraPosition.longitude = NAN;
> +  cameraPosition.altitude = NAN;
> +  cameraPosition.timestamp = NAN;
> +
> +  mNativeCameraControl->TakePicture(pictureSize, 0, NS_LITERAL_STRING("image/jpeg"), cameraPosition, (uint64_t)PR_Now() / 1000, this, this);

PR_Now() returns microseconds since epoch[1]; TakePicture() requires seconds.

Also, the cast shouldn't be necessary, as PRTime is a PRUint64/uint64_t[2].

1. https://developer.mozilla.org/en-US/docs/PR_Now
2. https://developer.mozilla.org/en-US/docs/PRTime

@@ +639,5 @@
> +                                                       TrackTicks aTrackOffset,
> +                                                       uint32_t aTrackEvents,
> +                                                       const MediaSegment& aQueuedMedia) {
> +  MonitorAutoLock enter(mMonitor);
> +  MediaSegment *mediaSegment = const_cast<MediaSegment*>(&aQueuedMedia);

style nit: 'MediaSegment* mediaSegment'

::: dom/camera/DOMCameraManager.h
@@ +46,4 @@
>    void Register(mozilla::nsDOMCameraControl* aDOMCameraControl);
>    void OnNavigation(uint64_t aWindowId);
>  
> +  nsresult GetNumberOfCameras(int32_t &aDeviceCount);

The & goes with the type, not with the parameter name.

::: dom/camera/FallbackCameraManager.cpp
@@ +5,5 @@
>  #include "DOMCameraManager.h"
>  
>  // From nsDOMCameraManager.
> +nsresult
> +nsDOMCameraManager::GetNumberOfCameras(int32_t &aDeviceCount)

The & goes with the type, not with the parameter name.

::: dom/media/MediaManager.cpp
@@ +1053,5 @@
>  
> +#ifdef MOZ_WIDGET_GONK
> +  if (picture) {
> +    // ShowFilePickerForMimeType() must run on the Main Thread! (on Android)
> +	mMediaThread->Dispatch(gUMRunnable, NS_DISPATCH_NORMAL);

Fix this indentation: your files contains tabs, use spaces.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +61,5 @@
>    // TODO([e-mail]): is there a way to make this async?
>    nsresult ret;
>    RUN_ON_THREAD(sts_thread_,
> +        WrapRunnableRet(this, &MediaPipeline::Init_s, &ret),
> +        NS_DISPATCH_SYNC);

style nit: indent the above two lines so that they line up with the start of sts_thread.

Comment 22

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

(In reply to Chiajung Hung [:chiajung] from comment #20)
> Created attachment 726529 [details] [diff] [review]
> Patch V3
> 
> For CameraThread problem, I think we can file another follow up bug to
> discuss it. I think I should not change them in this patch :P

That sounds good to me.

By the way, I've only looked over the dom/media, content/media, and media/webrtc changes for CameraControl API issues and general style.  You need to ask someone familiar with the WebRTC code to review these changes as well.

Comment 23

[Chiajung Hung [:chiajung]]

Attachment: Patch V4 - [Part1] Camera changes

Changelog:
  - Fix remaining indention and style 

I split original patch to 2 different patch.
Thanks for review!

Comment 24

[Chiajung Hung [:chiajung]]

Attachment: Patch V4 - [Part2] WebRTC changes

WebRTC part

Comment 25

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

Comment on attachment 726983 [details] [diff] [review]
Patch V4 - [Part1] Camera changes

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

r+ with the fix noted below.

::: dom/camera/FallbackCameraManager.cpp
@@ +5,5 @@
>  #include "DOMCameraManager.h"
>  
>  // From nsDOMCameraManager.
> +nsresult
> +nsDOMCameraManager::GetNumberOfCameras(int32_t &aDeviceCount)

The & goes with the type, not the parameter name.

Comment 26

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

Comment on attachment 726984 [details] [diff] [review]
Patch V4 - [Part2] WebRTC changes

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

I just noticed one small style nit as noted below.  f+

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +317,5 @@
>    {
>      mVideoSources.Init();
>      mAudioSources.Init();
> +#ifdef MOZ_WIDGET_GONK
> +    nsPIDOMWindow *window = static_cast<nsPIDOMWindow*>(nsGlobalWindow::GetInnerWindowWithId(mWindowId));

style nit: the * should go with the type, not the variable.

Comment 27

[Chiajung Hung [:chiajung]]

Attachment: Patch V4.1 - [Part1] Camera changes

Fix minor style problem.

I start to think that I should find a style checker to check such problem for me...

Thanks for the review, Mike :)

Comment 28

[Chiajung Hung [:chiajung]]

Attachment: Patch V4.1 - [Part2] WebRTC changes

WebRTC part

Comment 29

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

Comment on attachment 727464 [details] [diff] [review]
Patch V4.1 - [Part1] Camera changes

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

Ship it!  :)

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

Almost everything from bug 825112 comment 10 applies to this patch as well, most especially the testing pieces, which is to say that I don't believe it's reasonable to land this stuff turned on for production code unless the existing C++ and mochitests are passing as well as running under the B2G test automation, for the reasons outlined in that comment.

As far as the right reviewers go, my understanding is that mhabicher is just the right guy from a B2G video sense.  But I think someone else on the platform team (jesup?) is probably a better fit on WebRTC side.

Comment 31

[Eric Rescorla (:ekr)]

Comment on attachment 727466 [details] [diff] [review]
Patch V4.1 - [Part2] WebRTC changes

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

This is solely a review of the VideoConduit and MediaPipeline code.

Please correct the issues below before commit.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +520,5 @@
>      MOZ_ASSERT(PR_FALSE);
>      return kMediaConduitMalformedArgument;
>    }
>  
> +  webrtc::RawVideoType type;

Prefer a switch here.

if you are going to use if/else, the style in this code is:

if (expr) {


} else {


}

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +13,5 @@
>  #include "nspr.h"
>  #include <prlog.h>
>  #include "srtp.h"
>  
> +#define MOZILLA_INTERNAL_API

This is going to cause failure to compile in the desktop unit test framework.

If for some reason MOZILLA_INTERNAL_API isn't being set in the compile on b2g
you need to set it elsewhere.

@@ +61,5 @@
>    // TODO([e-mail]): is there a way to make this async?
>    nsresult ret;
>    RUN_ON_THREAD(sts_thread_,
> +                WrapRunnableRet(this, &MediaPipeline::Init_s, &ret),
> +                NS_DISPATCH_SYNC);

What changed here?

@@ +150,5 @@
>    State *state = rtcp ? &rtcp_state_ : &rtp_state_;
>  
>    if (*state != MP_CONNECTING) {
>      MOZ_MTLOG(PR_LOG_ERROR, "Transport ready for flow in wrong state:" <<
> +          description_ << ": " << (rtcp ? "rtcp" : "rtp"));

This looks like a whitespace-only change to the indent. Please revert.

@@ +157,5 @@
>  
>    nsresult res;
>  
>    MOZ_MTLOG(MP_LOG_INFO, "Transport ready for pipeline " <<
> +        static_cast<void *>(this) << " flow " << description_ << ": " <<

This looks like a whitespace-only change to the indent. Please revert.

@@ +787,2 @@
>  
> +    void *basePtr;

Move this down to right before use.

@@ +787,4 @@
>  
> +    void *basePtr;
> +    android::sp<android::GraphicBuffer> graphicBuffer = layers::GrallocBufferActor::GetFrom(grallocHandle);
> +    graphicBuffer->lock(android::GraphicBuffer::USAGE_SW_READ_MASK, (void**)&basePtr);

No need to cast &basePtr here. It is already void **.

@@ +787,5 @@
>  
> +    void *basePtr;
> +    android::sp<android::GraphicBuffer> graphicBuffer = layers::GrallocBufferActor::GetFrom(grallocHandle);
> +    graphicBuffer->lock(android::GraphicBuffer::USAGE_SW_READ_MASK, (void**)&basePtr);
> +    conduit->SendVideoFrame((unsigned char*)basePtr,

Please use C++ casts in C++ code.

@@ +788,5 @@
> +    void *basePtr;
> +    android::sp<android::GraphicBuffer> graphicBuffer = layers::GrallocBufferActor::GetFrom(grallocHandle);
> +    graphicBuffer->lock(android::GraphicBuffer::USAGE_SW_READ_MASK, (void**)&basePtr);
> +    conduit->SendVideoFrame((unsigned char*)basePtr,
> +                            graphicBuffer->getWidth() * graphicBuffer->getHeight() * 3 / 2,

I would parenthesize the multiplication to make the order of operations clear.

@@ +795,5 @@
> +                            mozilla::kVideoNV21, 0);
> +    graphicBuffer->unlock();
> +    return;
> +  }
> +#endif

Please clean up this format logic here so it is an if/else or a switch. It's really
odd to have fast success return here and a fast fail return right below.

Comment 32

[Chiajung Hung [:chiajung]]

Attachment: Patch V5 - [Part2] WebRTC changes

Further split the WebRTC part to 2 patches

Comment 33

[Chiajung Hung [:chiajung]]

Attachment: Patch V5 - [Part3] WebRTC Media changes

Hi Eric,

I fixed the style problem you mentioned in previous change, and split the patch to two part.

Thanks for review!!!

Comment 34

[alberto.pastor]

I can't compile m-c with these patches in MacOS 10.8 x_64. I'm getting some errors related to'mozilla:layers'.

/Users/alberto/Projects/theLab/B2G/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:782:13: error: no type named 'GonkIOSurfaceImage' in namespace 'mozilla::layers'; did you mean 'MacIOSurfaceImage'?

Should I be able to do it?

Thanks!

Comment 35

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

It looks like that block of code needs to be wrapped in #ifdef MOZ_WIDGET_GONK.

Comment 36

[Chiajung Hung [:chiajung]]

Attachment: Patch V5.1 - [Part3] WebRTC Media changes

Sorry for forget wrap the code around MOZ_GONK_WIDGET.
I think this version fix the build break on other platform.

Comment 37

[Eric Rescorla (:ekr)]

Which patch has the MEdiaPipeline changes?

Comment 38

[Chiajung Hung [:chiajung]]

Attachment: Patch V5.2 - [Part3] WebRTC Media changes

I just noticed some typo when I gen last patch :p
Here it is.

Comment 39

[alberto.pastor]

Hey guys, thanks for the update. Now is compiling fine. How are you testing it? I tried it in B2G Desktop, using the http://mozilla.github.com/webrtc-landing/gum_test.html test and it doesn't seem to work. I compiled it for Unagi as well without luck. Any recommendation? Thanks!

Comment 40

[Eric Rescorla (:ekr)]

Comment on attachment 729336 [details] [diff] [review]
Patch V5.2 - [Part3] WebRTC Media changes

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

lgtm with minor issues below.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +529,5 @@
> +    case kVideoNV21:
> +      type = webrtc::kVideoNV21;
> +      break;
> +    default:
> +      CSFLogError(logTag,  "%s VideoType Invalid. Only 1420 Supported",__FUNCTION__);

This log message is no longer accurate.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +827,5 @@
> +    // OK, pass it on to the conduit
> +    MOZ_MTLOG(PR_LOG_DEBUG, "Sending a video frame");
> +    // Not much for us to do with an error
> +    conduit->SendVideoFrame(y, length, width, height, mozilla::kVideoI420, 0);
> +  } else if (format != PLANAR_YCBCR) {

You don't needs this if, since it's redundant with the previous test.

Comment 41

[Chiajung Hung [:chiajung]]

Attachment: Patch V6 - [Part3] WebRTC Media changes

Update log and fix the if conditions.

Comment 42

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 727583 [details] [diff] [review]
Patch V5 - [Part2] WebRTC changes

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

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +78,5 @@
> +    , mNativeCameraControl(nullptr)
> +    , mCaptureIndex(aIndex)
> +    , mWindowId(aWindowId)
> +    , mWidth(640)
> +    , mHeight(480)

These should probably be symbolic constants; see MediaEngineWebRTC.h or MediaEngine.h; or they should come from preferences (and in the future from the app); see how resolution is now handled in MediaEngineWebRTC.

@@ +83,5 @@
> +    , mMonitor("WebRTCCamera.Monitor")
> +    , mCallbackMonitor("WebRTCCamera.CallbackMonitor")
> +    , mInitDone(false)
> +    , mInSnapshotMode(false)
> +    , mSnapshotPath(NULL)

nullptr

@@ +87,5 @@
> +    , mSnapshotPath(NULL)
> +    , mPreviewStream(nullptr)
> +  {
> +    mState = kReleased;
> +    NS_NewThread(getter_AddRefs(mCameraThread), nullptr);

NS_NewNamedThread, please.  It makes debugging easier

@@ +309,5 @@
>  public:
> +  MediaEngineWebRTC(uint64_t aWindowId = 0)
> +    : mMutex("mozilla::MediaEngineWebRTC")
> +    , mVideoEngine(NULL)
> +    , mVoiceEngine(NULL)

nullptr (I realize these already were NULL; please fix it).

Comment 43

[Eric Rescorla (:ekr)]

Comment on attachment 729877 [details] [diff] [review]
Patch V6 - [Part3] WebRTC Media changes

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

lgtm

Comment 44

[Chiajung Hung [:chiajung]]

Attachment: Patch V6 - [Part2] WebRTC changes

This one fix various style problem in last version.

Comment 45

[Chiajung Hung [:chiajung]]

Attachment: Patch V6.1 - [Part2] WebRTC changes

Forget width/height handling in last one

Comment 46

[Chiajung Hung [:chiajung]]

Attachment: Patch V6.1 - [Part3] WebRTC Media changes

I found my build environment got some problem and clean up objdir then found some mistake in previous patch.

Sorry for that.

Comment 47

[Chiajung Hung [:chiajung]]

Attachment: Patch V6.2 - [Part3] WebRTC Media changes

WebRTC part

Comment 48

[Chiajung Hung [:chiajung]]

Attachment: Patch V7 - [Part2] WebRTC changes

Argh...I found I forget 2 files in previous patch...

Comment 49

[Chiajung Hung [:chiajung]]

Attachment: Patch V6.3 - [Part3] WebRTC Media changes

Regenerate with hg diff -U8

Comment 50

[Chiajung Hung [:chiajung]]

Attachment: Patch V7.1 - [Part2] WebRTC changes

Regeneration patch with hg diff -U8 and rebase to newest mozilla-central codebase.

Comment 51

[Chiajung Hung [:chiajung]]

Attachment: Patch V4.2 - [Part1] Camera changes

Update camera part to exclude MediaManager which should belongs to part 2

Comment 52

[Chiajung Hung [:chiajung]]

Attachment: Patch V4.2 - [Part1] Camera changes

obsolete older patchs.

Comment 53

[Chiajung Hung [:chiajung]]

@Mike

After r126171, this will crash in cyclecollector when dereference DOMCameraManager...
I think I can workaround it tomorrow. And we may need some discussion on it.

Comment 54

[Shian-Yow Wu [:swu]]

(In reply to Chiajung Hung [:chiajung] from comment #53)
> @Mike
> 
> After r126171, this will crash in cyclecollector when dereference
> DOMCameraManager...
> I think I can workaround it tomorrow. And we may need some discussion on it.

I think you are refering to r126170 (Bug 839025).

Comment 55

[Chiajung Hung [:chiajung]]

(In reply to Chiajung Hung [:chiajung] from comment #53)
> @Mike
> 
> After r126171, this will crash in cyclecollector when dereference
                                                        decrease reference count of
> DOMCameraManager...
> I think I can workaround it tomorrow. And we may need some discussion on it.

Basically, if I pass nsRefPtr<DOMCameraManager> in argument, it will crash on leave.

Comment 56

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

Hmm, I get a 404 on both r126170 and r126171.

Comment 57

[Chiajung Hung [:chiajung]]

Though I can workaround for CycleCollector change by use raw pointer...The change in r125614 slient the MediaStreamListener::NotifyQueuedTrackChanges callback, and since the VideoFrameContainer strongly binded with HTMLElement, I can only get preview frame from MediaStreamListener originally...I will find some other way to make things work again.

Comment 58

[Chiajung Hung [:chiajung]]

Attachment: Patch V5 - [Part1] Camera Changes

Add a new callback into CameraPreviewMediaStream to compensate for MediaStreamListener::NotifyQueuedTrackChange.

Comment 59

[Chiajung Hung [:chiajung]]

Attachment: Patch V8 - [Part2] WebRTC changes

Add a workaround for avoid crash on smart point manipulation in MediaThread.

Comment 60

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 730485 [details] [diff] [review]
Patch V8 - [Part2] WebRTC changes

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

r- to resolve DOM/threadsafety issues

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +176,5 @@
> +#ifdef MOZ_WIDGET_GONK
> +  nsRefPtr<nsDOMCameraManager> mCameraManager;
> +  nsRefPtr<nsDOMCameraControl> mDOMCameraControl;
> +  nsRefPtr<nsGonkCameraControl> mNativeCameraControl;
> +  nsRefPtr<DOMCameraPreview> mPreviewStream;

Please verify these classes are threadsafe, or only used on this thread.  Per IRC, DOMCameraControl likely isn't.

@@ +180,5 @@
> +  nsRefPtr<DOMCameraPreview> mPreviewStream;
> +  uint64_t mWindowId;
> +  mozilla::ReentrantMonitor mCallbackMonitor; // Monitor for camera callback handling
> +  nsRefPtr<nsIThread> mCameraThread;
> +  nsRefPtr<nsIDOMBlob> mLastCapture;

check this too

@@ +316,5 @@
>      mVideoSources.Init();
>      mAudioSources.Init();
> +#ifdef MOZ_WIDGET_GONK
> +    nsPIDOMWindow* window = static_cast<nsPIDOMWindow*>(nsGlobalWindow::GetInnerWindowWithId(mWindowId));
> +    mCameraManager = nsDOMCameraManager::CheckPermissionAndCreateInstance(window);

I'm not certain you can manipulate the window (or call GetInnerWindow) from non-MainThread.  You may need to do all this before sending the runnable to this thread.  See how most of this is done from MediaManager.

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +228,5 @@
> +  if (mState == kReleased && mInitDone) {
> +    ReentrantMonitorAutoEnter sync(mCallbackMonitor);
> +    ChooseCapability(aPrefs);
> +    mDOMCameraControl = new nsDOMCameraControl(mCaptureIndex, mCameraThread, this, this, mWindowId);
> +    mCameraManager->Register(mDOMCameraControl);

If this really is a DOM object, this won't work.

@@ +432,5 @@
> +    return NS_ERROR_FAILURE;
> +
> +  // The camera return nsDOMMemoryFile indeed, and the inheritance tree is:
> +  // nsIDOMBlob <- nsIDOMFile <- nsDOMFileBase <- nsDOMFile <- nsDOMMemoryFile
> +  *aFile = static_cast<nsIDOMFile*>(mLastCapture.get());

This may need to be done on MainThread (or the conversion to a DOMFile at least).

@@ +525,3 @@
>  
> +  snprintf(mDeviceName, sizeof(mDeviceName), "%s", deviceName.get());
> +  snprintf(mUniqueId, sizeof(mUniqueId), "%s", deviceName.get());

PR_snprintf please. snprintf isn't always safe

@@ +613,5 @@
> +// nsICameraTakePictureCallback
> +nsresult
> +MediaEngineWebRTCVideoSource::HandleEvent(nsIDOMBlob* picture) {
> +  ReentrantMonitorAutoEnter sync(mCallbackMonitor);
> +  mLastCapture = picture;

DOMBlobs likely are mainthread only.

Comment 61

[Chiajung Hung [:chiajung]]

Attachment: Patch V9 - [Part2] WebRTC changes

Make all DOM objects (DOMCameraManager/DOMCameraControl) runs on MainThread.

Comment 62

[Chiajung Hung [:chiajung]]

> 
> @@ +613,5 @@
> > +// nsICameraTakePictureCallback
> > +nsresult
> > +MediaEngineWebRTCVideoSource::HandleEvent(nsIDOMBlob* picture) {
> > +  ReentrantMonitorAutoEnter sync(mCallbackMonitor);
> > +  mLastCapture = picture;
> 
> DOMBlobs likely are mainthread only.

All such CameraCallbacks are called from MainThread. 
(That's why I have to make sure MediaEngineWebRTCVideoSource operations does not run on MainThread for B2G and use Monitor to block MediaThread to wait camera operation done)

Comment 63

[Eric Rescorla (:ekr)]

Comment on attachment 730064 [details] [diff] [review]
Patch V6.3 - [Part3] WebRTC Media changes

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

lgtm

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +786,5 @@
>  
> +    android::sp<android::GraphicBuffer> graphicBuffer = layers::GrallocBufferActor::GetFrom(grallocHandle);
> +    void *basePtr;
> +    graphicBuffer->lock(android::GraphicBuffer::USAGE_SW_READ_MASK, &basePtr);
> +    conduit->SendVideoFrame(static_cast<unsigned char*>(basePtr),

I would suggest an assert for these values being even.

Comment 64

[Jason Smith [:jsmith]]

No longer a meta bug since we're doing work here.

Comment 65

[Chiajung Hung [:chiajung]]

Attachment: Patch V10 - [Part2] WebRTC changes

Fix build break on other platform

Comment 66

[Chiajung Hung [:chiajung]]

Attachment: Patch V10.1 - [Part2] WebRTC changes

Add a missing change.

Comment 67

[Chiajung Hung [:chiajung]]

Attachment: Patch V10.2 - [Part2] WebRTC changes

Avoid race condition which may crash on deallocation.

Comment 68

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 735051 [details] [diff] [review]
Patch V10.2 - [Part2] WebRTC changes

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

r- for the if (.... || 1) which I think is accidental.
If that's just removed, then r+ with nits fixed

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +24,5 @@
>   */
>  NS_IMPL_THREADSAFE_ISUPPORTS1(MediaEngineWebRTCVideoSource, nsIRunnable)
>  
>  // ViEExternalRenderer Callback.
> +#ifndef MOZ_WIDGET_GONK

The FrameSize can't change in GONK?  Rotation can't change it?  Shouldn't we at least leave a stub that logs that we received it?

Looking at the end, it seems this is replaced in Gonk by OnNewFrame(); if so comment it here.

@@ +274,5 @@
> +    mNativeCameraControl->ReleaseHardware(this, this);
> +    mNativeCameraControl = nullptr;
> +    mCallbackMonitor.Wait();
> +    if (mState != kReleased)
> +      return NS_ERROR_FAILURE;

braces

@@ +330,5 @@
> +  size.height = mCapability.height;
> +  mNativeCameraControl->GetPreviewStream(size, this, this);
> +  mCallbackMonitor.Wait();
> +  if (mState != kStarted)
> +    return NS_ERROR_FAILURE;

braces

@@ +637,5 @@
> +MediaEngineWebRTCVideoSource::OnNewFrame(const gfxIntSize& aIntrinsicSize, layers::Image* aImage) {
> +  MonitorAutoLock enter(mMonitor);
> +  mImage = aImage;
> +  mWidth = aIntrinsicSize.width;
> +  mHeight = aIntrinsicSize.height;

Consider logging resolution changes

::: dom/media/MediaManager.cpp
@@ +1067,5 @@
>      return NS_OK;
>    }
>  #endif
>    // XXX No full support for picture in Desktop yet (needs proper UI)
> +  if (aPrivileged || fake || 1) {

This is wrong - debug code?

Comment 69

[Chiajung Hung [:chiajung]]

Attachment: Patch V10.3 - [Part2] WebRTC changes

Remove a debug code, and fix some style problem.

Comment 70

[Chiajung Hung [:chiajung]]

Attachment: Patch V10.4 - [Part2] WebRTC changes

Fix a small typo.

Comment 71

[Chiajung Hung [:chiajung]]

Attachment: Patch V11 - [Part2] WebRTC changes

After some more detailed try, I found the takePicture function got some problem and decided to carry them into this patch :)

I found some property in current Camera HAL:
1. If takePicture called without startPreview first
  (1) It may cause mediaserver SIGSEGV 
  (2) It may just deadlock somewhere in camera HAL
  (3) It may callback without problem, but the data is null

2. If I startPreview first, then takePicture runs well

So I just call startPreview before takePicture. And since our MediaManager forbid {video: true, picture: true} case, it should be safe to do that everytime we need takePicture.

Comment 72

[u459114]

Hi ben,
Are you checking this issue?
SIGSEGV while calls takePicture befire startPreview.

Comment 73

[Chiajung Hung [:chiajung]]

Attachment: Patch V11.1 - [Part2] WebRTC changes

Fix a bug when takePicture first then startPreview fail.

Comment 74

[Chiajung Hung [:chiajung]]

Attachment: Patch V11.2 - [Part2] WebRTC changes

Avoid one more cycle collector crash.

Comment 75

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 741657 [details] [diff] [review]
Patch V11.2 - [Part2] WebRTC changes

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

I'll provisionally r+ based on fixing any C-string vs UTF8-string issues, addressing nits, and verifying that the mCameraManager calls are threadsafe (and TSAN data-race safe).  If you're concerned about any changes in response to my comments need re-review, likely they'll be generic XPCOM/etc issues, so you can generate a delta patch (on top of this one) for other people to review - most any review peer could do that.

::: content/media/webrtc/MediaEngineWebRTC.cpp
@@ +56,5 @@
> +   * mVideoSources must be updated.
> +   */
> +  int num = 0;
> +  nsresult result;
> +  result = mCameraManager->GetNumberOfCameras(num);

Are the calls to CameraManager threadsafe?  OR are they only called from this thread, and only access data set from this thread?

@@ +62,5 @@
> +    return;
> +  }
> +
> +  for (int i = 0; i < num; i++) {
> +    nsCString cameraName;

Is this a C string, or UTF8?

@@ +64,5 @@
> +
> +  for (int i = 0; i < num; i++) {
> +    nsCString cameraName;
> +    result = mCameraManager->GetCameraName(i, cameraName);
> +

remove the blank line

@@ -214,5 @@
>  
>      int error = ptrVoEHw->GetRecordingDeviceName(i, deviceName, uniqueId);
>      if (error) {
> -      LOG((" VoEHardware:GetRecordingDeviceName: Failed %d",
> -           ptrVoEBase->LastError() ));

Is there a reason these log messages are removed?

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +185,5 @@
>    void Shutdown();
>  
>    // Engine variables.
> +#ifdef MOZ_WIDGET_GONK
> +  nsDOMCameraManager* mCameraManager; // This DOM-object is passed in from WebRTCMediaEngine

Please comment here: say why this is a raw pointer, and why it's safe to do this (i.e. what's keeping mCameraManager alive).

@@ +368,5 @@
>    nsRefPtrHashtable<nsStringHashKey, MediaEngineWebRTCAudioSource > mAudioSources;
> +
> +#ifdef MOZ_WIDGET_GONK
> +  uint64_t mWindowId;
> +  nsDOMCameraManager* mCameraManager;

Comment why it's a raw pointer

@@ +408,5 @@
> +    nsCString deviceName;
> +    mVideoSource->mCameraManager->GetCameraName(mVideoSource->mCaptureIndex, deviceName);
> +
> +    PR_snprintf(mVideoSource->mDeviceName, sizeof(mVideoSource->mDeviceName), "%s", deviceName.get());
> +    PR_snprintf(mVideoSource->mUniqueId, sizeof(mVideoSource->mUniqueId), "%s", deviceName.get());

mDeviceName/mUniqueId are UTF8 arrays, not C strings - though this likely works due to the values returned by GetCameraName.  Please convert to UTF8 (if GetCameraName doesn't support that already); and there's likely a better way to assign it than PR_snprintf() (which with UTF8 could leave it improperly terminated in instances)

Comment 76

[Randell Jesup [:jesup] (needinfo me)]

BTW, please file a followup bug for fixing the nsDOMCameraManager usage, unless that followup is bug 853000.

Please include the number in the comments I asked for about why a raw pointer is being used.

Comment 77

[Chiajung Hung [:chiajung]]

bug 853000 is about to separate the Camera API to a clear native part and a DOM object part, which would fix the CycleCollector crash from the root.

Comment 78

[Chiajung Hung [:chiajung]]

(In reply to Randell Jesup [:jesup] from comment #75)
> Comment on attachment 741657 [details] [diff] [review]
> Patch V11.2 - [Part2] WebRTC changes
> 
> Review of attachment 741657 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'll provisionally r+ based on fixing any C-string vs UTF8-string issues,
> addressing nits, and verifying that the mCameraManager calls are threadsafe
> (and TSAN data-race safe).  If you're concerned about any changes in
> response to my comments need re-review, likely they'll be generic XPCOM/etc
> issues, so you can generate a delta patch (on top of this one) for other
> people to review - most any review peer could do that.
> 
> ::: content/media/webrtc/MediaEngineWebRTC.cpp
> @@ +56,5 @@
> > +   * mVideoSources must be updated.
> > +   */
> > +  int num = 0;
> > +  nsresult result;
> > +  result = mCameraManager->GetNumberOfCameras(num);
> 
> Are the calls to CameraManager threadsafe?  OR are they only called from
> this thread, and only access data set from this thread?
CameraManager call like GetNumberOfCameras and GetCameraName does not access any member data in fact, they are thread-safe I think :)

CameraManager::Register can only called from main thread and all the data need to be protected are only access from main thread.

> 
> @@ +62,5 @@
> > +    return;
> > +  }
> > +
> > +  for (int i = 0; i < num; i++) {
> > +    nsCString cameraName;
> 
> Is this a C string, or UTF8?
It's a C string.

> 
> @@ +64,5 @@
> > +
> > +  for (int i = 0; i < num; i++) {
> > +    nsCString cameraName;
> > +    result = mCameraManager->GetCameraName(i, cameraName);
> > +
> 
> remove the blank line
done
> 
> @@ -214,5 @@
> >  
> >      int error = ptrVoEHw->GetRecordingDeviceName(i, deviceName, uniqueId);
> >      if (error) {
> > -      LOG((" VoEHardware:GetRecordingDeviceName: Failed %d",
> > -           ptrVoEBase->LastError() ));
> 
> Is there a reason these log messages are removed?
They cause macro expansion error originally, I will re-add them and reordered the inclusion sequence and fix it.
> 
> ::: content/media/webrtc/MediaEngineWebRTC.h
> @@ +185,5 @@
> >    void Shutdown();
> >  
> >    // Engine variables.
> > +#ifdef MOZ_WIDGET_GONK
> > +  nsDOMCameraManager* mCameraManager; // This DOM-object is passed in from WebRTCMediaEngine
> 
> Please comment here: say why this is a raw pointer, and why it's safe to do
> this (i.e. what's keeping mCameraManager alive).
done
> 
> @@ +368,5 @@
> >    nsRefPtrHashtable<nsStringHashKey, MediaEngineWebRTCAudioSource > mAudioSources;
> > +
> > +#ifdef MOZ_WIDGET_GONK
> > +  uint64_t mWindowId;
> > +  nsDOMCameraManager* mCameraManager;
> 
> Comment why it's a raw pointer
done.
> 
> @@ +408,5 @@
> > +    nsCString deviceName;
> > +    mVideoSource->mCameraManager->GetCameraName(mVideoSource->mCaptureIndex, deviceName);
> > +
> > +    PR_snprintf(mVideoSource->mDeviceName, sizeof(mVideoSource->mDeviceName), "%s", deviceName.get());
> > +    PR_snprintf(mVideoSource->mUniqueId, sizeof(mVideoSource->mUniqueId), "%s", deviceName.get());
> 
> mDeviceName/mUniqueId are UTF8 arrays, not C strings - though this likely
> works due to the values returned by GetCameraName.  Please convert to UTF8
> (if GetCameraName doesn't support that already); and there's likely a better
> way to assign it than PR_snprintf() (which with UTF8 could leave it
> improperly terminated in instances)
done.

As Bas report some more issue in debug build, I will also try it and upload a newer version.

All camera related function *must* be called from MainThread now, since camera API now wrap the callbacks with nsMainThreadPtrHolder<T>, I changed all such call to Runnable, and implement all such runnable in other way. I think the change is relative big and it is safer to reviewed again.

Comment 79

[Chiajung Hung [:chiajung]]

Attachment: Patch V11.3 - [Part2] WebRTC changes

The logic is basically the same, main change:

- Avoid call CameraControl functions from MediaThread, which cause crash on DEBUG version
- Add proper comment about the raw pointer
- Fix UTF8 string usage

Though the logic is the same, I think it is better to be reviewed again.

Comment 80

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 742964 [details] [diff] [review]
Patch V11.3 - [Part2] WebRTC changes

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

Please use a consistent 8-line context for diffs; the previous patch used them, this one uses I think 4 (the default).  You can set it in .hgrc.  The change makes it hard to compare the diffs.

r- for the mState/etc access from callbacks; Either proxy those to the right thread, or add locks to every use use of mState.

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +524,4 @@
>  
> +  nsString deviceNameUTF16;
> +  deviceNameUTF16.AssignASCII(deviceName.get());
> +  char* UTF8Name = ToNewUTF8String(deviceNameUTF16);

How about something like: nsAutoPtr<char> UTF8Name(ToNewUTF8String(deviceNameUTF16));

And remove the delete.  It's a a better style, though in this case it's equally correct.

@@ +589,5 @@
>  }
>  
> +#ifdef MOZ_WIDGET_GONK
> +void
> +MediaEngineWebRTCVideoSource::AllocImpl() {

Please add a comment here about how these functions all run on MainThread (and consider adding mainthread assertions).

@@ +608,5 @@
> +void
> +MediaEngineWebRTCVideoSource::StartImpl() {
> +  idl::CameraSize size;
> +  size.width = mCapability.width;
> +  size.height = mCapability.height;

Please document which member vars are touched from which thread, which will help us determine which ones need locking to access and any potential data races.  If locking would be needed here, for example, the caller could pass width and height as parameters via WrapRunnable.  You have to be careful with this, as these as async messages with no synchronous resolution (since you can't safely DISPATCH_SYNC *to* the MainThread.

@@ +651,5 @@
> +  mPreviewStream = static_cast<DOMCameraPreview*>(stream);
> +  mPreviewStream->Start();
> +  CameraPreviewMediaStream* cameraStream = static_cast<CameraPreviewMediaStream*>(mPreviewStream->GetStream());
> +  cameraStream->SetFrameCallback(this);
> +  mState = kStarted;

What thread does this (and the other) callbacks occur?  If not on the MediaManager/MediaEngine thread (and I assume it's not, given the use of the Reentrant Monito), locking will be needed around access to the member vars touched here everywhere else that touches them, or we'll need to proxy the callback back to the MediaManager/MediaEngine thread to make these modifications.

For example, this implies that *all* things touching mState (to read or write it) must do so under the ReentrantMonitor

Comment 81

[Eric Rescorla (:ekr)]

Comment on attachment 742964 [details] [diff] [review]
Patch V11.3 - [Part2] WebRTC changes

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

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +524,4 @@
>  
> +  nsString deviceNameUTF16;
> +  deviceNameUTF16.AssignASCII(deviceName.get());
> +  char* UTF8Name = ToNewUTF8String(deviceNameUTF16);

Is that in fact correct? The documentation suggests that we should call NS_Free() and not delete:

These methods return a buffer allocated using XPCOM's allocator instead of the traditional allocator (malloc, etc.). You should use NS_Free to deallocate the result when you no longer need it.

https://developer.mozilla.org/en-US/docs/Mozilla_internal_string_guide

Comment 82

[Chiajung Hung [:chiajung]]

(In reply to Randell Jesup [:jesup] from comment #80)
> Comment on attachment 742964 [details] [diff] [review]
> Patch V11.3 - [Part2] WebRTC changes
> 
> Review of attachment 742964 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please use a consistent 8-line context for diffs; the previous patch used
> them, this one uses I think 4 (the default).  You can set it in .hgrc.  The
> change makes it hard to compare the diffs.
Do you mean add this section into .hgrc?

[diff]
git = 1
showfunc = 1
unified = 8 
> 
> r- for the mState/etc access from callbacks; Either proxy those to the right
> thread, or add locks to every use use of mState.
> 
> ::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
> @@ +524,4 @@
> >  
> > +  nsString deviceNameUTF16;
> > +  deviceNameUTF16.AssignASCII(deviceName.get());
> > +  char* UTF8Name = ToNewUTF8String(deviceNameUTF16);
> 
> How about something like: nsAutoPtr<char>
> UTF8Name(ToNewUTF8String(deviceNameUTF16));
> 
> And remove the delete.  It's a a better style, though in this case it's
> equally correct.
Following comment 81, since NS_Free is more accurate base on document.
> 
> @@ +589,5 @@
> >  }
> >  
> > +#ifdef MOZ_WIDGET_GONK
> > +void
> > +MediaEngineWebRTCVideoSource::AllocImpl() {
> 
> Please add a comment here about how these functions all run on MainThread
> (and consider adding mainthread assertions).
Comment and ASSERT added.
> 
> @@ +608,5 @@
> > +void
> > +MediaEngineWebRTCVideoSource::StartImpl() {
> > +  idl::CameraSize size;
> > +  size.width = mCapability.width;
> > +  size.height = mCapability.height;
> 
> Please document which member vars are touched from which thread, which will
> help us determine which ones need locking to access and any potential data
> races.  If locking would be needed here, for example, the caller could pass
> width and height as parameters via WrapRunnable.  You have to be careful
> with this, as these as async messages with no synchronous resolution (since
> you can't safely DISPATCH_SYNC *to* the MainThread.
MediaThread:
  NotifyPull: mState, mImage, mWidth, mHeight
  ChooseCapability: mCapability, mPrefs
  GetName: mDeviceName
  GetUUID: mUniqueId
  Allocate: mState, mInitDone
  Deallocate: mSources, mState
  Start: mInitDone, mSources, mImageContainer, mStartTime
  Stop: mSources, mState, mImage
  Snapshot: mInitDone, mState, mLastCapture
  Init: mDeviceName, mUniqueId, mInitDone
  ShutDown: mInitDone, mState

MainThread:
  AllocImpl: mDOMCameraControl, mCaptureIndex, mCameraThread, mWindowId, mCameraManager
  DeallocImpl: mNativeCameraControl
  StartImpl: mCapability, mNativeCameraControl
  StopImpl: mNativeCameraControl, mPreviewStream
  SnapshotImpl: mCapability, mNativeCameraControl
  CameraEvents: mState, mPreviewStream, mLastCapture, mWidth, mHeight

mWidth, mHeight, mImage are protected by mMonitor now.
I will protect mState by mCallbackMonitor in next patch.
And mCapability would become a parameter passed to StartImpl.
> 
> @@ +651,5 @@
> > +  mPreviewStream = static_cast<DOMCameraPreview*>(stream);
> > +  mPreviewStream->Start();
> > +  CameraPreviewMediaStream* cameraStream = static_cast<CameraPreviewMediaStream*>(mPreviewStream->GetStream());
> > +  cameraStream->SetFrameCallback(this);
> > +  mState = kStarted;
> 
> What thread does this (and the other) callbacks occur?  If not on the
> MediaManager/MediaEngine thread (and I assume it's not, given the use of the
> Reentrant Monito), locking will be needed around access to the member vars
> touched here everywhere else that touches them, or we'll need to proxy the
> callback back to the MediaManager/MediaEngine thread to make these
> modifications.
> 
> For example, this implies that *all* things touching mState (to read or
> write it) must do so under the ReentrantMonitor
Frame callback are running on Camera Preview Thread. All other Camera callbacks are running on Main Thread.

Comment 83

[Chiajung Hung [:chiajung]]

Attachment: Patch V11.4 - [Part2] WebRTC changes

Fix potential multithreading problem. Thanks for detailed review!!!

Comment 84

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 743469 [details] [diff] [review]
Patch V11.4 - [Part2] WebRTC changes

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

Please document in the .h file which member vars are accessed from what thread, and if from more than one thread, which lock is used to lock access.  If possible, re-order the member vars to match the locking constraints, but this isn't always possible.  Note this may require re-ordering member-var initial settings in the constructor to match or it will break Mac builds.

With these nits fixed, r+

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +617,5 @@
>  }
> +
> +void
> +MediaEngineWebRTCVideoSource::DeallocImpl() {
> +  // All these functions must be run on MainThread!

Just say that once after #ifdef MOZ_WIDGET_GONK

Comment 85

[Chiajung Hung [:chiajung]]

Attachment: Patch V5 - [Part1] Camera changes

Rebase to newest version. Carry r+

Comment 86

[Chiajung Hung [:chiajung]]

Attachment: Patch V6.3 - [Part3] WebRTC Media changes

Rebased to newest code base. Carry r+

Comment 87

[Chiajung Hung [:chiajung]]

Attachment: Patch V11.4 - [Part2] WebRTC changes

Rebase to newest codebase. Carry r+

Comment 88

[Chiajung Hung [:chiajung]]

Try server result:

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

Comment 89

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/projects/birch/rev/88b09f034e9f
https://hg.mozilla.org/projects/birch/rev/5bce97a3fa8e
https://hg.mozilla.org/projects/birch/rev/79c82a501ad0

FWIW, the patches you labeled as part 2 and part 3 were flipped around in their commit messages. I applied them in the order of the commit messages.

Comment 90

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/88b09f034e9f
https://hg.mozilla.org/mozilla-central/rev/5bce97a3fa8e
https://hg.mozilla.org/mozilla-central/rev/79c82a501ad0