Closed Bug 916643 Opened 11 years ago Closed 10 years ago

ImageCapture - Implement WebIDL and takePhoto()

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: shelly, Assigned: ayang)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(5 files, 26 obsolete files)

9.50 KB, patch
ayang
: review+
Details | Diff | Splinter Review
16.42 KB, patch
ayang
: review+
Details | Diff | Splinter Review
23.18 KB, patch
ayang
: review+
Details | Diff | Splinter Review
22.49 KB, patch
ayang
: review+
Details | Diff | Splinter Review
3.90 KB, patch
ayang
: review+
Details | Diff | Splinter Review
Move the current work of ImageCapture from bug 888177 to here, bug 888177 should be a meta bug to keep track of.

The webidl of this patch hasn't update to the latest version, but the function "takePhoto()" should work.
Attachment #805134 - Attachment is obsolete: true
Blocks: 888177
Assignee: nobody → slin
Assignee: slin → ayang
Attached patch imagecapture-error-event (obsolete) — Splinter Review
[draft]
Although it's still lack lots of comments, but it's probably time to get other people's view first.

1. Add ImageCaptureError webidl and implementation.
2. Add ImageCaptureErrorEvent webidl.
Attachment #8450731 - Flags: feedback?(rlin)
Attachment #8450731 - Flags: feedback?(jwwang)
Attachment #8450731 - Flags: feedback?(jolin)
Attachment #8450731 - Flags: feedback?(bechen)
Attached patch imagecapture (obsolete) — Splinter Review
[draft]

1. Add ImageCapture webidl and implementation.
2. Add ImageCaptureManager to manage ImageCapture thread.
Attachment #8450735 - Flags: feedback?(rlin)
Attachment #8450735 - Flags: feedback?(jwwang)
Attachment #8450735 - Flags: feedback?(jolin)
Attachment #8450735 - Flags: feedback?(bechen)
Comment on attachment 8450731 [details] [diff] [review]
imagecapture-error-event

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

