Closed
Bug 1129051
Opened 10 years ago
Closed 10 years ago
[B2G][Camera] Multiple camera control listeners cause double free for take picture callbacks
Categories
(Firefox OS Graveyard :: Gaia::Camera, defect)
Tracking
(firefox40 fixed)
RESOLVED
FIXED
2.2 S11 (1may)
| Tracking | Status | |
|---|---|---|
| firefox40 | --- | fixed |
People
(Reporter: aosmond, Assigned: royang51)
References
Details
Attachments
(1 file, 4 obsolete files)
|
9.82 KB,
patch
|
aosmond
:
review+
|
Details | Diff | Splinter Review |
Latent issue (not presently triggered) in camera control code:
nsGonkCameraControl::OnTakePictureComplete copies the picture blob data into new buffer
CameraControlImpl::OnTakePictureComplete forwards buffer to listeners
DOMCameraControlListener::OnTakePictureComplete posts to the main thread and wraps the buffer via File::CreateMemoryFile
The copy should happen for each listener instead of once in nsGonkCameraControl, because File::CreateMemoryFile assumes that it owns the buffer pointer and will free it when it goes out of scope.
|
Reporter | |
Updated•10 years ago
|
Assignee: nobody → aosmond
Blocks: camera-backlog
Status: NEW → ASSIGNED
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
|
|
Reporter | |
Updated•10 years ago
|
Assignee: aosmond → royang51
Attachment #8597730 -
Flags: review?(aosmond)
Attachment #8597732 -
Flags: review?(aosmond)
Attachment #8597730 -
Attachment is obsolete: true
Attachment #8597730 -
Flags: review?(aosmond)
Attachment #8597732 -
Attachment is obsolete: true
Attachment #8597732 -
Flags: review?(aosmond)
Attachment #8597733 -
Flags: review?(aosmond)
|
|
Reporter | |
Comment 4•10 years ago
|
||
Comment on attachment 8597733 [details] [diff] [review]
bug1129051.patch, V2
Review of attachment 8597733 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good to me!
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8af61ffd252a
Attachment #8597733 -
Flags: review?(aosmond) → review+
|
|
Reporter | |
Comment 5•10 years ago
|
||
royang, could you update the patch with a description? See one of the patches in bug 994912 as an example. It should look something like:
> Bug XXX - Description. r=aosmond
>
> diff --git ...
> ...
Also please request Commit Access (Level 1) so that you can push to the try server yourself. Follow the instructions at https://www.mozilla.org/en-US/about/governance/policies/commit/. Add a needinfo for me and I will vouch for you.
Comment on attachment 8597733 [details] [diff] [review]
bug1129051.patch, V2
From: Roger Yang <royang51@gmail.com>
Bug 1129051 - Fix Camera Control Listener double free. Fix webrtc memory leak.
>diff --git a/dom/camera/CameraControlListener.h b/dom/camera/CameraControlListener.h
>index 532fdaf..e489c11 100644
>--- a/dom/camera/CameraControlListener.h
>+++ b/dom/camera/CameraControlListener.h
>@@ -84,17 +84,17 @@ public:
> public:
> uint32_t mMaxMeteringAreas;
> uint32_t mMaxFocusAreas;
> };
> virtual void OnConfigurationChange(const CameraListenerConfiguration& aConfiguration) { }
>
> virtual void OnAutoFocusComplete(bool aAutoFocusSucceeded) { }
> virtual void OnAutoFocusMoving(bool aIsMoving) { }
>- virtual void OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) { }
>+ virtual void OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) { }
> virtual void OnFacesDetected(const nsTArray<ICameraControl::Face>& aFaces) { }
>
> enum UserContext
> {
> kInStartCamera,
> kInStopCamera,
> kInAutoFocus,
> kInStartFaceDetection,
>diff --git a/dom/camera/DOMCameraControlListener.cpp b/dom/camera/DOMCameraControlListener.cpp
>index 96f55ab..7a586c4 100644
>--- a/dom/camera/DOMCameraControlListener.cpp
>+++ b/dom/camera/DOMCameraControlListener.cpp
>@@ -336,28 +336,30 @@ DOMCameraControlListener::OnAutoFocusComplete(bool aAutoFocusSucceeded)
> protected:
> bool mAutoFocusSucceeded;
> };
>
> NS_DispatchToMainThread(new Callback(mDOMCameraControl, aAutoFocusSucceeded));
> }
>
> void
>-DOMCameraControlListener::OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType)
>+DOMCameraControlListener::OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType)
> {
> class Callback : public DOMCallback
> {
> public:
> Callback(nsMainThreadPtrHandle<nsISupports> aDOMCameraControl,
>- uint8_t* aData, uint32_t aLength, const nsAString& aMimeType)
>+ const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType)
> : DOMCallback(aDOMCameraControl)
>- , mData(aData)
> , mLength(aLength)
> , mMimeType(aMimeType)
>- { }
>+ {
>+ mData = (uint8_t*) malloc(aLength);
>+ memcpy(mData, aData, aLength);
>+ }
>
> void
> RunCallback(nsDOMCameraControl* aDOMCameraControl) override
> {
> nsCOMPtr<nsIDOMBlob> picture =
> File::CreateMemoryFile(mDOMCameraControl.get(),
> static_cast<void*>(mData),
> static_cast<uint64_t>(mLength),
>diff --git a/dom/camera/DOMCameraControlListener.h b/dom/camera/DOMCameraControlListener.h
>index a5c30c6..c1a1be8 100644
>--- a/dom/camera/DOMCameraControlListener.h
>+++ b/dom/camera/DOMCameraControlListener.h
>@@ -16,17 +16,17 @@ class CameraPreviewMediaStream;
> class DOMCameraControlListener : public CameraControlListener
> {
> public:
> DOMCameraControlListener(nsDOMCameraControl* aDOMCameraControl, CameraPreviewMediaStream* aStream);
>
> virtual void OnAutoFocusComplete(bool aAutoFocusSucceeded) override;
> virtual void OnAutoFocusMoving(bool aIsMoving) override;
> virtual void OnFacesDetected(const nsTArray<ICameraControl::Face>& aFaces) override;
>- virtual void OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) override;
>+ virtual void OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) override;
>
> virtual void OnHardwareStateChange(HardwareState aState, nsresult aReason) override;
> virtual void OnPreviewStateChange(PreviewState aState) override;
> virtual void OnRecorderStateChange(RecorderState aState, int32_t aStatus, int32_t aTrackNum) override;
> virtual void OnConfigurationChange(const CameraListenerConfiguration& aConfiguration) override;
> virtual void OnShutter() override;
> virtual void OnRateLimitPreview(bool aLimit) override;
> virtual bool OnNewPreviewFrame(layers::Image* aImage, uint32_t aWidth, uint32_t aHeight) override;
>diff --git a/dom/camera/GonkCameraControl.cpp b/dom/camera/GonkCameraControl.cpp
>index 3e1eee7..41a15e5 100644
>--- a/dom/camera/GonkCameraControl.cpp
>+++ b/dom/camera/GonkCameraControl.cpp
>@@ -1529,24 +1529,20 @@ nsGonkCameraControl::OnFacesDetected(camera_frame_metadata_t* aMetaData)
> CameraControlImpl::OnFacesDetected(faces);
> }
>
> void
> nsGonkCameraControl::OnTakePictureComplete(uint8_t* aData, uint32_t aLength)
> {
> ReentrantMonitorAutoEnter mon(mReentrantMonitor);
>
>- uint8_t* data = new uint8_t[aLength];
>-
>- memcpy(data, aData, aLength);
>-
> nsString s(NS_LITERAL_STRING("image/"));
> s.Append(mFileFormat);
> DOM_CAMERA_LOGI("Got picture, type '%s', %u bytes\n", NS_ConvertUTF16toUTF8(s).get(), aLength);
>- OnTakePictureComplete(data, aLength, s);
>+ OnTakePictureComplete(aData, aLength, s);
>
> if (mResumePreviewAfterTakingPicture) {
> nsresult rv = StartPreview();
> if (NS_FAILED(rv)) {
> DOM_CAMERA_LOGE("Failed to restart camera preview (%x)\n", rv);
> OnPreviewStateChange(CameraControlListener::kPreviewStopped);
> }
> }
>diff --git a/dom/media/webrtc/MediaEngineGonkVideoSource.cpp b/dom/media/webrtc/MediaEngineGonkVideoSource.cpp
>index 3243807..141e60b 100644
>--- a/dom/media/webrtc/MediaEngineGonkVideoSource.cpp
>+++ b/dom/media/webrtc/MediaEngineGonkVideoSource.cpp
>@@ -585,28 +585,28 @@ MediaEngineGonkVideoSource::OnUserError(UserContext aContext, nsresult aError)
> MonitorAutoLock lock(mMonitor);
> if (mPhotoCallbacks.Length()) {
> NS_DispatchToMainThread(new TakePhotoError(mPhotoCallbacks, aError));
> }
> }
> }
>
> void
>-MediaEngineGonkVideoSource::OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType)
>+MediaEngineGonkVideoSource::OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType)
> {
> // It needs to start preview because Gonk camera will stop preview while
> // taking picture.
> mCameraControl->StartPreview();
>
> // Create a main thread runnable to generate a blob and call all current queued
> // PhotoCallbacks.
> class GenerateBlobRunnable : public nsRunnable {
> public:
> GenerateBlobRunnable(nsTArray<nsRefPtr<PhotoCallback>>& aCallbacks,
>- uint8_t* aData,
>+ const uint8_t* aData,
> uint32_t aLength,
> const nsAString& aMimeType)
> : mPhotoDataLength(aLength)
> {
> mCallbacks.SwapElements(aCallbacks);
> mPhotoData = (uint8_t*) malloc(aLength);
> memcpy(mPhotoData, aData, mPhotoDataLength);
> mMimeType = aMimeType;
>diff --git a/dom/media/webrtc/MediaEngineGonkVideoSource.h b/dom/media/webrtc/MediaEngineGonkVideoSource.h
>index 12d0372..b072f19 100644
>--- a/dom/media/webrtc/MediaEngineGonkVideoSource.h
>+++ b/dom/media/webrtc/MediaEngineGonkVideoSource.h
>@@ -69,17 +69,17 @@ public:
> SourceMediaStream* aSource,
> TrackID aId,
> StreamTime aDesiredTime) override;
>
> void OnHardwareStateChange(HardwareState aState, nsresult aReason) override;
> void GetRotation();
> bool OnNewPreviewFrame(layers::Image* aImage, uint32_t aWidth, uint32_t aHeight) override;
> void OnUserError(UserContext aContext, nsresult aError) override;
>- void OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) override;
>+ void OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) override;
>
> void AllocImpl();
> void DeallocImpl();
> void StartImpl(webrtc::CaptureCapability aCapability);
> void StopImpl();
> uint32_t ConvertPixelFormatToFOURCC(int aFormat);
> void RotateImage(layers::Image* aImage, uint32_t aWidth, uint32_t aHeight);
> void Notify(const mozilla::hal::ScreenConfiguration& aConfiguration);
Comment on attachment 8597733 [details] [diff] [review]
bug1129051.patch, V2
From: Roger Yang <royang51@gmail.com>
Bug 1129051 - Fix double free in Camera Control Listener. Fix webrtc memory leak.
>diff --git a/dom/camera/CameraControlListener.h b/dom/camera/CameraControlListener.h
>index 532fdaf..e489c11 100644
>--- a/dom/camera/CameraControlListener.h
>+++ b/dom/camera/CameraControlListener.h
>@@ -84,17 +84,17 @@ public:
> public:
> uint32_t mMaxMeteringAreas;
> uint32_t mMaxFocusAreas;
> };
> virtual void OnConfigurationChange(const CameraListenerConfiguration& aConfiguration) { }
>
> virtual void OnAutoFocusComplete(bool aAutoFocusSucceeded) { }
> virtual void OnAutoFocusMoving(bool aIsMoving) { }
>- virtual void OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) { }
>+ virtual void OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) { }
> virtual void OnFacesDetected(const nsTArray<ICameraControl::Face>& aFaces) { }
>
> enum UserContext
> {
> kInStartCamera,
> kInStopCamera,
> kInAutoFocus,
> kInStartFaceDetection,
>diff --git a/dom/camera/DOMCameraControlListener.cpp b/dom/camera/DOMCameraControlListener.cpp
>index 96f55ab..7a586c4 100644
>--- a/dom/camera/DOMCameraControlListener.cpp
>+++ b/dom/camera/DOMCameraControlListener.cpp
>@@ -336,28 +336,30 @@ DOMCameraControlListener::OnAutoFocusComplete(bool aAutoFocusSucceeded)
> protected:
> bool mAutoFocusSucceeded;
> };
>
> NS_DispatchToMainThread(new Callback(mDOMCameraControl, aAutoFocusSucceeded));
> }
>
> void
>-DOMCameraControlListener::OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType)
>+DOMCameraControlListener::OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType)
> {
> class Callback : public DOMCallback
> {
> public:
> Callback(nsMainThreadPtrHandle<nsISupports> aDOMCameraControl,
>- uint8_t* aData, uint32_t aLength, const nsAString& aMimeType)
>+ const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType)
> : DOMCallback(aDOMCameraControl)
>- , mData(aData)
> , mLength(aLength)
> , mMimeType(aMimeType)
>- { }
>+ {
>+ mData = (uint8_t*) malloc(aLength);
>+ memcpy(mData, aData, aLength);
>+ }
>
> void
> RunCallback(nsDOMCameraControl* aDOMCameraControl) override
> {
> nsCOMPtr<nsIDOMBlob> picture =
> File::CreateMemoryFile(mDOMCameraControl.get(),
> static_cast<void*>(mData),
> static_cast<uint64_t>(mLength),
>diff --git a/dom/camera/DOMCameraControlListener.h b/dom/camera/DOMCameraControlListener.h
>index a5c30c6..c1a1be8 100644
>--- a/dom/camera/DOMCameraControlListener.h
>+++ b/dom/camera/DOMCameraControlListener.h
>@@ -16,17 +16,17 @@ class CameraPreviewMediaStream;
> class DOMCameraControlListener : public CameraControlListener
> {
> public:
> DOMCameraControlListener(nsDOMCameraControl* aDOMCameraControl, CameraPreviewMediaStream* aStream);
>
> virtual void OnAutoFocusComplete(bool aAutoFocusSucceeded) override;
> virtual void OnAutoFocusMoving(bool aIsMoving) override;
> virtual void OnFacesDetected(const nsTArray<ICameraControl::Face>& aFaces) override;
>- virtual void OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) override;
>+ virtual void OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) override;
>
> virtual void OnHardwareStateChange(HardwareState aState, nsresult aReason) override;
> virtual void OnPreviewStateChange(PreviewState aState) override;
> virtual void OnRecorderStateChange(RecorderState aState, int32_t aStatus, int32_t aTrackNum) override;
> virtual void OnConfigurationChange(const CameraListenerConfiguration& aConfiguration) override;
> virtual void OnShutter() override;
> virtual void OnRateLimitPreview(bool aLimit) override;
> virtual bool OnNewPreviewFrame(layers::Image* aImage, uint32_t aWidth, uint32_t aHeight) override;
>diff --git a/dom/camera/GonkCameraControl.cpp b/dom/camera/GonkCameraControl.cpp
>index 3e1eee7..41a15e5 100644
>--- a/dom/camera/GonkCameraControl.cpp
>+++ b/dom/camera/GonkCameraControl.cpp
>@@ -1529,24 +1529,20 @@ nsGonkCameraControl::OnFacesDetected(camera_frame_metadata_t* aMetaData)
> CameraControlImpl::OnFacesDetected(faces);
> }
>
> void
> nsGonkCameraControl::OnTakePictureComplete(uint8_t* aData, uint32_t aLength)
> {
> ReentrantMonitorAutoEnter mon(mReentrantMonitor);
>
>- uint8_t* data = new uint8_t[aLength];
>-
>- memcpy(data, aData, aLength);
>-
> nsString s(NS_LITERAL_STRING("image/"));
> s.Append(mFileFormat);
> DOM_CAMERA_LOGI("Got picture, type '%s', %u bytes\n", NS_ConvertUTF16toUTF8(s).get(), aLength);
>- OnTakePictureComplete(data, aLength, s);
>+ OnTakePictureComplete(aData, aLength, s);
>
> if (mResumePreviewAfterTakingPicture) {
> nsresult rv = StartPreview();
> if (NS_FAILED(rv)) {
> DOM_CAMERA_LOGE("Failed to restart camera preview (%x)\n", rv);
> OnPreviewStateChange(CameraControlListener::kPreviewStopped);
> }
> }
>diff --git a/dom/media/webrtc/MediaEngineGonkVideoSource.cpp b/dom/media/webrtc/MediaEngineGonkVideoSource.cpp
>index 3243807..141e60b 100644
>--- a/dom/media/webrtc/MediaEngineGonkVideoSource.cpp
>+++ b/dom/media/webrtc/MediaEngineGonkVideoSource.cpp
>@@ -585,28 +585,28 @@ MediaEngineGonkVideoSource::OnUserError(UserContext aContext, nsresult aError)
> MonitorAutoLock lock(mMonitor);
> if (mPhotoCallbacks.Length()) {
> NS_DispatchToMainThread(new TakePhotoError(mPhotoCallbacks, aError));
> }
> }
> }
>
> void
>-MediaEngineGonkVideoSource::OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType)
>+MediaEngineGonkVideoSource::OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType)
> {
> // It needs to start preview because Gonk camera will stop preview while
> // taking picture.
> mCameraControl->StartPreview();
>
> // Create a main thread runnable to generate a blob and call all current queued
> // PhotoCallbacks.
> class GenerateBlobRunnable : public nsRunnable {
> public:
> GenerateBlobRunnable(nsTArray<nsRefPtr<PhotoCallback>>& aCallbacks,
>- uint8_t* aData,
>+ const uint8_t* aData,
> uint32_t aLength,
> const nsAString& aMimeType)
> : mPhotoDataLength(aLength)
> {
> mCallbacks.SwapElements(aCallbacks);
> mPhotoData = (uint8_t*) malloc(aLength);
> memcpy(mPhotoData, aData, mPhotoDataLength);
> mMimeType = aMimeType;
>diff --git a/dom/media/webrtc/MediaEngineGonkVideoSource.h b/dom/media/webrtc/MediaEngineGonkVideoSource.h
>index 12d0372..b072f19 100644
>--- a/dom/media/webrtc/MediaEngineGonkVideoSource.h
>+++ b/dom/media/webrtc/MediaEngineGonkVideoSource.h
>@@ -69,17 +69,17 @@ public:
> SourceMediaStream* aSource,
> TrackID aId,
> StreamTime aDesiredTime) override;
>
> void OnHardwareStateChange(HardwareState aState, nsresult aReason) override;
> void GetRotation();
> bool OnNewPreviewFrame(layers::Image* aImage, uint32_t aWidth, uint32_t aHeight) override;
> void OnUserError(UserContext aContext, nsresult aError) override;
>- void OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) override;
>+ void OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) override;
>
> void AllocImpl();
> void DeallocImpl();
> void StartImpl(webrtc::CaptureCapability aCapability);
> void StopImpl();
> uint32_t ConvertPixelFormatToFOURCC(int aFormat);
> void RotateImage(layers::Image* aImage, uint32_t aWidth, uint32_t aHeight);
> void Notify(const mozilla::hal::ScreenConfiguration& aConfiguration);
Comment on attachment 8597733 [details] [diff] [review]
bug1129051.patch, V2
>From: Roger Yang <royang51@gmail.com>
>
>Bug 1129051 - Fix Camera Control Listener double free. Fix webrtc memory leak r=royang51
>
>diff --git a/dom/camera/CameraControlListener.h b/dom/camera/CameraControlListener.h
>index 532fdaf..e489c11 100644
>--- a/dom/camera/CameraControlListener.h
>+++ b/dom/camera/CameraControlListener.h
>@@ -84,17 +84,17 @@ public:
> public:
> uint32_t mMaxMeteringAreas;
> uint32_t mMaxFocusAreas;
> };
> virtual void OnConfigurationChange(const CameraListenerConfiguration& aConfiguration) { }
>
> virtual void OnAutoFocusComplete(bool aAutoFocusSucceeded) { }
> virtual void OnAutoFocusMoving(bool aIsMoving) { }
>- virtual void OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) { }
>+ virtual void OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) { }
> virtual void OnFacesDetected(const nsTArray<ICameraControl::Face>& aFaces) { }
>
> enum UserContext
> {
> kInStartCamera,
> kInStopCamera,
> kInAutoFocus,
> kInStartFaceDetection,
>diff --git a/dom/camera/DOMCameraControlListener.cpp b/dom/camera/DOMCameraControlListener.cpp
>index 96f55ab..7a586c4 100644
>--- a/dom/camera/DOMCameraControlListener.cpp
>+++ b/dom/camera/DOMCameraControlListener.cpp
>@@ -336,28 +336,30 @@ DOMCameraControlListener::OnAutoFocusComplete(bool aAutoFocusSucceeded)
> protected:
> bool mAutoFocusSucceeded;
> };
>
> NS_DispatchToMainThread(new Callback(mDOMCameraControl, aAutoFocusSucceeded));
> }
>
> void
>-DOMCameraControlListener::OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType)
>+DOMCameraControlListener::OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType)
> {
> class Callback : public DOMCallback
> {
> public:
> Callback(nsMainThreadPtrHandle<nsISupports> aDOMCameraControl,
>- uint8_t* aData, uint32_t aLength, const nsAString& aMimeType)
>+ const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType)
> : DOMCallback(aDOMCameraControl)
>- , mData(aData)
> , mLength(aLength)
> , mMimeType(aMimeType)
>- { }
>+ {
>+ mData = (uint8_t*) malloc(aLength);
>+ memcpy(mData, aData, aLength);
>+ }
>
> void
> RunCallback(nsDOMCameraControl* aDOMCameraControl) override
> {
> nsCOMPtr<nsIDOMBlob> picture =
> File::CreateMemoryFile(mDOMCameraControl.get(),
> static_cast<void*>(mData),
> static_cast<uint64_t>(mLength),
>diff --git a/dom/camera/DOMCameraControlListener.h b/dom/camera/DOMCameraControlListener.h
>index a5c30c6..c1a1be8 100644
>--- a/dom/camera/DOMCameraControlListener.h
>+++ b/dom/camera/DOMCameraControlListener.h
>@@ -16,17 +16,17 @@ class CameraPreviewMediaStream;
> class DOMCameraControlListener : public CameraControlListener
> {
> public:
> DOMCameraControlListener(nsDOMCameraControl* aDOMCameraControl, CameraPreviewMediaStream* aStream);
>
> virtual void OnAutoFocusComplete(bool aAutoFocusSucceeded) override;
> virtual void OnAutoFocusMoving(bool aIsMoving) override;
> virtual void OnFacesDetected(const nsTArray<ICameraControl::Face>& aFaces) override;
>- virtual void OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) override;
>+ virtual void OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) override;
>
> virtual void OnHardwareStateChange(HardwareState aState, nsresult aReason) override;
> virtual void OnPreviewStateChange(PreviewState aState) override;
> virtual void OnRecorderStateChange(RecorderState aState, int32_t aStatus, int32_t aTrackNum) override;
> virtual void OnConfigurationChange(const CameraListenerConfiguration& aConfiguration) override;
> virtual void OnShutter() override;
> virtual void OnRateLimitPreview(bool aLimit) override;
> virtual bool OnNewPreviewFrame(layers::Image* aImage, uint32_t aWidth, uint32_t aHeight) override;
>diff --git a/dom/camera/GonkCameraControl.cpp b/dom/camera/GonkCameraControl.cpp
>index 3e1eee7..41a15e5 100644
>--- a/dom/camera/GonkCameraControl.cpp
>+++ b/dom/camera/GonkCameraControl.cpp
>@@ -1529,24 +1529,20 @@ nsGonkCameraControl::OnFacesDetected(camera_frame_metadata_t* aMetaData)
> CameraControlImpl::OnFacesDetected(faces);
> }
>
> void
> nsGonkCameraControl::OnTakePictureComplete(uint8_t* aData, uint32_t aLength)
> {
> ReentrantMonitorAutoEnter mon(mReentrantMonitor);
>
>- uint8_t* data = new uint8_t[aLength];
>-
>- memcpy(data, aData, aLength);
>-
> nsString s(NS_LITERAL_STRING("image/"));
> s.Append(mFileFormat);
> DOM_CAMERA_LOGI("Got picture, type '%s', %u bytes\n", NS_ConvertUTF16toUTF8(s).get(), aLength);
>- OnTakePictureComplete(data, aLength, s);
>+ OnTakePictureComplete(aData, aLength, s);
>
> if (mResumePreviewAfterTakingPicture) {
> nsresult rv = StartPreview();
> if (NS_FAILED(rv)) {
> DOM_CAMERA_LOGE("Failed to restart camera preview (%x)\n", rv);
> OnPreviewStateChange(CameraControlListener::kPreviewStopped);
> }
> }
>diff --git a/dom/media/webrtc/MediaEngineGonkVideoSource.cpp b/dom/media/webrtc/MediaEngineGonkVideoSource.cpp
>index 3243807..141e60b 100644
>--- a/dom/media/webrtc/MediaEngineGonkVideoSource.cpp
>+++ b/dom/media/webrtc/MediaEngineGonkVideoSource.cpp
>@@ -585,28 +585,28 @@ MediaEngineGonkVideoSource::OnUserError(UserContext aContext, nsresult aError)
> MonitorAutoLock lock(mMonitor);
> if (mPhotoCallbacks.Length()) {
> NS_DispatchToMainThread(new TakePhotoError(mPhotoCallbacks, aError));
> }
> }
> }
>
> void
>-MediaEngineGonkVideoSource::OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType)
>+MediaEngineGonkVideoSource::OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType)
> {
> // It needs to start preview because Gonk camera will stop preview while
> // taking picture.
> mCameraControl->StartPreview();
>
> // Create a main thread runnable to generate a blob and call all current queued
> // PhotoCallbacks.
> class GenerateBlobRunnable : public nsRunnable {
> public:
> GenerateBlobRunnable(nsTArray<nsRefPtr<PhotoCallback>>& aCallbacks,
>- uint8_t* aData,
>+ const uint8_t* aData,
> uint32_t aLength,
> const nsAString& aMimeType)
> : mPhotoDataLength(aLength)
> {
> mCallbacks.SwapElements(aCallbacks);
> mPhotoData = (uint8_t*) malloc(aLength);
> memcpy(mPhotoData, aData, mPhotoDataLength);
> mMimeType = aMimeType;
>diff --git a/dom/media/webrtc/MediaEngineGonkVideoSource.h b/dom/media/webrtc/MediaEngineGonkVideoSource.h
>index 12d0372..b072f19 100644
>--- a/dom/media/webrtc/MediaEngineGonkVideoSource.h
>+++ b/dom/media/webrtc/MediaEngineGonkVideoSource.h
>@@ -69,17 +69,17 @@ public:
> SourceMediaStream* aSource,
> TrackID aId,
> StreamTime aDesiredTime) override;
>
> void OnHardwareStateChange(HardwareState aState, nsresult aReason) override;
> void GetRotation();
> bool OnNewPreviewFrame(layers::Image* aImage, uint32_t aWidth, uint32_t aHeight) override;
> void OnUserError(UserContext aContext, nsresult aError) override;
>- void OnTakePictureComplete(uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) override;
>+ void OnTakePictureComplete(const uint8_t* aData, uint32_t aLength, const nsAString& aMimeType) override;
>
> void AllocImpl();
> void DeallocImpl();
> void StartImpl(webrtc::CaptureCapability aCapability);
> void StopImpl();
> uint32_t ConvertPixelFormatToFOURCC(int aFormat);
> void RotateImage(layers::Image* aImage, uint32_t aWidth, uint32_t aHeight);
> void Notify(const mozilla::hal::ScreenConfiguration& aConfiguration);
|
|
Reporter | |
Comment 9•10 years ago
|
||
Comment on attachment 8597733 [details] [diff] [review]
bug1129051.patch, V2
Review of attachment 8597733 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/camera/GonkCameraControl.cpp
@@ +1537,4 @@
> nsString s(NS_LITERAL_STRING("image/"));
> s.Append(mFileFormat);
> DOM_CAMERA_LOGI("Got picture, type '%s', %u bytes\n", NS_ConvertUTF16toUTF8(s).get(), aLength);
> + OnTakePictureComplete(aData, aLength, s);
I just realized this is missing a change in CameraControlImpl.h and CameraControlImpl.cpp to add const to CameraControlImpl::OnTakePictureComplete (called here). That should generate a compiler warning (apparently non-fatal, that's weird).
Attachment #8597733 -
Flags: review+ → review-
|
Assignee | |
Comment 10•10 years ago
|
||
Comment on attachment 8597733 [details] [diff] [review]
bug1129051.patch, V2
Review of attachment 8597733 [details] [diff] [review]:
-----------------------------------------------------------------
you're correct. I will update the patch. Thanks,
Attachment #8597733 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 11•10 years ago
|
||
From: Roger Yang <royang51@gmail.com>
Bug 1129051 - Fix Camera Control Listener double free. Fix webrtc memory leak r=royang51
Attachment #8598351 -
Flags: review?(aosmond)
Attachment #8598351 -
Attachment is obsolete: true
Attachment #8598351 -
Flags: review?(aosmond)
|
|
Assignee | |
Comment 12•10 years ago
|
||
Attachment #8598364 -
Flags: review?(aosmond)
|
|
Reporter | |
Comment 13•10 years ago
|
||
Comment on attachment 8598364 [details] [diff] [review]
bug1129051.patch, V3
Review of attachment 8598364 [details] [diff] [review]:
-----------------------------------------------------------------
Much better, thanks. I'll try landing this again today.
Attachment #8598364 -
Flags: review?(aosmond) → review+
|
|
Reporter | |
Comment 14•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S11 (1may)
You need to log in
before you can comment on or make changes to this bug.
Description
•