Closed Bug 1054905 Opened 5 years ago Closed 5 years ago

Use takePhotoComplete callback on B2G

Categories

(Core :: Audio/Video, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: ayang, Assigned: ayang)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 7 obsolete files)

After bug 916643, the supporting image is the preview callback from gonk. On B2G, it needs to support CameraControlListener to retrieve the fullsize capture image instead of preview.

http://dxr.mozilla.org/mozilla-central/source/dom/camera/DOMCameraControlListener.h#24
Attached patch b2g_imagecapture_takephoto (obsolete) — Splinter Review
Hi :roc, this is a WIP of imagecapture which uses B2G camera function.
Could you please feedback it?
Thank you.
Attachment #8482163 - Flags: feedback?(roc)
Comment on attachment 8482163 [details] [diff] [review]
b2g_imagecapture_takephoto

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

Seems reasonable other than the given comments

::: content/media/MediaStreamTrack.h
@@ +47,5 @@
> +     * It is for VideoStreamTrack which support snapshots command in platform
> +     * like B2G. If the snapshot is success, the CommandListener::SnapshotComplete
> +     * will be called.
> +     */
> +    VIDEO_SNAPSHOT

Rather than enumerating the commands like this I think we should just have one virtual function per command and a class dedicated to receiving the video snapshots. I don't see what it buys us to have an enum here.

The name of this function should emphasize that it's a custom takePhoto implementation.

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +851,5 @@
> +  MonitorAutoLock lock(mMonitor);
> +  // The camera control should return the picture by the order of issuing TakePhoto()
> +  // so we always use the oldest listener.
> +  mCommandListeners.ElementAt(0)->SnapshotComplete(image, aMimeType);
> +  mCommandListeners.RemoveElementAt(0);

shouldn't you iterate through all the mCommandListeners here and call SnapshotComplete on all of them?
Attachment #8482163 - Flags: feedback?(roc) → feedback+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> Comment on attachment 8482163 [details] [diff] [review]
> b2g_imagecapture_takephoto
> 
> Review of attachment 8482163 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/MediaStreamTrack.h
> ::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
> @@ +851,5 @@
> > +  MonitorAutoLock lock(mMonitor);
> > +  // The camera control should return the picture by the order of issuing TakePhoto()
> > +  // so we always use the oldest listener.
> > +  mCommandListeners.ElementAt(0)->SnapshotComplete(image, aMimeType);
> > +  mCommandListeners.RemoveElementAt(0);
> 
> shouldn't you iterate through all the mCommandListeners here and call
> SnapshotComplete on all of them?

hmm... I may not get your point here.
The reason here is because OnTakePictureComplete() follows mCameraControl->TakePicture() in pairs and mCameraControl->TakePicture() is an async API.
So in cases of multiple ImageCapture objects, it needs to call SnapshotComplete() only once for each OnTakePictureComplete() callback; otherwise, ImageCapture could receive the wrong image.
Attached patch b2g_imagecapture_takephoto (obsolete) — Splinter Review
Attachment #8482163 - Attachment is obsolete: true
Attachment #8482636 - Flags: review?(roc)
Attachment #8482636 - Flags: review?(roc)
Attached file b2g_imagecapture_takephoto (obsolete) —
Addressed all feedback in comment 2.
Attachment #8482636 - Attachment is obsolete: true
Attachment #8483318 - Flags: review?(roc)
Attached patch b2g_imagecapture_takephoto (obsolete) — Splinter Review
Attachment #8483318 - Attachment is obsolete: true
Attachment #8483318 - Flags: review?(roc)
Attachment #8483331 - Flags: review?(roc)
Comment on attachment 8483331 [details] [diff] [review]
b2g_imagecapture_takephoto

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

Do we really need to add to VideoStreamTrack? Can't ImageCapture.cpp be responsible for finding the MediaEngine and calling a method on it directly? It seems to me that MediaEngine should define TakePhoto and PhotoCallback. TrackCaptureTask doesn't need to be exposed in its own file, it can just be some code in ImageCapture.cpp. Does that make sense?

::: content/media/webrtc/MediaEngine.h
@@ +139,5 @@
> +   * command, the picture should be return via aCallback object. Otherwise, it
> +   * returns NS_ERROR_NOT_IMPLEMENTED.
> +   * Currently, only Gonk MediaEngineSource implementation supports it.
> +   */
> +  virtual nsresult TakePhotoByCameraHal(CameraHalPhotoCallback* aCallback) = 0;

We don't need to mention "camera" or "hal" here. Just call this TakePhoto and PhotoCallback.
Attachment #8483331 - Flags: review?(roc)
Attached patch b2g_imagecapture_takephoto (obsolete) — Splinter Review
Attachment #8483331 - Attachment is obsolete: true
Attachment #8484035 - Flags: review?(roc)
Comment on attachment 8484035 [details] [diff] [review]
b2g_imagecapture_takephoto

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

::: content/media/imagecapture/ImageCapture.h
@@ +44,5 @@
> + *
> + *  There are two ways to capture image, MediaEngineSource and MediaStreamGraph.
> + *  When the implementation of MediaEngineSource supports TakePhoto() in platform
> + *  like B2G, it uses the platform camera to grab image. Otherwise, it falls back
> + *  MediaStreamGraph way.

to the MediaStreamGraph way.

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +942,5 @@
> +    NS_IMETHOD Run()
> +    {
> +      nsRefPtr<dom::DOMFile> blob =
> +        dom::DOMFile::CreateMemoryFile(mPhoto.Elements(), mPhoto.Length(), mMimeType);
> +      uint32_t callbackNumbers = mCallbacks.Length();

callbackCount
Attachment #8484035 - Flags: review?(roc) → review+
Thank you for review!
(In reply to Alfredo Yang from comment #3)

> ... OnTakePictureComplete() follows mCameraControl->TakePicture() in pairs and
> mCameraControl->TakePicture() is an async API.

Don't count on that. Although we suppress it now (see bug 990761 for HDR-specific issue) due to limitations in our event model, some camera hardware supports returning multiple photos for a single AOSP takePicture() call, and we should support that.

That's part of the reason why bug 994912 is moving towards an event model instead a callback one.
Attached patch b2g_imagecapture_takephoto (obsolete) — Splinter Review
Rebase
Attachment #8484035 - Attachment is obsolete: true
Attachment #8484767 - Flags: review+
Keywords: checkin-needed
Update comments.
Attachment #8486240 - Attachment is obsolete: true
Attachment #8486246 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/3f9b3f11c39d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.