::: dom/events/ImageCaptureError.cpp
@@ +52,5 @@
> +}
> +
> +void
> +ImageCaptureError::GetMessage(nsString& retval)
> +{

Nothing to do?

::: dom/events/ImageCaptureError.h
@@ +38,5 @@
> +  void GetMessage(nsString& retval);
> +
> +private:
> +  nsRefPtr<ImageCapture> mParent;
> +  uint16_t mCode;

memory alignment.

::: dom/webidl/ImageCaptureErrorEvent.webidl
@@ +1,5 @@
> +/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */

Document the source of the webidl.
Attachment #8450731 - Flags: feedback?(rlin) → feedback+
Comment on attachment 8450735 [details] [diff] [review]
imagecapture

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

Could you separate this patch into 
webidl ->You could request roc to review that first.
ImageCapture
ImageCaptureManager/ImageGrabber
others?
Thanks
Attachment #8450735 - Flags: feedback?(rlin)
Comment on attachment 8450735 [details] [diff] [review]
imagecapture

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

::: content/media/ImageCaptureManager.h
@@ +22,5 @@
> + *  singleton object which owns a ImageCapture thread and dispatches the capture
> + *  task into ImageCapture thread.
> + *  A capture task is a runnable running on ImageCapture thread. It will capture
> + *  an image and encode the image when necessary. The capture result willl be
> + *  will be dispatched to main thread via another runnable.

type error.

@@ +99,5 @@
> +  // Note:
> +  //   Both constructor and destructor run on main thread.
> +  //   It is prohibited to create/destrory this singleton on other thread.
> +  ImageCaptureManager();
> +  ~ImageCaptureManager();

Because the ImageCaptureManager is singleton , we should not expose the constructor/destructor.
Comment on attachment 8450735 [details] [diff] [review]
imagecapture

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

::: content/media/ImageCapture.cpp
@@ +32,5 @@
> +                           nsPIDOMWindow* aOwnerWindow)
> +  : DOMEventTargetHelper(aOwnerWindow)
> +  , mCaptureImageManager(nullptr)
> +  , mActive(true)
> +{

Since ImageCapture : public nsIDocumentActivity,
you should call |doc->RegisterActivityObserver, doc->UnregisterActivityObserver| somewhere.

::: content/media/ImageCaptureManager.cpp
@@ +158,5 @@
> +  if (jpeg_data.Length()) {
> +    rv = result->SetResult(jpeg_data);
> +  }
> +
> +  NS_DispatchToMainThread(result);

This won't guarantee the |mImageCapture| is destroyed on main thread. See line 151 you wrote.
Comment on attachment 8450735 [details] [diff] [review]
imagecapture

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

::: content/media/ImageCaptureManager.h
@@ +102,5 @@
> +  ImageCaptureManager();
> +  ~ImageCaptureManager();
> +
> +  // It is created by ImageCapture and dispatched to ImageCapture thread.
> +  // It should be created on main thread and destroyed on ImageCapture thread.

As you described, the CaptureTask is destroyed on ImageCapture thread, hence you can't remove the listener at destructor.
http://dxr.mozilla.org/mozilla-central/source/content/media/MediaStreamGraph.cpp?from=mediastreamgraph.cpp&case=true#1740
Comment on attachment 8450731 [details] [diff] [review]
imagecapture-error-event

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

::: dom/events/ImageCaptureError.h
@@ +32,5 @@
> +
> +  const static uint16_t FRAME_GRAB_ERROR = 1;
> +  const static uint16_t SETTINGS_ERROR = 2;
> +  const static uint16_t PHOTO_ERROR = 3;
> +  const static uint16_t ERROR_UNKNOWN = 4;

Use enum for constants?
Attachment #8450731 - Flags: feedback?(jolin) → feedback+
Comment on attachment 8450735 [details] [diff] [review]
imagecapture

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

::: content/media/ImageCapture.cpp
@@ +34,5 @@
> +  , mCaptureImageManager(nullptr)
> +  , mActive(true)
> +{
> +  MOZ_COUNT_CTOR(ImageCapture);
> +  MOZ_ASSERT(&aVideoStreamTrack);

This check doesn't make sense for there is no null-reference in C++.

@@ +74,5 @@
> +{
> +  MOZ_ASSERT(mCaptureImageManager);
> +  nsRefPtr<ImageCaptureManager::CaptureTask> ct =
> +    new ImageCaptureManager::CaptureTask(this, mCaptureImageManager);
> +  mCaptureImageManager->AddTask(ct);

ImageCaptureManager should expose a function as simple as TakePhoto(ImageCaputre*) instead of exposing CaptureTask to ImageCaputre.

::: content/media/ImageCaptureManager.cpp
@@ +27,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (!sImageCaptureManager) {
> +    sImageCaptureManager = new ImageCaptureManager;
> +    sImageCaptureManager->Init();

Always check return value for Init() could fail.

@@ +46,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (sImageCaptureManager->mClients.RemoveElement(aClient)) {
> +    if (!sImageCaptureManager->mClients.Length()) {

IsEmpty() is clearer.

@@ +53,5 @@
> +    }
> +    return;
> +  }
> +
> +  // The manager could be leakage due to malfunction client.

The comment is wrong. We reach here for |aClient| is missing in |mClients| which could mean Connect()/Disconnect() mismatch.

@@ +75,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  // mClients.Length() should be zero; otherwise, there must be leakage
> +  // somewhere.
> +  MOZ_ASSERT(!mClients.Length());

IsEmpty()

@@ +92,5 @@
> +
> +bool
> +ImageCaptureManager::IsImageCaptureThread()
> +{
> +  if (mImageCaptureTaskQueue) {

Assert mImageCaptureTaskQueue is true for if Init() fails, there is no way to continue.

@@ +126,5 @@
> +  MOZ_ASSERT(vst);
> +  nsRefPtr<MediaStream> ms = vst->GetStream()->GetStream();
> +  MOZ_ASSERT(ms);
> +
> +  ms->RemoveListener(mImageGrabber);

stream register/deregister code should move to ImageGrabber. Such details should be hidden in ImageGrabber instead of exposing to CaptureTask.

@@ +132,5 @@
> +
> +NS_IMETHODIMP
> +ImageCaptureManager::CaptureTask::Run()
> +{
> +  MOZ_ASSERT(NS_IsMainThread());

Isn't it the capture thread?

@@ +138,5 @@
> +  // Do capture task here. ImageGrabber->GetImage() is a synchronized API.
> +  nsRefPtr<layers::Image> image;
> +  nsresult rv = mImageGrabber->GetImage(image);
> +
> +  // Convert image to JPEG format.

Does the spec. specify JPEG format?

@@ +140,5 @@
> +  nsresult rv = mImageGrabber->GetImage(image);
> +
> +  // Convert image to JPEG format.
> +  nsTArray<uint8_t> jpeg_data;
> +  if (NS_SUCCEEDED(rv) && image) {

When rv is SUCCESS, it should guarantee image is valid.

@@ +231,5 @@
> +nsresult
> +ImageCaptureManager::CaptureResult::SetResult(nsTArray<uint8_t>& aImage)
> +{
> +  // mCapturedImage should be empty.
> +  MOZ_ASSERT(!mCapturedImage.Length());

IsEmpty()

::: content/media/ImageCaptureManager.h
@@ +83,5 @@
> +  // main thread only.
> +  nsresult AddTask(nsIRunnable* aTask);
> +
> +  // Post a runnable to main thread, it runs on ImageCapture thread only.
> +  nsresult PostTaskResult();

I can't find where it is defined.

::: content/media/ImageGrabber.cpp
@@ +13,5 @@
> +  , mTrackEnded(false)
> +  , mReentrantMonitor("media.ImageGrabber")
> +{
> +  MOZ_COUNT_CTOR(ImageGrabber);
> +  mVideoFrame->SetNull();

Is this needed?
Attachment #8450735 - Flags: feedback?(jwwang)
Comment on attachment 8450735 [details] [diff] [review]
imagecapture

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

::: content/media/ImageCapture.cpp
@@ +10,5 @@
> +#include "mozilla/dom/ImageCaptureErrorEvent.h"
> +#include "mozilla/dom/ImageCaptureErrorEventBinding.h"
> +#include "nsIDocument.h"
> +#include "mozilla/dom/DOMException.h"
> +#include "ImageCapture.h"

Put it in front of other headers to make sure all dependencies are resolved in this header file?

@@ +33,5 @@
> +  : DOMEventTargetHelper(aOwnerWindow)
> +  , mCaptureImageManager(nullptr)
> +  , mActive(true)
> +{
> +  MOZ_COUNT_CTOR(ImageCapture);

jesup: "Not needed if it's NS_INLINE_DECL_THREADSAFE_REFCOUNTING() (or NS_IMPL_ADDREF).  Only for non-refcounted objects."

@@ +46,5 @@
> +}
> +
> +ImageCapture::~ImageCapture()
> +{
> +  MOZ_COUNT_DTOR(ImageCapture);

Ditto.

@@ +71,5 @@
> +
> +void
> +ImageCapture::TakePhoto(ErrorResult& aResult)
> +{
> +  MOZ_ASSERT(mCaptureImageManager);

Seems not needed cause it has been checked in ctr and will only be null-ed by dtr.

@@ +74,5 @@
> +{
> +  MOZ_ASSERT(mCaptureImageManager);
> +  nsRefPtr<ImageCaptureManager::CaptureTask> ct =
> +    new ImageCaptureManager::CaptureTask(this, mCaptureImageManager);
> +  mCaptureImageManager->AddTask(ct);

Let manager create the task?

@@ +88,5 @@
> +  }
> +
> +  if (!IsActive()) {
> +    // Document is not active, post error.
> +    return PostErrorEvent(ImageCaptureError::PHOTO_ERROR);

aReason is NS_OK?

@@ +96,5 @@
> +  init.mBubbles = false;
> +  init.mCancelable = false;
> +  init.mData = aBlob;
> +
> +  nsRefPtr<BlobEvent> blob_event =

s/blob_event/blobEvent/g

@@ +110,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +  nsresult rv = CheckInnerWindowCorrectness();
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsString error_msg;

s/error_msg/errorMsg/g

@@ +140,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  nsRefPtr<DOMMediaStream> ms = mVideoStreamTrack->GetStream();
> +  if (!ms) {

s/!ms/NS_WARN_IF(!ms)/ to print debugging message?

@@ +144,5 @@
> +  if (!ms) {
> +    return false;
> +  }
> +  nsCOMPtr<nsIPrincipal> principal = ms->GetPrincipal();
> +  if (!GetOwner())

Debugging message?

@@ +145,5 @@
> +    return false;
> +  }
> +  nsCOMPtr<nsIPrincipal> principal = ms->GetPrincipal();
> +  if (!GetOwner())
> +    return false;

Surround with {}.

@@ +148,5 @@
> +  if (!GetOwner())
> +    return false;
> +  nsCOMPtr<nsIDocument> doc = GetOwner()->GetExtantDoc();
> +  if (!doc || !principal)
> +    return false;

Ditto.

@@ +152,5 @@
> +    return false;
> +
> +  bool subsumes;
> +  if (NS_FAILED(doc->NodePrincipal()->Subsumes(principal, &subsumes)))
> +    return false;

Ditto.

@@ +170,5 @@
> +    mActive = false;
> +    return;
> +  }
> +
> +  mActive = true;

How about |mActive = doc->IsActive() && doc->IsVisible();|?

::: content/media/ImageCapture.h
@@ +18,5 @@
> +class ImageCaptureManager;
> +
> +namespace dom {
> +
> +class CaptureTask;

Why this?

@@ +59,5 @@
> +                                                    JSContext* aCx,
> +                                                    VideoStreamTrack& aTrack,
> +                                                    ErrorResult& aRv);
> +
> +  ImageCapture(VideoStreamTrack& aVideoStreamTrack, nsPIDOMWindow* aOwnerWindow);

Make this protected or private if it's not going to be called by arbitrary class?

@@ +64,5 @@
> +
> +  virtual ~ImageCapture();
> +
> +  // Post a Blob event to script.
> +  nsresult PostBlobEvent(nsIDOMBlob* aBlob);

Ditto.

@@ +68,5 @@
> +  nsresult PostBlobEvent(nsIDOMBlob* aBlob);
> +
> +  // Post an error event to script.
> +  // aErrorCode should be one of error codes in ImageCaptureError.h
> +  nsresult PostErrorEvent(uint16_t aErrorCode, nsresult aReason = NS_OK);

Ditto.

::: content/media/ImageCaptureManager.cpp
@@ +27,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (!sImageCaptureManager) {
> +    sImageCaptureManager = new ImageCaptureManager;
> +    sImageCaptureManager->Init();

Check Init() return value?

@@ +46,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  if (sImageCaptureManager->mClients.RemoveElement(aClient)) {
> +    if (!sImageCaptureManager->mClients.Length()) {

IsEmpty()?

@@ +75,5 @@
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  // mClients.Length() should be zero; otherwise, there must be leakage
> +  // somewhere.
> +  MOZ_ASSERT(!mClients.Length());

IsEmpty()?

@@ +152,5 @@
> +
> +  // Release ImageCapture early before dispatching CaptureResult to main thread
> +  // so we can guarantee that ImageCapture won't be destroyed in ImageCapture
> +  // thread.
> +  mImageCapture = nullptr;

If mImageCapture becomes nullptr here, line 125 will SIGSEGV.

::: content/media/ImageCaptureManager.h
@@ +18,5 @@
> +class SharedThreadPool;
> +
> +/**
> + *  ImageCaptureManager is the main class to perform ImageCapture task. It is a
> + *  singleton object which owns a ImageCapture thread and dispatches the capture

...owns *an* ImageCapture...

@@ +71,5 @@
> + *  Note:
> + *    ImageCaptureManager::PostTaskResult() is called on ImageCapture thread
> + *    during CaptureTask::Run(). It will create CaptureResult which holds a
> + *    reference to ImageCapture. So ImageCapture reference count will be n+2
> + *    in PostTaskResult().

Can the ref simply "moved" here (using nsRefPtr::swap())? It saves 1 pair of addRef()/release().

@@ +80,5 @@
> +{
> +public:
> +  // Add a ImageCapture task to mTaskQueue. This function should be invoked on
> +  // main thread only.
> +  nsresult AddTask(nsIRunnable* aTask);

How about AddTask(ImageCapture*) and let manager create the task? This way manager can do additional sanity check to see if given ImageCapture is connected or not.
Attachment #8450735 - Flags: feedback?(jolin) → feedback-
Comment on attachment 8450731 [details] [diff] [review]
imagecapture-error-event

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

::: dom/events/ImageCaptureError.cpp
@@ +25,5 @@
> +  , mCode(aCode)
> +  , mMessage(aMessage)
> +{
> +  SetIsDOMBinding();
> +  mCode = ERROR_UNKNOWN;

Overwrite the value specified in the initialization list?

@@ +45,5 @@
> +  return ImageCaptureErrorBinding::Wrap(aCx, this);
> +}
> +
> +uint16_t
> +ImageCaptureError::Code()

const-correctness.

@@ +51,5 @@
> +  return mCode;
> +}
> +
> +void
> +ImageCaptureError::GetMessage(nsString& retval)

const-correctness.

::: dom/events/ImageCaptureError.h
@@ +19,5 @@
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(ImageCaptureError)
> +
> +public:
> +  ImageCaptureError(ImageCapture* aParent, uint16_t aCode, nsString& aMessage);

s/nsString/const nsAString/

@@ +37,5 @@
> +
> +  void GetMessage(nsString& retval);
> +
> +private:
> +  nsRefPtr<ImageCapture> mParent;

const?

@@ +38,5 @@
> +  void GetMessage(nsString& retval);
> +
> +private:
> +  nsRefPtr<ImageCapture> mParent;
> +  uint16_t mCode;

const?

@@ +39,5 @@
> +
> +private:
> +  nsRefPtr<ImageCapture> mParent;
> +  uint16_t mCode;
> +  nsString mMessage;

const?
Attachment #8450731 - Flags: feedback?(jwwang) → feedback-
Attachment #8450731 - Flags: feedback?(bechen)
Attachment #8450735 - Flags: feedback?(bechen)
Comment on attachment 8450735 [details] [diff] [review]
imagecapture

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

::: content/media/ImageCaptureManager.cpp
@@ +218,5 @@
> +  , mManager(aManager)
> +{
> +  MOZ_COUNT_CTOR(CaptureResult);
> +  MOZ_ASSERT(mManager);
> +  MOZ_ASSERT(mManager->IsImageCaptureThread());

Looks like this is the only place ImageCaptureManager is used in CaptureTask & CaptureResult, and seems can be replaced with sImageCaptureManager.
How about removing CaptureTask::mManager and CaptureResult::mManager?

::: content/media/ImageGrabber.cpp
@@ +42,5 @@
> +        ReentrantMonitorAutoEnter mon(mReentrantMonitor);
> +        mVideoFrame->TakeFrom(&chunk.mFrame);
> +        mon.NotifyAll();
> +        return;
> +      }

Add support to IsNull() case? (Black screen)

@@ +58,5 @@
> +void
> +ImageGrabber::NotifyRemoved(MediaStreamGraph* aGraph)
> +{
> +  ReentrantMonitorAutoEnter mon(mReentrantMonitor);
> +  mTrackEnded = true;

Do we want to keep mVideoFrame after track ended?
Attached patch part1-imagecapture-error-event (obsolete) — Splinter Review
Attachment #805136 - Attachment is obsolete: true
Attachment #8450731 - Attachment is obsolete: true
Attachment #8450735 - Attachment is obsolete: true
Attached patch part2-imagecapture-webidl (obsolete) — Splinter Review
Attached patch part3-imagecapture-manager (obsolete) — Splinter Review
Attached patch part4-imagecapture-task (obsolete) — Splinter Review
Attached patch part5-imagecapture-grabber (obsolete) — Splinter Review
Attached patch part6-imagecapture-log (obsolete) — Splinter Review
Attachment #8457757 - Flags: review?(bugs)
Attachment #8457758 - Flags: review?(roc)
Attachment #8457760 - Flags: review?(roc)
Attachment #8457761 - Flags: review?(roc)
Attachment #8457762 - Flags: review?(roc)
Attachment #8457763 - Flags: review?(roc)
Comment on attachment 8457762 [details] [diff] [review]
part5-imagecapture-grabber

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

::: content/media/imagecapture/ImageGrabber.h
@@ +63,5 @@
> +  // Track ended flag, protected by mReentrantMonitor.
> +  bool mTrackEnded;
> +
> +  // It protects members which accessed by MSG thread and ImageCapture thread.
> +  ReentrantMonitor mReentrantMonitor;

Do we need ReentrantMonitor here? Or just a Monitor.
Comment on attachment 8457757 [details] [diff] [review]
part1-imagecapture-error-event

Looks like the spec has a small issue with imageCaptureError attribute.
How could it be non-null if imageCaptureErrorInitDict is optional.

Could you file a spec bug about imageCaptureError usage.
BlobEvent has the same issue.
Attachment #8457757 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #21)
> Comment on attachment 8457757 [details] [diff] [review]
> part1-imagecapture-error-event
> 
> Looks like the spec has a small issue with imageCaptureError attribute.
> How could it be non-null if imageCaptureErrorInitDict is optional.
> 
> Could you file a spec bug about imageCaptureError usage.
> BlobEvent has the same issue.

Sure, I'll do that.
Comment on attachment 8457758 [details] [diff] [review]
part2-imagecapture-webidl

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

Patches should build and pass tests without depending on patches after them. So, some patches should be folded together here.

::: content/media/imagecapture/ImageCapture.h
@@ +85,5 @@
> +  nsRefPtr<VideoStreamTrack> mVideoStreamTrack;
> +
> +  // ImageCaptureManager is a singleton, its life time is longer than an
> +  // ImageCapture object so it is safe to use a raw pointer here.
> +  ImageCaptureManager* mCaptureImageManager;

Call it mImageCaptureManager for consistency
Attachment #8457758 - Flags: review?(roc) → review+
Comment on attachment 8457758 [details] [diff] [review]
part2-imagecapture-webidl

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

Also, this needs DOM peer review.
Attachment #8457758 - Flags: review?(bugs)
Comment on attachment 8457760 [details] [diff] [review]
part3-imagecapture-manager

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

::: content/media/imagecapture/ImageCaptureManager.h
@@ +47,5 @@
> + *                                      +------------------------+
> + *                                        ImageCapture obj B
> + *
> + *
> + *  ImageCaputre's reference is held by JS object, CaptureTask and CaptureResult.

typo

@@ +72,5 @@
> + *                  |                             |           |  n
> + *
> + *  Note:
> + *    If an ImageCapture has no any runnable object on ImageCapture thread or
> + *    CaptureResult on main thread. Its lifetime is controlled by cycle collector.

The first sentence here isn't a complete sentence and I don't understand it.
Attachment #8457760 - Flags: review?(roc) → review+
Comment on attachment 8457762 [details] [diff] [review]
part5-imagecapture-grabber

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

::: content/media/imagecapture/ImageGrabber.cpp
@@ +15,5 @@
> +
> +// Generate a black image.
> +nsresult GenerateBlackImage(uint32_t aWidth,
> +                            uint32_t aHeight,
> +                            nsRefPtr<layers::Image>& aImage)

Don't we already have a copy of this code elsewhere? find it and share it

::: content/media/imagecapture/ImageGrabber.h
@@ +48,5 @@
> +  virtual void NotifyRemoved(MediaStreamGraph* aGraph) MOZ_OVERRIDE;
> +
> +  // Get an image from mediastream. It waits for mediastream to get a image;
> +  // in other words, it is a sync function.
> +  nsresult GetImage(nsRefPtr<layers::Image>& aImage);

called on which thread?

Why doesn't this return an already_AddRefed<Image>?

@@ +63,5 @@
> +  // Track ended flag, protected by mReentrantMonitor.
> +  bool mTrackEnded;
> +
> +  // It protects members which accessed by MSG thread and ImageCapture thread.
> +  ReentrantMonitor mReentrantMonitor;

Say which members this protects
Attachment #8457762 - Flags: review?(roc) → review-
Comment on attachment 8457763 [details] [diff] [review]
part6-imagecapture-log

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

::: content/media/imagecapture/ImageCaptureLog.h
@@ +11,5 @@
> +#ifdef PR_LOGGING
> +
> +#ifndef IC_LOG
> +PRLogModuleInfo* GetICLog();
> +#define IC_LOG(...) PR_LOG(GetICLog(), PR_LOG_DEBUG, (__VA_ARGS__))

Don't make this a separate patch. Merge it into the others.

::: content/media/imagecapture/moz.build
@@ +17,5 @@
> +    'CaptureTask.cpp',
> +    'ImageCapture.cpp',
> +    'ImageCaptureLog.cpp',
> +    'ImageCaptureManager.cpp',
> +    'ImageGrabber.cpp',

This should be moved to other patches
Attachment #8457763 - Flags: review?(roc) → review-
It's not clear to me why we need the ImageCaptureManager. Couldn't every ImageCapture::takePhoto start its own thread to take the photo?
Blocks: 1041393
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> It's not clear to me why we need the ImageCaptureManager. Couldn't every
> ImageCapture::takePhoto start its own thread to take the photo?

Two reasons for that.
1. Keep a reference of ImageCapture during takePhoto() in case of all JS references are deleted. So the takePhoto() can work properly either success or error.
2. Centralize the camera hardware controller. Because there is only one camera hardware controller in platform like B2G, it's easier to manage camera hardware in a single class/thread.
Comment on attachment 8457758 [details] [diff] [review]
part2-imagecapture-webidl

https://dvcs.w3.org/hg/dap/raw-file/default/media-stream-capture/ImageCapture.html#widl-ImageCapture-onphoto is btw nonsense.
onphoto handler may get any kind of event interface as event, it just means that event.type is "photo".
target.dispatchEvent(new MouseEvent("photo")); is perfectly valid.
Could you file a spec bug.


>--- /dev/null
>+++ b/content/media/imagecapture/ImageCapture.cpp
I don't see this file being added to any moz.build
>+ImageCapture::ImageCapture(VideoStreamTrack& aVideoStreamTrack,
>+                           nsPIDOMWindow* aOwnerWindow)
>+  : DOMEventTargetHelper(aOwnerWindow)
>+  , mCaptureImageManager(nullptr)
>+{
>+  MOZ_ASSERT(aOwnerWindow);
>+
>+  mVideoStreamTrack = &aVideoStreamTrack;
Make the constructor to take VideoStreamTrack* and force
caller to pass +

>+
>+  // Connect to ImageCaptureManaer
>+  mCaptureImageManager = ImageCaptureManager::Connect(this);
>+  MOZ_ASSERT(mCaptureImageManager);
So here we assert that mCaptureImageManager is never null, but in
ImageCapture::TakePhoto you null check. If mCaptureImageManager is never
null, there shouldn't be need for that null check, or, if mCaptureImageManager
can be actually null in some cases, MOZ_ASSERT(mCaptureImageManager); is then wrong.

>+ImageCapture::TakePhoto(ErrorResult& aResult)
>+{
>+  if (!mCaptureImageManager) {
>+    aResult.Throw(NS_ERROR_FAILURE);
>+    return;
>+  }
>+  mCaptureImageManager->TakePhoto(this);
>+}
So if mCaptureImageManager is never null, TakePhoto wouldn't have to throw
(But it is not clear to me whether mCaptureImageManager can be null)
>+  nsCOMPtr<nsIDOMEvent> event =
>+    ImageCaptureErrorEvent::Constructor(this, NS_LITERAL_STRING("error"), init);
>+  event->SetTrusted(true);
>+
>+  return DispatchDOMEvent(nullptr, event, nullptr, nullptr);
You could just create the event and call
DispatchTrustedEvent(event);


>+ImageCapture::CheckPrincipal()
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+
>+  nsRefPtr<DOMMediaStream> ms = mVideoStreamTrack->GetStream();
>+  if (!ms) {
>+    return false;
>+  }
>+  nsCOMPtr<nsIPrincipal> principal = ms->GetPrincipal();
>+  if (!GetOwner()) {
>+    return false;
>+  }
Since if (!GetOwner()) isn't related to GetPrincipal call, 
add a newline before if (!GetOwner()) {

>+  // The MediaStream passed into the constructor.
>+  VideoStreamTrack* GetVideoStreamTrack() const {
{ should be in its own line

return mVideoStreamTrack.get();
You shouldn't need .get();

>+  // ImageCaptureManager is a singleton, its life time is longer than an
>+  // ImageCapture object so it is safe to use a raw pointer here.
>+  ImageCaptureManager* mCaptureImageManager;
Actually, if mCaptureImageManager is a singleton, is there any reason to have this member variable?
Couldn't we just get the singleton object whenever needed.

>+'ImageCapture': {
>+    'headerFile': 'ImageCapture.h',
If you export ImageCapture.h using EXPORTS.mozilla.dom in moz.build, this shouldn't be needed.


>+    'implicitJSContext': [ 'constructor' ],
You don't need this

>+    'resultNotAddRefed': [ 'videoStreamTrack' ],
accessing videoStreamTrack isn't really that performance critical, so this isn't needed.

>+    'binaryNames': { 'videoStreamTrack': 'GetVideoStreamTrack' }
Why you need special binary name? Just use the default.
VideoStreamTrack()
>+},
And if you do those all, you wouldn't need to add anything to Bindings.conf

>+NON_IDL_EVENT(photo,
>+              NS_IMAGECAPTURE_PHOTO,
>+              EventNameType_None,
>+              NS_EVENT)
Do you really need this...

>+// ImageCapture events.
>+#define NS_IMAGECAPTURE_EVENT_START 5900
>+#define NS_IMAGECAPTURE_PHOTO               (NS_IMAGECAPTURE_EVENT_START + 1)
and this?
Attachment #8457758 - Flags: review?(bugs) → review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #26)
> Comment on attachment 8457762 [details] [diff] [review]
> part5-imagecapture-grabber
> 
> Review of attachment 8457762 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/imagecapture/ImageGrabber.cpp
> @@ +15,5 @@
> > +
> > +// Generate a black image.
> > +nsresult GenerateBlackImage(uint32_t aWidth,
> > +                            uint32_t aHeight,
> > +                            nsRefPtr<layers::Image>& aImage)
> 
> Don't we already have a copy of this code elsewhere? find it and share it
> 

I found there is a CreateMutedFrame function in VideoTrackEncoder. However, not all subclass use it. Like OMX track encoder has its owned way to generate a gralloc black image. After discussing with :jolin and :bechen, I plan to remove this function from VideoTrackEncoder and add a function in VideoFrame to generate a black image.
Attached patch part2-imagecapture-blackimage (obsolete) — Splinter Review
Attachment #8457758 - Attachment is obsolete: true
Attachment #8457760 - Attachment is obsolete: true
Attachment #8457761 - Attachment is obsolete: true
Attachment #8457762 - Attachment is obsolete: true
Attachment #8457763 - Attachment is obsolete: true
Attachment #8457761 - Flags: review?(roc)
Attachment #8460079 - Flags: review?(roc)
(In reply to Olli Pettay [:smaug] from comment #30)
> Comment on attachment 8457758 [details] [diff] [review]
> part2-imagecapture-webidl
> 
> https://dvcs.w3.org/hg/dap/raw-file/default/media-stream-capture/
> ImageCapture.html#widl-ImageCapture-onphoto is btw nonsense.
> onphoto handler may get any kind of event interface as event, it just means
> that event.type is "photo".
> target.dispatchEvent(new MouseEvent("photo")); is perfectly valid.
> Could you file a spec bug.
> 

Yes, I'll do it.

> 
> >+  // ImageCaptureManager is a singleton, its life time is longer than an
> >+  // ImageCapture object so it is safe to use a raw pointer here.
> >+  ImageCaptureManager* mCaptureImageManager;
> Actually, if mCaptureImageManager is a singleton, is there any reason to
> have this member variable?
> Couldn't we just get the singleton object whenever needed.
> 

ImageCaptureManager should be retrieved via ImageCaptureManager::Connect() only. ImageCaptureManager::Connect() and ImageCaptureManager::Disconnect() should be called in pairs.
So it could make codes complicated to call Connect() and Disconnect() whenever ImageCaptureManager is needed.
Attached patch part2-imagecapture-blackimage (obsolete) — Splinter Review
Make CreateBlackImage() a share function between ImageCapture and MediaRecorder.
Attachment #8460079 - Attachment is obsolete: true
Attachment #8460079 - Flags: review?(roc)
Attachment #8460089 - Flags: review?(roc)
(In reply to Olli Pettay [:smaug] from comment #30)
> Comment on attachment 8457758 [details] [diff] [review]
> part2-imagecapture-webidl
> 
> >+    'binaryNames': { 'videoStreamTrack': 'GetVideoStreamTrack' }
> Why you need special binary name? Just use the default.
> VideoStreamTrack()
> >+},

VideoStreamTrack is another class name in the header file VideoStreamTrack.h. I've to rename it; otherwise it won't compile.
(In reply to Alfredo Yang from comment #33)
> ImageCaptureManager should be retrieved via ImageCaptureManager::Connect()
> only. ImageCaptureManager::Connect() and ImageCaptureManager::Disconnect()
> should be called in pairs.
> So it could make codes complicated to call Connect() and Disconnect()
> whenever ImageCaptureManager is needed.
I see
Attached patch part3-imagecapture-webidl (obsolete) — Splinter Review
Fold patches into one according to :roc reviews.
Attachment #8460098 - Flags: review?(roc)
Attachment #8460098 - Flags: review?(bugs)
Attachment #8460098 - Attachment is patch: true
Comment on attachment 8460098 [details] [diff] [review]
part3-imagecapture-webidl

>+  if (mCapturedImage.Length()) {
>+    // Capture success, preparing a Blob.
>+    nsCOMPtr<nsIDOMBlob> blob;
>+    nsString mime_type(NS_LITERAL_STRING("image/png"));
>+
>+    // TODO: bug 888742, it needs to use moz_malloc here so the memory usage
>+    // will be twice.
>+    uint32_t blob_size = mCapturedImage.Length() * sizeof(uint8_t);
>+    void* blob_data = moz_malloc(blob_size);
>+    MOZ_ASSERT(blob_data);
>+
>+    if (blob_data) {
>+      memcpy(blob_data, mCapturedImage.Elements(), blob_size);
>+    }
MOZ_ASSERT is useless here.
You use fallible malloc, so you need to assume the allocation may fail. And you do that.
So MOZ_ASSERT is just not needed.



>+    blob = dom::DOMFile::CreateMemoryFile(blob_data, blob_size, mime_type);
I don't think you should try to create a memory file in case blob_data points to null, but
just return early (and dispatch error event?).

>+protected:
>+  nsRefPtr<dom::ImageCapture> mImageCapture;
>+
>+  ImageCaptureManager* mManager;
This needs a comment why raw pointer is safe to use.
What guarantees that the manager outlives CaptureResult object?

>+class SurfaceHelper : public nsRunnable {
Nit, { goes to its own ling



>+  NS_IMETHOD Run() {
also here.
Similar also elsewhere

>+  // ImageCaptureManager is not a reference object and its lifetime is longer
reference counted.
Why its lifetime is longer? Please add a comment
>+  nsRefPtr<BlobEvent> blob_event =
>+    BlobEvent::Constructor(this, NS_LITERAL_STRING("photo"), init);
>+  blob_event->SetTrusted(true);
>+
>+  return DispatchDOMEvent(nullptr, blob_event, nullptr, nullptr);

Use DispatchTrustedEvent. That way you don't need to use ->SetTrusted(true) here.

>+  // Each ImageCapture object should connect manager once.
>+  if (sImageCaptureManager->mClients.Contains(aClient)) {
>+    MOZ_ASSERT(false);
>+    sImageCaptureManager = nullptr;
>+    return nullptr;
>+  }
Couldn't you just do
MOZ_ASSERT(!sImageCaptureManager->mClients.Contains(aClient));



I could take still another look from DOM point of view.
I assume roc reviews thread handling etc. but please tell me if that is not the case.
Attachment #8460098 - Flags: review?(bugs) → review-
(In reply to Alfredo Yang from comment #29)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> > It's not clear to me why we need the ImageCaptureManager. Couldn't every
> > ImageCapture::takePhoto start its own thread to take the photo?
> 
> Two reasons for that.
> 1. Keep a reference of ImageCapture during takePhoto() in case of all JS
> references are deleted. So the takePhoto() can work properly either success
> or error.

I don't see why ImageCaptureManager is needed for that.

takePhoto needs to ensure that the ImageCapture object is kept alive as long as takePhoto could, in the future, dispatch an event to that object. We'll need some object to track the in-progress takePhoto, and that object can keep the ImageCapture alive. But there doesn't need to be a single central ImageCaptureManager to do that.

> 2. Centralize the camera hardware controller. Because there is only one
> camera hardware controller in platform like B2G, it's easier to manage
> camera hardware in a single class/thread.

But there's one of these ImageCaptureManagers per process. So there can still be contention for the hardware. What code is assuming it's the only ImageCaptureManager?
Attachment #8460098 - Flags: review?(roc)
Attached patch part3-imagecapture-webidl (obsolete) — Splinter Review
Update according to :smaug review and :roc suggests to remove ImageCaptureManager.

Hi Roc,
Could you please feedback it?
Thanks.
Attachment #8460098 - Attachment is obsolete: true
Attachment #8462484 - Flags: feedback?(roc)
Comment on attachment 8462484 [details] [diff] [review]
part3-imagecapture-webidl

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

I think this is definitely better, but I should have looked deeper into the previous patch. Sorry... I think that here we actually don't need to create or manage any threads:

I think the ImageGrabber class doesn't need to use Notify/Wait. Instead, when it gets an image (or end of stream), it can send an event to the main thread.

We should still encode the photo off the main thread. For that, I suggest using ImageEncoder::ExtractDataAsync --- that's what CanvasRenderingContext2D uses for off-main-thread image encoding. You will need to refactor that code a little bit, and you should probably move it to dom/base, but it already does what you need to create a Blob etc. Note that ImageEncoder::ExtractDataAsync takes a CanvasRenderingContext2D* parameter but doesn't actually use it, so you can just remove that parameter. You'd want to extend EncodingCompleteEvent and ExtractDataAsync to support dispatching an event to an EventTarget as well as (or instead of) calling a FileCallback.

I think CaptureTask and ImageGrabber can probably be merged into a single class.

::: content/media/imagecapture/CaptureTask.h
@@ +35,5 @@
> +
> +  NS_IMETHOD Run() MOZ_OVERRIDE;
> +
> +protected:
> +  // Encode an image, it uses PNG format.

Er, why PNG format? Surely it makes a lot more sense to encode photos in JPEG format?

::: content/media/imagecapture/TaskDispatcher.h
@@ +61,5 @@
> +  // Creates the CaptureTask runnable. It should be called on main thread only.
> +  nsresult TakePhoto(dom::ImageCapture* aImageCapture);
> +
> +  // Initialized the task queue and the thread pool. (Actually, it creates only
> +  // one thread.) It should be called on main thread only.

No need to mention a thread pool if there's only one thread.
Attachment #8462484 - Flags: feedback?(roc) → feedback-
Blocks: 923046
Attached patch refactory_image_encoder (obsolete) — Splinter Review
ImageEncoder refactory.

Hi Roc,
I add an new callback interface in ImageEncoder::ExtractDataAsync instead of FileCallback. Could you please feedback on it?
Thanks.
Attachment #8462484 - Attachment is obsolete: true
Attachment #8463365 - Flags: feedback?(roc)
(In reply to Olli Pettay [:smaug] from comment #30)
> Comment on attachment 8457758 [details] [diff] [review]
> part2-imagecapture-webidl
> 
> https://dvcs.w3.org/hg/dap/raw-file/default/media-stream-capture/
> ImageCapture.html#widl-ImageCapture-onphoto is btw nonsense.
> onphoto handler may get any kind of event interface as event, it just means
> that event.type is "photo".
> target.dispatchEvent(new MouseEvent("photo")); is perfectly valid.
> Could you file a spec bug.
> 

I'm not quite understand this. Do you mean it needs a new photo event interface like
target.dispatchEvent(new PhotoEvent()); ?

I checked other spec like https://dvcs.w3.org/hg/dap/raw-file/tip/media-stream-capture/MediaRecorder.html#widl-MediaRecorder-ondataavailable; is it also wrong?
Flags: needinfo?(bugs)
(In reply to Alfredo Yang from comment #43) 
> I'm not quite understand this. Do you mean it needs a new photo event
> interface like
> target.dispatchEvent(new PhotoEvent()); ?

No. I just meant the spec is nonsense when it says
"Register/unregister for photo events of type BlobEvent. The handler should expect to get a BlobEvent object as its first parameter."
You don't register for certain event interface, like BlobEvent, you add handler for certain event type. And same event type
can be used with many event interfaces.
Flags: needinfo?(bugs)
Comment on attachment 8463365 [details] [diff] [review]
refactory_image_encoder

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

Use hg move to get a better diff. It's difficult to tell what you changed otherwise.
Attachment #8463365 - Flags: feedback?(roc) → feedback-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #45)
> Comment on attachment 8463365 [details] [diff] [review]
> refactory_image_encoder
> 
> Review of attachment 8463365 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Use hg move to get a better diff. It's difficult to tell what you changed
> otherwise.

Oh, I never use this command before. I'll update the patch.
Attached patch refactory_image_encoder (obsolete) — Splinter Review
Attachment #8463365 - Attachment is obsolete: true
Attachment #8466069 - Flags: feedback?(roc)
Attached patch part3-imagecapture-webidl (obsolete) — Splinter Review
Attachment #8466070 - Flags: feedback?(roc)
Attachment #8460089 - Flags: review?(roc)
Comment on attachment 8466069 [details] [diff] [review]
refactory_image_encoder

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

The basic idea looks good.

::: dom/base/ImageEncoder.cpp
@@ +246,5 @@
> +                                                     aImage,
> +                                                     encoder,
> +                                                     completeEvent,
> +                                                     imgIEncoder::INPUT_FORMAT_HOSTARGB,
> +                                                     nsIntSize(0, 0),

Pass the real size here. This is confusing.

@@ +316,5 @@
>                                    nsICanvasRenderingContextInternal* aContext,
>                                    nsIInputStream** aStream,
>                                    imgIEncoder* aEncoder)
>  {
> +  if (aSize.IsEmpty() && !aImage) {

aSize should be the size of aImage when this gets called, so you shouldn't need to change this line.

@@ +339,5 @@
>      rv = aContext->GetInputStream(encoderType.get(),
>                                    nsPromiseFlatString(aOptions).get(),
>                                    getter_AddRefs(imgStream));
> +  } else if (aImage) {
> +    // SourceSurface is retrieved on main thread only.

Why is that? It seems to me that in most cases we could get the data directly from the Image. For CairoImages I guess that's unsafe, but we can add a GetAsSourceSurfaceFromAnyThread method that you can call here. In the cairo case we could do a sync main-thread dispatch to copy the data. I'm guessing you won't hit the cairo case here anyway.

The code you've written here relies on passing 'surface' from the main thread back to this thread, which is unsafe.

::: dom/base/ImageEncoder.h
@@ +55,5 @@
>                                     bool aUsingCustomOptions,
>                                     uint8_t* aImageBuffer,
>                                     int32_t aFormat,
>                                     const nsIntSize aSize,
> +                                   EncodeCompleteCallback* aEncodeCallback);

You need to document when this callback gets called, and on what thread.

@@ +63,5 @@
> +  static nsresult ExtractImageAsync(nsAString& aType,
> +                                   const nsAString& aOptions,
> +                                   bool aUsingCustomOptions,
> +                                   layers::Image* aImage,
> +                                   EncodeCompleteCallback* aEncodeCallback);

Same here. Also here you need to document that the data is taken from aImage. This is potentially very confusing since "image" means two things: the layers Image containing the uncompressed data, and the compressed image we generate. So it's probably better to call this ExtractDataFromLayersImageAsync.
Attachment #8466069 - Flags: feedback?(roc) → feedback-
Comment on attachment 8466070 [details] [diff] [review]
part3-imagecapture-webidl

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

::: content/media/imagecapture/CaptureResult.h
@@ +20,5 @@
> + *  It posts a JS event according to whether the capturing is success or
> + *  failure.
> + *  This runnable is on main thread.
> + */
> +class CaptureResult : public dom::EncodeCompleteCallback

CaptureResult doesn't need a .h/.cpp file of its own. Just add them to CaptureTask.cpp.

::: content/media/imagecapture/CaptureTask.cpp
@@ +59,5 @@
> +  return dom::ImageEncoder::ExtractImageAsync(type,
> +                                              options,
> +                                              false,
> +                                              mImage,
> +                                              new CaptureResult(imagecapture));

Hmm, in your ImageEncoder patch you went to some effort to make this function work when called off the main thread, but here we're calling it on the main thread.

Seems to me the simplest approach would be to say that ExtractImageAsync can only be called on the main thread. Then we can simplify ExtractImageAsync to get the data for the Image synchronously and pass it off to an encoder thread for encoding.

Another possible approach, which would be slightly more performant, would be to do what I suggested to make ExtractImageAsync work without using the main thread at all, and then call it directly from the MSG thread in CaptureTask::NotifyQueuedTrackChanges.

::: content/media/imagecapture/ImageCapture.cpp
@@ +123,5 @@
> +  nsRefPtr<DOMMediaStream> ms = mVideoStreamTrack->GetStream();
> +  if (!ms) {
> +    return false;
> +  }
> +  nsCOMPtr<nsIPrincipal> principal = ms->GetPrincipal();

The principal could have changed since we actually grabbed the photo. I think we may need to start monitoring principal changes in TakePhoto and check that the principal does not change between TakePhoto and CheckPrincipal.

::: content/media/imagecapture/ImageCapture.h
@@ +23,5 @@
> + *  capture/ImageCapture.html.
> + *  The ImageCapture accepts a VideoStreamTrack as input source. The image will
> + *  be sent back as a JPG format via Blob event.
> + *
> + *  All the functions in ImageCaputre are run in main thread.

ImageCapture

::: content/media/imagecapture/ImageCaptureLog.h
@@ +17,5 @@
> +
> +#else
> +
> +#ifndef IC_LOG
> +#define IC_LOG(...)

This macro is unused?
Attachment #8466070 - Flags: feedback?(roc) → feedback-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #49)
> Comment on attachment 8466069 [details] [diff] [review]
> refactory_image_encoder
> 
> Review of attachment 8466069 [details] [diff] [review]:
> @@ +339,5 @@
> >      rv = aContext->GetInputStream(encoderType.get(),
> >                                    nsPromiseFlatString(aOptions).get(),
> >                                    getter_AddRefs(imgStream));
> > +  } else if (aImage) {
> > +    // SourceSurface is retrieved on main thread only.
> 
> Why is that? It seems to me that in most cases we could get the data
> directly from the Image. For CairoImages I guess that's unsafe, but we can
> add a GetAsSourceSurfaceFromAnyThread method that you can call here. In the
> cairo case we could do a sync main-thread dispatch to copy the data. I'm
> guessing you won't hit the cairo case here anyway.

Does that mean the Image here is actually PlanarYCbCrImage in most cases and it is safe to ignore other ImageFormat?
Flags: needinfo?(roc)
For WebRTC streams (both decoded PeerConnection streams and camera input) and video decodes it normally will be PlanarYCbCrImage. For screen sharing it's going to be a CairoImage. So I don't think we can ignore CairoImage but I guess it won't be the common case.
Flags: needinfo?(roc)
BTW it would be cool if we could pass YCbCr directly to the image encoder, since the first step of JPEG compression is to convert to YCbCr, but we shouldn't worry about that now.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #53)
> BTW it would be cool if we could pass YCbCr directly to the image encoder,
> since the first step of JPEG compression is to convert to YCbCr, but we
> shouldn't worry about that now.

You have to be careful here. JPEG uses different levels (0...255) than video (16...235 for luma, 16...240 for chroma), so there's still some conversion required compared to what we normally store in a PlanarYCbCrImage.
Attached patch part3-refactory_image_encoder (obsolete) — Splinter Review
Attachment #8466069 - Attachment is obsolete: true
Attachment #8466070 - Attachment is obsolete: true
Attachment #8469814 - Flags: feedback?(roc)
Attached patch part4-imagecapture-webidl (obsolete) — Splinter Review
Attachment #8469815 - Flags: feedback?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #50)
> Comment on attachment 8466070 [details] [diff] [review]
> part3-imagecapture-webidl
> 
> ::: content/media/imagecapture/ImageCapture.cpp
> @@ +123,5 @@
> > +  nsRefPtr<DOMMediaStream> ms = mVideoStreamTrack->GetStream();
> > +  if (!ms) {
> > +    return false;
> > +  }
> > +  nsCOMPtr<nsIPrincipal> principal = ms->GetPrincipal();
> 
> The principal could have changed since we actually grabbed the photo. I
> think we may need to start monitoring principal changes in TakePhoto and
> check that the principal does not change between TakePhoto and
> CheckPrincipal.

Should MediaEncoder apply this principal monitor during recording?
Comment on attachment 8469814 [details] [diff] [review]
part3-refactory_image_encoder

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

::: dom/base/ImageEncoder.cpp
@@ +341,5 @@
> +
> +      // Convert color format to BGRA.
> +      RefPtr<gfx::DataSourceSurface> dataSurface;
> +      if (surface->GetFormat() == gfx::SurfaceFormat::B8G8R8A8 ||
> +          surface->GetFormat() != gfx::SurfaceFormat::B8G8R8X8) {

Did you mean == here?

But I'm not sure if B8G8R8X8 actually works. I think InitFromData expects alpha values to be 255, which B8G8R8X8 does not guarantee.

@@ +362,5 @@
> +                                  imgIEncoder::INPUT_FORMAT_HOSTARGB,
> +                                  aOptions);
> +      dataSurface->Unmap();
> +    } else {
> +      return NS_ERROR_FAILURE;

I'd rather not just fail here. It means various things could fail if, for example, on Windows we decode video using DXVA, make a MediaStream from that and call takePhoto on it.

I'm not 100% sure what we should do here, to be honest, so I think we should do something simple for all non-PLANAR_YCBCR images. I think that is: a sync call to the main thread (if not already on the main thread) that
-- calls Image::GetAsSourceSurface
-- then calls gfxUtils::CopySurfaceToDataSourceSurfaceWithFormat on that surface if it's not already B8G8R8A8
-- returning the B8G8R8A8 DataSourceSurface back to this thread in an nsMainThreadSourceSurfaceRef.
Then we can access the data and do the encoding on this thread. How does that sound? I'm sorry that it's not immediately clear to me what the right thing to do is here.

::: dom/base/ImageEncoder.h
@@ +59,5 @@
>                                     int32_t aFormat,
>                                     const nsIntSize aSize,
> +                                   EncodeCompleteCallback* aEncodeCallback);
> +
> +  // Extrac an Image asynchronously. Its function is same as ExtractDataAsync

Extract

@@ +60,5 @@
>                                     const nsIntSize aSize,
> +                                   EncodeCompleteCallback* aEncodeCallback);
> +
> +  // Extrac an Image asynchronously. Its function is same as ExtractDataAsync
> +  // except for the paramenters. aImage is the uncompressed data. aEncodeCallback

parameters
Attachment #8469814 - Flags: feedback?(roc) → feedback-
Comment on attachment 8469815 [details] [diff] [review]
part4-imagecapture-webidl

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

Generally looks pretty good.

::: content/media/imagecapture/CaptureTask.cpp
@@ +80,5 @@
> +CaptureTask::NotifyQueuedTrackChanges(MediaStreamGraph* aGraph, TrackID aID,
> +                                       TrackRate aTrackRate,
> +                                       TrackTicks aTrackOffset,
> +                                       uint32_t aTrackEvents,
> +                                       const MediaSegment& aQueuedMedia)

Fix indent

@@ +112,5 @@
> +    while (!iter.IsEnded()) {
> +      VideoChunk chunk = *iter;
> +      // Extract the first valid video frame.
> +      VideoFrame frame;
> +      if (!chunk.IsNull()) {

I think we should probably

@@ +116,5 @@
> +      if (!chunk.IsNull()) {
> +        nsRefPtr<layers::Image> image;
> +        if (chunk.mFrame.GetForceBlack()) {
> +          // Create a black image.
> +          image = VideoFrame::CreateBlackImage(gfxIntSize(640, 480));

chunk.mFrame has mIntrinsicSize which you should use here.

@@ +143,5 @@
> +
> +void
> +CaptureTask::NotifyEvent(MediaStreamGraph* aGraph, MediaStreamGraphEvent aEvent)
> +{
> +  if (aEvent == TRACK_EVENT_ENDED && !mImageGrabbedOrTrackEnd) {

I don't think this works. TRACK_EVENT_ENDED is not one of the MediaStreamGraphEvents. You can only get TRACK_EVENT_ENDED in NotifyQueuedTrackChanges, so you need to check for TRACK_EVENT_ENDED there.

You *do* have to check for EVENT_FINISHED and EVENT_REMOVED here in case your listener is removed because the stream went away (e.g. because the browser is shutting down). We don't want a CaptureTask (and other stuff) to leak if we shut down before the photo is taken.

::: content/media/imagecapture/CaptureTask.h
@@ +29,5 @@
> +    , mTrackID(aTrackID)
> +    , mImageGrabbedOrTrackEnd(false)
> +    , mPrincipalChanged(false) {}
> +
> +  virtual ~CaptureTask() {}

This should be 'protected'

::: content/media/imagecapture/ImageCapture.cpp
@@ +10,5 @@
> +#include "mozilla/dom/ImageCaptureErrorEvent.h"
> +#include "mozilla/dom/ImageCaptureErrorEventBinding.h"
> +#include "mozilla/dom/VideoStreamTrack.h"
> +#include "nsIDocument.h"
> +#include "ImageCapture.h"

You must #include "ImageCapture.h", the header for this file, first.

::: content/media/moz.build
@@ +52,5 @@
>  
>  if CONFIG['MOZ_EME']:
>      PARALLEL_DIRS += ['eme']
>  
> +PARALLEL_DIRS += ['imagecapture']

Merge this with the 'webspeech' line above.
Attachment #8469815 - Flags: feedback?(roc) → feedback-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #61)
> Comment on attachment 8469815 [details] [diff] [review]
> part4-imagecapture-webidl
> 
> @@ +112,5 @@
> > +    while (!iter.IsEnded()) {
> > +      VideoChunk chunk = *iter;
> > +      // Extract the first valid video frame.
> > +      VideoFrame frame;
> > +      if (!chunk.IsNull()) {
> 
> I think we should probably

I fixed others except for this one. Do you mean it doesn't need to check?
Flags: needinfo?(roc)
Ignore that comment. I was wondering whether skipping null frames was the right thing to do, and decided that you were right and it is.
Flags: needinfo?(roc)
Attached patch part3-refactory_image_encoder (obsolete) — Splinter Review
Attachment #8469814 - Attachment is obsolete: true
Attachment #8469815 - Attachment is obsolete: true
Attachment #8471335 - Flags: feedback?(roc)
Attached patch part4-imagecapture-webidl (obsolete) — Splinter Review
Attachment #8471336 - Flags: feedback?(roc)
Comment on attachment 8471335 [details] [diff] [review]
part3-refactory_image_encoder

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

great!
Attachment #8471335 - Flags: review+
Attachment #8471335 - Flags: feedback?(roc)
Attachment #8471335 - Flags: feedback+
Comment on attachment 8471336 [details] [diff] [review]
part4-imagecapture-webidl

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

Looks great! r+ from me. Needs DOM peer review.
Attachment #8471336 - Flags: feedback?(roc) → feedback+
Attached patch part2-imagecapture-blackimage (obsolete) — Splinter Review
Thank you for reviews.
Could you please also review this patch?
Attachment #8460089 - Attachment is obsolete: true
Attachment #8471499 - Flags: review?(roc)
Attachment #8471336 - Flags: review?(bugs)
Comment on attachment 8471336 [details] [diff] [review]
part4-imagecapture-webidl

>+#ifdef PR_LOGGING
>+
>+PRLogModuleInfo* GetICLog()
>+{
>+  static PRLogModuleInfo* log = nullptr;
>+  if (!log) {
>+    log = PR_NewLogModule("ImageCapture");
>+  }
>+  return log;
>+}
Somewhat surprising to see PRLogging for this kind of small feature.
But if you need it, fine.

>+ImageCapture::TakePhoto(ErrorResult& aResult)
>+{
>+  nsRefPtr<CaptureTask> task =
>+    new CaptureTask(this, mVideoStreamTrack->GetTrackID());
>+
>+  // It adds itself into MediaStreamGraph, so ImageCapture doesn't need to hold the
>+  // reference.
>+  task->AttachStream();
>+}
Per the current spec TakePhoto should dispatch ImageCaptureErrorEvent synchronously if
VideoStreamTrack.readyState isn't "live", or if "upon invocation of multiple takePhoto() method calls in rapid succession".
Then otherwise queue a task.
I don't see where that synchronous event dispatch happens.


(we need tests for this)
Attachment #8471336 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #69)
> Comment on attachment 8471336 [details] [diff] [review]
> part4-imagecapture-webidl
> 
> >+ImageCapture::TakePhoto(ErrorResult& aResult)
> >+{
> >+  nsRefPtr<CaptureTask> task =
> >+    new CaptureTask(this, mVideoStreamTrack->GetTrackID());
> >+
> >+  // It adds itself into MediaStreamGraph, so ImageCapture doesn't need to hold the
> >+  // reference.
> >+  task->AttachStream();
> >+}
> Per the current spec TakePhoto should dispatch ImageCaptureErrorEvent
> synchronously if
> VideoStreamTrack.readyState isn't "live", or if "upon invocation of multiple
> takePhoto() method calls in rapid succession".
> Then otherwise queue a task.
> I don't see where that synchronous event dispatch happens.
> 
> 
> (we need tests for this)

In gecko, VideoStreamStrack.readyState is not implemented yet, maybe we can check VideoStreamTrack.enabled instead of?
For calling takePhoto() in rapid succession, our implementation should be about to handle it. I'll add test case for that in bug 1041393.

[1] http://dev.w3.org/2011/webrtc/editor/getusermedia.html#idl-def-MediaStreamTrackState
Attached patch part4-imagecapture-webidl (obsolete) — Splinter Review
Attachment #8471336 - Attachment is obsolete: true
Attachment #8472212 - Flags: review?(bugs)
Comment on attachment 8472212 [details] [diff] [review]
part4-imagecapture-webidl


>+  // The error code should be INVALID_TRACK, but spec doesn't defined it in
s/defined/define/
And is there a spec bug for the INVALID_TRACK missing?

>+
>+  // The MediaStream passed into the constructor.
>+  already_AddRefed<VideoStreamTrack> GetVideoStreamTrack() const;
As far as I see, VideoStreamTrack* GetVideoStreamTrack() would work here too, and
then the implementation of the method could be just
return mVideoStreamTrack;


>+[Constructor(VideoStreamTrack track)]
>+interface ImageCapture : EventTarget {
>+  // readonly attribute PhotoSettingsOptions photoSettingsOptions;
>+  readonly attribute VideoStreamTrack videoStreamTrack;
>+  attribute EventHandler onphoto;
>+  attribute EventHandler onerror;
>+  // attribute EventHandler onphotosettingschange;
>+  // attribute EventHandler onframegrab;
>+
>+  // [Throws]
>+  // void setOptions (PhotoSettings? photoSettings);
>+  [Throws]
>+  void takePhoto();
>+  // [Throws]
>+  // void getFrame();
>+};
I wonder... do we really want to expose this partially implemented interface to web?
Probably not, and anyway before exposing this to web we need a intent-to-implement/ship
email to dev.platform.

So, for now, could we expose this only to some apps? Or pref this off until we have
more of the API implemented?
Perhaps initially Func="Navigator::HasCameraSupport" ?

r+ for the code, but another patch to enable this only in some contexts?
Attachment #8472212 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #72)
> Comment on attachment 8472212 [details] [diff] [review]
> part4-imagecapture-webidl
> 
> 
> >+  // The error code should be INVALID_TRACK, but spec doesn't defined it in
> s/defined/define/
> And is there a spec bug for the INVALID_TRACK missing?
> 
> >+
> >+  // The MediaStream passed into the constructor.
> >+  already_AddRefed<VideoStreamTrack> GetVideoStreamTrack() const;
> As far as I see, VideoStreamTrack* GetVideoStreamTrack() would work here
> too, and
> then the implementation of the method could be just
> return mVideoStreamTrack;
> 
> 
> >+[Constructor(VideoStreamTrack track)]
> >+interface ImageCapture : EventTarget {
> >+  // readonly attribute PhotoSettingsOptions photoSettingsOptions;
> >+  readonly attribute VideoStreamTrack videoStreamTrack;
> >+  attribute EventHandler onphoto;
> >+  attribute EventHandler onerror;
> >+  // attribute EventHandler onphotosettingschange;
> >+  // attribute EventHandler onframegrab;
> >+
> >+  // [Throws]
> >+  // void setOptions (PhotoSettings? photoSettings);
> >+  [Throws]
> >+  void takePhoto();
> >+  // [Throws]
> >+  // void getFrame();
> >+};
> I wonder... do we really want to expose this partially implemented interface
> to web?
> Probably not, and anyway before exposing this to web we need a
> intent-to-implement/ship
> email to dev.platform.
> 
> So, for now, could we expose this only to some apps? Or pref this off until
> we have
> more of the API implemented?
> Perhaps initially Func="Navigator::HasCameraSupport" ?
> 
> r+ for the code, but another patch to enable this only in some contexts?

Thank you for reviews. I'll add another patch on pref to turn it on/off.

Here are w3c bugs found during reviews.
https://www.w3.org/Bugs/Public/show_bug.cgi?id=26577
https://www.w3.org/Bugs/Public/show_bug.cgi?id=26578
https://www.w3.org/Bugs/Public/show_bug.cgi?id=26579
(In reply to Olli Pettay [:smaug] from comment #72)
> Comment on attachment 8472212 [details] [diff] [review]
> part4-imagecapture-webidl
> >+  void takePhoto();
> >+  // [Throws]
> >+  // void getFrame();
> >+};
> I wonder... do we really want to expose this partially implemented interface
> to web?
> Probably not, and anyway before exposing this to web we need a
> intent-to-implement/ship
> email to dev.platform.
> 
> So, for now, could we expose this only to some apps? Or pref this off until
> we have
> more of the API implemented?
> Perhaps initially Func="Navigator::HasCameraSupport" ?

HasCameraSupport is exists for mozCamera permission check.
I plan to use [Pref="dom.imagecapture.enabled", Func="Navigator::HasImageCaptureSupport".
Attached patch part5-imagecapture-pref (obsolete) — Splinter Review
Attachment #8472893 - Flags: review?(bugs)
Attachment #8472893 - Flags: review?(bugs)
Attached patch part5-imagecapture-pref (obsolete) — Splinter Review
Sorry for spam, forgot to enable it in testing/profiles/prefs_general.js.
Attachment #8472893 - Attachment is obsolete: true
Attachment #8473552 - Flags: review?(bugs)
Comment on attachment 8473552 [details] [diff] [review]
part5-imagecapture-pref

># HG changeset patch
># Parent b64c39480817cd3044b77baffc687ebf8ba45659
># User Alfredo Yang <ayang@mozilla.com>
>Bug 916643: ImageCapture preference. r=smaug
>
>diff --git a/dom/base/Navigator.cpp b/dom/base/Navigator.cpp
>--- a/dom/base/Navigator.cpp
>+++ b/dom/base/Navigator.cpp
>@@ -2207,16 +2207,24 @@ bool
> Navigator::HasCameraSupport(JSContext* /* unused */, JSObject* aGlobal)
> {
>   nsCOMPtr<nsPIDOMWindow> win = GetWindowFromGlobal(aGlobal);
>   return win && nsDOMCameraManager::CheckPermission(win);
> }
> 
> /* static */
> bool
>+Navigator::HasImageCaptureSupport(JSContext* /* unused */, JSObject* aGlobal)
>+{
>+  nsCOMPtr<nsPIDOMWindow> win = GetWindowFromGlobal(aGlobal);
>+  return win && Preferences::GetBool("dom.imagecapture.enabled", false);
>+}
Why you need this when you have the Pref which also checks "dom.imagecapture.enabled"

>-[Constructor(VideoStreamTrack track)]
>+[Pref="dom.imagecapture.enabled", Func="Navigator::HasImageCaptureSupport",
So the Pref should be enough here.


>+++ b/testing/profiles/prefs_general.js
>@@ -220,8 +220,11 @@ user_pref('apz.test.logging_enabled', tr
> user_pref("browser.translation.bing.authURL", "http://%(server)s/browser/browser/components/translation/test/bing.sjs");
> user_pref("browser.translation.bing.translateArrayURL", "http://%(server)s/browser/browser/components/translation/test/bing.sjs");
> 
> // Make sure we don't try to load snippets from the network.
> user_pref("browser.aboutHomeSnippets.updateUrl", "nonexistent://test");
> 
> // Enable debug logging in the mozApps implementation.
> user_pref("dom.mozApps.debug", true);
>+
>+// Enable ImageCapture.
>+user_pref("dom.imagecapture.enabled", true);
Please enable the pref in the tests needing it.
Attachment #8473552 - Flags: review?(bugs) → review-
Attached file part5-imagecapture-pref (obsolete) —
Attachment #8473552 - Attachment is obsolete: true
Attachment #8474395 - Flags: review?(bugs)
Blocks: 1054905
Attachment #8474395 - Flags: review?(bugs) → review+
Rebase.
Attachment #8457757 - Attachment is obsolete: true
Attachment #8471335 - Attachment is obsolete: true
Attachment #8471499 - Attachment is obsolete: true
Attachment #8472212 - Attachment is obsolete: true
Attachment #8474395 - Attachment is obsolete: true
Attachment #8484053 - Flags: review+
Attachment #8484054 - Flags: review+
Attachment #8484055 - Flags: review+
Attachment #8484054 - Attachment is patch: true
Attachment #8484056 - Flags: review+
Attachment #8484057 - Flags: review+
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: