1108950 - [FoxEye] Implement VideoMonitor Web API which can be associated to MediaStreamTrack and dispatch to WebWorker.

Status: NEW

(Core :: Audio/Video: MediaStreamGraph, defect, P3)

Description

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

This bug addresses the MediaStreamTrack with worker part. It will add some functions into MediaStreamTrack to associate with VideoWorkers.

Comment 1

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

API draft for this bug.

http://chiahungtai.github.io/mediacapture-worker/

Feel free to discuss it.

Comment 2

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

I would like to split this bug in two bugs for decouple the dependency on bug 1070216. The worker monitor part doesn't need to depend on bug 1070216. I will file a new bug for worker processor case which is depended on bug 1070216. And hope we can land this bug sooner.

Comment 3

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Bug 1108950 part.1 - Add onvideoprocess into DedicatedWorkerGlobalScope.

Comment 4

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Bug 1108950 part.2 - Add VideoMonitorEvent.

Comment 5

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Bug 1108950 part.3 - Add WorkerMonitor related functions into MediaStreamTrack.

Comment 6

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Bug 1108950 part.4 - Integrated with MediaStreamGraph.

Comment 7

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

I will change those patch to preference controlled version. And add some test case for the following patches.

Comment 8

[Tzuhao Kuo [:kaku]]

Comment on attachment 8637066 [details] [diff] [review]
Bug 1108950 part.1 - Add onvideoprocess into DedicatedWorkerGlobalScope.

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

::: dom/base/nsGkAtomList.h
@@ +2359,5 @@
>  GK_ATOM(onboundary, "onboundary")
>  #endif
>  
> +#ifdef MOZ_FOXEYE
> + GK_ATOM(onvideoprocess, "onvideoprocess")

Maybe follow the style in this file, no space at the beginning.

::: dom/webidl/DedicatedWorkerGlobalScope.webidl
@@ +23,5 @@
> +
> +#ifdef MOZ_FOXEYE
> +// For FoxEye project.
> +partial interface DedicatedWorkerGlobalScope {
> +  attribute EventHandler onvideoprocess;

Indicate where is this API from, maybe a documentary comments.

Comment 9

[Tzuhao Kuo [:kaku]]

Comment on attachment 8637068 [details] [diff] [review]
Bug 1108950 part.2 - Add VideoMonitorEvent.

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

::: dom/media/VideoProcessEvent.cpp
@@ +12,5 @@
> +namespace mozilla {
> +namespace dom {
> +
> +// Fixme: double check those marco
> +NS_IMPL_CYCLE_COLLECTION_INHERITED(VideoMonitorEvent, Event, mInputImageBitmap)

I don't think an ImageBitmap might refer back to a VideoProcessEvent.

@@ +25,5 @@
> +                                     nsPresContext* aPresContext,
> +                                     WidgetEvent* aEvent)
> +  : Event(aOwner, aPresContext, aEvent)
> +  , mPlaybackTime(0)
> +  , mInputImageBitmap(nullptr)

Initialize fields with the same order in the header.

@@ +55,5 @@
> +  return bitmap.forget();
> +}
> +
> +NS_IMETHODIMP
> +VideoMonitorEvent::InitEvent(StreamTime playbackTime,

aPlaybackTime,

@@ +58,5 @@
> +NS_IMETHODIMP
> +VideoMonitorEvent::InitEvent(StreamTime playbackTime,
> +                             const nsAString& aTrackId,
> +                             layers::Image* aImage)
> +{

MOZ_ASSERT(aImage);

@@ +61,5 @@
> +                             layers::Image* aImage)
> +{
> +  nsresult rv = Event::InitEvent(NS_LITERAL_STRING("videoprocess"),
> +                                 false /* non-bubbling */,
> +                                 true /* cancelable */);

if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

@@ +64,5 @@
> +                                 false /* non-bubbling */,
> +                                 true /* cancelable */);
> +  mPlaybackTime = playbackTime;
> +  mTrackId = aTrackId;
> +  mInputImageBitmap = ImageBitmap::Create(mOwner, aImage);

// Could the mInputBitmap be NULL? If not,
MOZ_ASSERT(mInputImageBitmap);

::: dom/media/VideoProcessEvent.h
@@ +11,5 @@
> +#include "mozilla/Attributes.h"
> +#include "mozilla/ErrorResult.h"
> +#include "nsCycleCollectionParticipant.h"
> +#include "nsWrapperCache.h"
> +#include "MediaSegment.h"

Alphabetic order.

@@ +51,5 @@
> +  already_AddRefed<ImageBitmap> GetInputImageBitmap(ErrorResult& aRv) const;
> +
> +  bool IsTrusted() const { return true; };
> +
> +  nsresult InitEvent(StreamTime playbackTime,

aPlaybackTime,

@@ +58,5 @@
> +
> +private:
> +  StreamTime mPlaybackTime;
> +  nsString mTrackId;
> +  nsRefPtr<ImageBitmap> mInputImageBitmap;

Field list: pointer go first than others in decreasing size.
nsRefPtr<ImageBitmap> mInputImageBitmap;
nsString mTrackId;
StreamTime mPlaybackTime;

Comment 10

[Tzuhao Kuo [:kaku]]

Comment on attachment 8637071 [details] [diff] [review]
Bug 1108950 part.3 - Add WorkerMonitor related functions into MediaStreamTrack.

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

Should take care on the life time of the added workers.

::: dom/media/DOMMediaStream.cpp
@@ +620,5 @@
> +void
> +DOMMediaStream::AddWorkerMonitor(dom::workers::WorkerPrivate* aWorker,
> +                                 TrackID aTrackID)
> +{
> +  TrackUnionStream* unionStream = mStream->AsTrackUnionStream();

In theory, mStream might be other type than TrackUnionStram but should not happen in FoxEye scenario, so MOZ_ASSERT(unionStream);

@@ +621,5 @@
> +DOMMediaStream::AddWorkerMonitor(dom::workers::WorkerPrivate* aWorker,
> +                                 TrackID aTrackID)
> +{
> +  TrackUnionStream* unionStream = mStream->AsTrackUnionStream();
> +  if (unionStream) {

If do MOZ_ASSERT(unionStream); already, remove this if statement.

@@ +633,5 @@
> +{
> +  TrackUnionStream* unionStream = mStream->AsTrackUnionStream();
> +  if (unionStream) {
> +    unionStream->RemoveWorkerMonitor(aWorker, aTrackID);
> +  }

Same here.

::: dom/media/DOMMediaStream.h
@@ +264,5 @@
> +#ifdef MOZ_FOXEYE
> +  void AddWorkerMonitor(dom::workers::WorkerPrivate* aWorker,
> +                        TrackID aTrackID);
> +  void RemoveWorkerMonitor(dom::workers::WorkerPrivate* aWorker,
> +                        TrackID aTrackID);

code alignment.

::: dom/media/MediaStreamTrack.cpp
@@ +67,5 @@
>  }
>  
> +#ifdef MOZ_FOXEYE
> +void
> +MediaStreamTrack::AddWorkerMonitor(WorkerPrivate& worker)

aWorker

@@ +73,5 @@
> +  mStream->AddWorkerMonitor(&worker, mTrackID);
> + }
> +
> +void
> +MediaStreamTrack::RemoveWorkerMonitor(WorkerPrivate& worker)

aWorker

::: dom/media/MediaStreamTrack.h
@@ +66,5 @@
>    void AssignId(const nsAString& aID) { mID = aID; }
>  
> +#ifdef MOZ_FOXEYE
> +  void AddWorkerMonitor(WorkerPrivate& worker);
> +  void RemoveWorkerMonitor(WorkerPrivate& worker);

aWorker.

::: dom/media/TrackUnionStream.cpp
@@ +177,5 @@
> +      virtual void Run() override
> +      {
> +        mStream->AsTrackUnionStream()->AddWorkerMonitorImpl(mWorker, mTrackID);
> +      }
> +      WorkerPrivate* mWorker;

Is it possible that the worker be destroyed before the ControlMessage be executed? If yes, might use nsRefPte<WorkerPrivate> mWorker;

@@ +193,5 @@
> +      if (entry->mOutputTrackID == aTrackID) {
> +        bool bFound = false;
> +        for (uint32_t j = 0; j < entry->mWorkerMonitors.Length(); ++j) {
> +          if (aWorker == entry->mWorkerMonitors[j].first()) {
> +            bFound = true;

break;

@@ +218,5 @@
> +      virtual void Run() override
> +      {
> +        mStream->AsTrackUnionStream()->RemoveWorkerMonitorImpl(mWorker, mTrackID);
> +      }
> +      WorkerPrivate* mWorker;

Some here. nsRefPtr<WorkerPrivate> mWorker;

::: dom/media/TrackUnionStream.h
@@ +43,5 @@
> +
> +  void RemoveWorkerMonitor(WorkerPrivate* aWorker,
> +                           TrackID aTrackID);
> +  void RemoveWorkerMonitorImpl(WorkerPrivate* aWorker,
> +                                 TrackID aTrackID);

The AddWorkerMonitorImpl() and RemoveWorkerMonitorImpe() should be private.

@@ +71,5 @@
>      TrackID mOutputTrackID;
>      nsAutoPtr<MediaSegment> mSegment;
> +#ifdef MOZ_FOXEYE
> +    // The worker status record the dom event is on fly or not. Take true as
> +    // an on fly event going.

Plz say more about why we need to know if there is an event on the fly for a worker here.

@@ +72,5 @@
>      nsAutoPtr<MediaSegment> mSegment;
> +#ifdef MOZ_FOXEYE
> +    // The worker status record the dom event is on fly or not. Take true as
> +    // an on fly event going.
> +    typedef Pair<WorkerPrivate*, bool> WorkerStatusPair;

We need to AddRef on the worker, right?
typedef Pair<nsRefPtr<WorkerPrivate>, bool> WorkerStatusPair;

::: dom/webidl/MediaStreamTrack.webidl
@@ +39,5 @@
> +#ifdef MOZ_FOXEYE
> +// For FoxEye project.
> +partial interface MediaStreamTrack {
> +    // WorkerMonitor is for the case of just frames analysis without any modification.
> +    // 'parameters' cloned and passed in VideoProcessEvent

What parameters?

@@ +41,5 @@
> +partial interface MediaStreamTrack {
> +    // WorkerMonitor is for the case of just frames analysis without any modification.
> +    // 'parameters' cloned and passed in VideoProcessEvent
> +    void addWorkerMonitor(Worker worker);
> +    void removeWorkerMonitor(Worker worker);

Use also 'aFoo' in WebIDL here so that code generator could help.

Also, I think we should throw if users misuse these methods.
For example, the stream does not have video data.

Comment 11

[Tzuhao Kuo [:kaku]]

Comment on attachment 8637075 [details] [diff] [review]
Bug 1108950 part.4 - Integrated with MediaStreamGraph.

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

I am not familiar with the busy-behavior part, need someone else to gives feedback on it.

::: dom/media/TrackUnionStream.cpp
@@ +38,5 @@
> +#include "WorkerRunnable.h"
> +#include "WorkerScope.h"
> +#include "VideoProcessEvent.h"
> +#include "mozilla/dom/ImageBitmap.h"
> +#include "nsQueryObject.h"

Alphabetic order.

@@ +199,5 @@
>        if (entry->mOutputTrackID == aTrackID) {
>          bool bFound = false;
>          for (uint32_t j = 0; j < entry->mWorkerMonitors.Length(); ++j) {
>            if (aWorker == entry->mWorkerMonitors[j].first()) {
>              bFound = true;

break;

@@ +207,5 @@
>          }
>          if (!bFound) {
>            entry->mWorkerMonitors.AppendElement(TrackMapEntry::WorkerStatusPair(aWorker, false));
> +          MediaStream* destStream = entry->mInputPort->GetDestination();
> +          destStream->QueryDOMTrackID(aTrackID, entry->mDOMTrackID);

MOZ_ASSERT(/*entry-mDOMTrackID is not void*/);

No matter the aWorker is found or not, they share the same code here, move there two lines of codes into the end of the "if (entry->mOutputTrackID == aTrackID) {" block.

@@ +260,5 @@
> +      TrackMapEntry* entry = &mTrackMap[i];
> +      if (entry->mOutputTrackID == aTrackID) {
> +        for (uint32_t j = 0; j < entry->mWorkerMonitors.Length(); ++j) {
> +          if (aWorker == entry->mWorkerMonitors[j].first()) {
> +            entry->mWorkerMonitors[j].second() = aHasOnFlyEvent;

So, you are updating the status of a MediaStream here, if the UpdateWorkerStatus() is not always ran in the MSG-thread, you need to use ControlMessage. Same as the way in AddWorkerMonitor()/RemoveWorkerMonitor().

Or, if the UpdateWorkerStatus() is always ran in MSG-thread, add an assertion here.

@@ +365,5 @@
> +  private:
> +    virtual bool
> +    WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
> +    {
> +      DOMEventTargetHelper* aTarget = aWorkerPrivate->GlobalScope();

Not 'aTarget', just 'target'.

@@ +392,5 @@
> +            mTrackID(aTrackID)
> +        {}
> +        virtual void Run() override
> +        {
> +          mStream->AsTrackUnionStream()->UpdateWorkerStatus(mWorker, mTrackID, false);

So, here is where the UpdateWorkerStatus() used. I suggest to hide this ControalMessage in the TrackUnionStream class, something likes the AddWorkerMonitor()/RemoveWorkerMonitor() methods.

@@ +394,5 @@
> +        virtual void Run() override
> +        {
> +          mStream->AsTrackUnionStream()->UpdateWorkerStatus(mWorker, mTrackID, false);
> +        }
> +        WorkerPrivate* mWorker;

Same here, use nsRefPtr to make sure the worker is still alive while this ControlMessage is executed.

@@ +404,5 @@
> +      public:
> +        UpdateWorkerStatusRunnable(TrackUnionStream* aStream, Message* aMessage)
> +          : mStream(aStream)
> +          , mMessage(aMessage)
> +        {}

MOZ_ASSERT(mStream);
MOZ_ASSERT(mMessage);

@@ +412,5 @@
> +        {
> +          mStream->GraphImpl()->AppendMessage(mMessage);
> +          return NS_OK;
> +        }
> +        TrackUnionStream* mStream;

What make sure that the mStream is still alive while this runnable is running in the main thread?

@@ +416,5 @@
> +        TrackUnionStream* mStream;
> +        Message* mMessage;
> +      };
> +      nsRefPtr<UpdateWorkerStatusRunnable> runnable = new
> +        UpdateWorkerStatusRunnable(mStream, new Message(mStream, aWorkerPrivate, mTrackID));

MOZ_ASSERT(runnable);

@@ +423,5 @@
> +    }
> +
> +    virtual bool
> +    PreDispatch(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
> +    {

MOZ_ASSERT(aWorkerPrivate);

@@ +454,5 @@
> +    TrackUnionStream* mStream;
> +    TrackID mTrackID;
> +    StreamTime mPlaybackTime;
> +    nsString mID;
> +    nsRefPtr<Image> mImage;

Again, pointers go first and then the others in decreasing size order.

@@ +514,5 @@
> +            nsAutoPtr<MediaSegment> tmpSegment;
> +            tmpSegment = aInputTrack->GetSegment()->CreateEmptyClone();
> +            tmpSegment->AppendSlice(*aInputTrack->GetSegment(),
> +                                   std::min(inputTrackEndPoint, inputStart),
> +                                   std::min(inputTrackEndPoint, inputEnd));

code alignment.

@@ +524,5 @@
> +              if (image != map->mLastImage) {
> +                map->mLastImage = image;
> +                DispatchToVideoWorkerMonitor(map,
> +                                             map->mOutputTrackID,
> +                                             std::min(inputTrackEndPoint, source->GraphTimeToStreamTime(interval.mStart)),

code alignment.

@@ +553,5 @@
> +                                                      TrackID aTrackID,
> +                                                      StreamTime aPlaybackTime,
> +                                                      const nsString& aID)
> +  {
> +    if (0 == aMapEntry->mWorkerMonitors.Length()) {

aMapEntry->mWorkerMonitors.IsEmpty()

@@ +567,5 @@
> +                                        aTrackID,
> +                                        aPlaybackTime,
> +                                        aID,
> +                                        aMapEntry->mLastImage);
> +        ErrorResult rv;

Not used variable?

@@ +570,5 @@
> +                                        aMapEntry->mLastImage);
> +        ErrorResult rv;
> +        runnable->Dispatch(nullptr);
> +      }
> +      workerPair.second() = true;

I suggest using a method to modify this status and assert it happens in the MSG-thread.

::: dom/media/TrackUnionStream.h
@@ +80,5 @@
>      // an on fly event going.
>      typedef Pair<WorkerPrivate*, bool> WorkerStatusPair;
>      nsTArray<WorkerStatusPair> mWorkerMonitors;
> +    nsString mDOMTrackID;
> +    nsRefPtr<VideoFrame::Image> mLastImage;

mLastImage before mDOMTrackID.

::: dom/workers/WorkerPrivate.h
@@ +198,5 @@
>    // in Construct() and emptied by WorkerFinishedRunnable, and conditionally
>    // traversed by the cycle collector if the busy count is zero.
>    nsRefPtr<WorkerPrivate> mSelfRef;
> +#ifdef MOZ_FOXEYE
> +  bool mAllowEventFromThirdParty;

Why do you introduce this flag? There is no logical difference in this patch.

Comment 12

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Part 0 - FoxEye perference - v1

Comment 13

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Part 1 - Add onvideoprocess into DedicatedWorkerGlobalScope - v2

Comment 14

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Part 2 - Add VideoMonitorEvent. - v2

Comment 15

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Part 3 - Add WorkerMonitor related functions into MediaStreamTrack. - v2

Comment 16

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Part 4 - Integrated with MediaStreamGraph. - v2

Comment 17

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Will add mochitest part recently.

Comment 18

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Part 4 - Integrated with MediaStreamGraph. - v2.1

Comment 19

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Part 5 - Fix code alignment in TrackUnionStream.cpp - v1

No code change. Only fix code alignment.

Comment 20

[Tzuhao Kuo [:kaku]]

Comment on attachment 8644790 [details] [diff] [review]
Part 3 - Add WorkerMonitor related functions into MediaStreamTrack. - v2

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

I think the thread interaction is very complicate now.
I suggest to create the WorkerTuple object alone with the VideoWorkerFeature object (as a member of WorkerTuple) at the DOMMediaStream::AddVideoMonitor() method.
Then refactor the TrackUnionStream::AddVideoMonitor() to be something like void TrackUnionStream::AddWorkerTuple(). (So does the Remove... parts.)

By this way, we dispatch runnables to worker-thread and msg-thread at the main-thread separately.
The main advantage of this refactoring is that in the VideoWorkerFrature::Notify(), you can just utilize the TrackUnionStream::RemoveWorkerTuple(), in stead of writing codes for the same function as the TrackUnionStream::RemoveWorkerTuple().

::: dom/media/MediaStreamTrack.cpp
@@ +7,5 @@
>  
>  #include "DOMMediaStream.h"
>  #include "nsIUUIDGenerator.h"
>  #include "nsServiceManagerUtils.h"
> +#include "mozilla/dom/WorkerPrivate.h"

move this one to VideoStreamTrack.cpp

@@ +8,5 @@
>  #include "DOMMediaStream.h"
>  #include "nsIUUIDGenerator.h"
>  #include "nsServiceManagerUtils.h"
> +#include "mozilla/dom/WorkerPrivate.h"
> +#include "VideoStreamTrack.h"

You don't need this one.

::: dom/media/MediaStreamTrack.h
@@ +62,5 @@
>    // Webrtc allows the remote side to name tracks whatever it wants, and we
>    // need to surface this to content.
>    void AssignId(const nsAString& aID) { mID = aID; }
>  
> +  // AddVideoMonitor and RemoveVideoMonitor will be override by the functions of

... overrode ...

@@ +65,5 @@
>  
> +  // AddVideoMonitor and RemoveVideoMonitor will be override by the functions of
> +  // VideoStreamTrack. So those functions should throw an exception when web
> +  // developer call them in wrong MediaStreamTrack. For example, call those
> +  // function in the MediaStreamTrack with kind="audio".

// functions ......

::: dom/media/TrackUnionStream.cpp
@@ +205,5 @@
> +        // |WorkerPrivate::AddFeature| could be called in worker thread only.
> +        // We need an WorkerRunnable to add the WorkerFeature to the worker.
> +        // This runnable is dispatched from MSG thread, not from parent thread,
> +        // main thread or worker thread. So the BusyCount balance is controlled
> +        // by the runnable itself.

Please document (and make sure) why this worker will be kept alive. To my understanding, since we call ModifyBusyCount(..., true); at VideoStreamTrack::AddVideoMonitor() so that we guarantee that the worker will live long enough to handle the AddFeatureRunnable. If this is right, please add this to comment.

@@ +352,5 @@
> +  }
> +
> +  // Worker is going to closing or terminating. Remove the feature and the
> +  // worker record in TrackMapEntry::mWorkerMonitors.
> +  bool TrackUnionStream::VideoWorkerFeature::Notify(JSContext* aCx, Status aStatus)

So, the functionality of VideoWorkerFeature::Notify() is the same as the TrackUnionStream::RemoveVideoMonitor() but be triggered at different thread. Please see the proposal in the overview to see if we can simplify this situation.

::: dom/media/TrackUnionStream.h
@@ +38,5 @@
> +  void AddVideoMonitor(WorkerPrivate* aWorker,
> +                       TrackID aTrackID);
> +
> +  void RemoveVideoMonitor(WorkerPrivate* aWorker,
> +                           TrackID aTrackID);

Code alignment, or just keep the function declaration in the same line.

@@ +48,5 @@
> +
> +  class VideoWorkerFeature final : public WorkerFeature
> +  {
> +  public:
> +    explicit VideoWorkerFeature(WorkerTuple* aTuple, TrackUnionStream* aStream)

You don't need 'explicit' here.

@@ +54,5 @@
> +      , mStream(aStream)
> +    {
> +    }
> +
> +    bool Notify(JSContext* aCx, Status aStatus) override;

Please document what should be done in this Notify() method.

@@ +65,5 @@
> +
> +  // The worker tuple keep the information of each worker.
> +  struct WorkerTuple {
> +
> +    bool operator==(const WorkerTuple &aOther) const {

const WorkerTuple& aOther

@@ +68,5 @@
> +
> +    bool operator==(const WorkerTuple &aOther) const {
> +      return mWorker == aOther.mWorker;
> +    }
> +    bool operator!=(const WorkerTuple &aOther) const {

const WorkerTuple& aOther

@@ +102,5 @@
>      // the track has been deleted.
>      TrackID mInputTrackID;
>      TrackID mOutputTrackID;
>      nsAutoPtr<MediaSegment> mSegment;
> +    nsTArray<WorkerTuple> mWorkerMonitors;

Please document the relationship of TrackUniomStream, WorkerTiple and VideoWorkerFeature and the life-cycle of WorkerTuple and VideoWorkerFeature. To my understanding, TrackUniomStream owns WorkerTuple and WorkerTuple owns VideoWorkerFeature.

@@ +128,5 @@
> +  // MSG thread only methods
> +  void AddVideoMonitorImpl(WorkerPrivate* aWorker,
> +                           TrackID aTrackID);
> +  void RemoveVideoMonitorImpl(WorkerPrivate* aWorker,
> +                              TrackID aTrackID);

Same here, the function declaration could be in one line, if you want.

::: dom/media/VideoStreamTrack.cpp
@@ +28,5 @@
> +VideoStreamTrack::RemoveVideoMonitor(WorkerPrivate& aWorker, ErrorResult& aRv)
> +{
> +  AutoJSAPI jsapi;
> +  jsapi.Init();
> +  JSContext* cx = jsapi.cx();

If you need a JSContext here, you can add the following line into the MediaStreamItem in dom/bindings/Bingings.conf"

'implicitJSContext': ['removeVideoMonitor']

@@ +30,5 @@
> +  AutoJSAPI jsapi;
> +  jsapi.Init();
> +  JSContext* cx = jsapi.cx();
> +  aWorker.ModifyBusyCount(cx, false);
> +  mStream->RemoveVideoMonitor(&aWorker, mTrackID);

Invert these two steps to secure the worker?

::: dom/media/VideoStreamTrack.h
@@ +26,5 @@
> +
> +  virtual void
> +  AddVideoMonitor(WorkerPrivate& aWorker, ErrorResult& aRv) override;
> +  virtual void
> +  RemoveVideoMonitor(WorkerPrivate& aWorker, ErrorResult& aRv) override;

How about follow the style in this file, do not separate the return type and function name into two lines.

Comment 21

[Tzuhao Kuo [:kaku]]

BTW, I think we should rename the WorkerTuple to something more expressive. ex. VideoWorkerInformation?

Comment 22

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1108950 part.0 - Add a pref to enable FoxEye APIs. r?smaug

Bug 1108950 part.0 - Add a pref to enable FoxEye APIs. r?smaug

Comment 23

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1108950 - part.1 Add onvideoprocess into DedicatedWorkerGlobalScope. r?smaug, roc

Bug 1108950 - part.1 Add onvideoprocess into DedicatedWorkerGlobalScope. r?smaug, roc

Comment 24

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1108950 part.2 - Add VideoMonitorEvent. r?smaug, roc

Bug 1108950 part.2 - Add VideoMonitorEvent. r?smaug, roc

Comment 25

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1108950 part.3 - Add WorkerMonitor related functions into MediaStreamTrack. r?smaug, roc

Bug 1108950 part.3 - Add WorkerMonitor related functions into MediaStreamTrack. r?smaug, roc

Comment 26

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1108950 part.4 - Integrated with MediaStreamGraph. r?roc

Bug 1108950 part.4 - Integrated with MediaStreamGraph. r?roc

Comment 27

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1108950 part.5 - Fix code alignment. r?roc

Bug 1108950 part.5 - Fix code alignment. r?roc

Comment 28

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: MozReview Request: Bug 1108950 part.6 - Test cases. r?roc

Bug 1108950 part.6 - Test cases. r?roc

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8655212 [details]
MozReview Request: Bug 1108950 part.2 - Add VideoMonitorEvent. r?smaug, roc

Please use Event codegenerator to create the event implementation. 
So, just add the VideoProcessEvent.webidl file, the interface, and the interface must also have constructor and dictionary for initializing the properties.
Then add 'VideoProcessEvent.webidl' to dom/webidl/moz.build GENERATED_EVENTS_WEBIDL_FILES and compile and you have the C++ implementation generated.
You may want to check GENERATED_EVENTS_WEBIDL_FILES what all is already codegen'ed

If you want to look at the generated code, see
<objdir>/dist/include/mozilla/dom/VideoProcessEvent.h
and
<objdir>/dom/bindings/VideoProcessEvent.cpp

Do we want to expose the event only on workers? If so, add [Exposed=Worker]. And do we need some Pref="" or Func="" on the interface so that it is exposed in the global scope only when some pref is set?


Sorry if you did extra work on implementing the Event when code generator can do all that work automatically.
(The codegen can deal typical kinds of event interfaces only, but luckily most of the event interfaces are such)

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8655210 [details]
MozReview Request: Bug 1108950 part.0 - Add a pref to enable FoxEye APIs. r?smaug

I assume there will be something like PrefEnabled() somewhere using FoxEyeEnabled(). But I think we should use some other name in the code.
Perhaps videoworkers.enabled and then same naming with the methods so that
whoever reads the code gets at least some hint what the method or pref is about.
With that, r+

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8655211 [details]
MozReview Request: Bug 1108950 - part.1 Add onvideoprocess into DedicatedWorkerGlobalScope. r?smaug, roc

Again here, could we talk about VideoWorkers or something, and not about Foxeye.
(Or if you can think of better name than VideoWorker, use that, but FoxEye sounds like a project name, and not API name :) )

With that r+.

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8655213 [details]
MozReview Request: Bug 1108950 part.3 - Add WorkerMonitor related functions into MediaStreamTrack. r?smaug, roc

I think I'd like to see a new version of this patch once the event interface is implemented using code generator (since that changes a bit how the event is initialized).

>  virtual already_AddRefed<Promise> RemoveVideoMonitor(JSContext* aCx, >WorkerPrivate& aWorker, ErrorResult& aRv) {
>    aRv.Throw(NS_ERROR_DOM_TYPE_MISMATCH_ERR);
>    nsRefPtr<Promise> promise;
>    promise = Promise::Create(GetOwnerGlobal(), aRv);
>    promise->MaybeReject(aRv);
>    return promise.forget();
>  }
webidl bindings layer should automatically create and reject a promise if you throw from a Promise returning method. Same with AddVideoMonitor

ResolvePromiseAtMainThread doesn't seem to guarantee mPromise is actually always released on the main thread. What if the non-main-thread still has a reference to the runnable when ::Run is called, and then last release is called on non-main-thread and that leads mPromise to be release on that thread.

Comment 33

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Thanks for your suggestion. How about |MediaStreamWithWorkeEnabled| and mediastream-workers.enabled? I change the name of VideoWorker to worker in spec. That is for the flexibility of leveraging AudioWorker in the future. 

(In reply to Olli Pettay [:smaug] (high review load, please consider other reviewers) from comment #30)
> Comment on attachment 8655210 [details]
> MozReview Request: Bug 1108950 part.0 - Add a pref to enable FoxEye APIs.
> r?smaug
> 
> I assume there will be something like PrefEnabled() somewhere using
> FoxEyeEnabled(). But I think we should use some other name in the code.
> Perhaps videoworkers.enabled and then same naming with the methods so that
> whoever reads the code gets at least some hint what the method or pref is
> about.
> With that, r+

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

that sounds ok to me.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://reviewboard.mozilla.org/r/17883/#review16147

::: dom/webidl/DedicatedWorkerGlobalScope.webidl:27
(Diff revision 1)
> +  [Func="mozilla::dom::workers::DedicatedWorkerGlobalScope::FoxEyeEnabled"]

I think it's best to avoid "code names" in the code. Here you could just say "VideoProcessorEnabled".

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://reviewboard.mozilla.org/r/17885/#review16149

Waiting for revised patch with codegen.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8655213 [details]
MozReview Request: Bug 1108950 part.3 - Add WorkerMonitor related functions into MediaStreamTrack. r?smaug, roc

https://reviewboard.mozilla.org/r/17887/#review16153

::: dom/media/DOMMediaStream.cpp:643
(Diff revision 1)
> +  TrackUnionStream* unionStream = mStream->AsTrackUnionStream();

How do you know this cast will succeed? I don't think we guarantee that DOMMediaStream always has a TrackUnionStream. I think we should make this stuff work with any kind of MediaStream.

The Monitor functions can be implemented by creating a MediaStreamListener that receives video frames via NotifyQueuedTrackChanges.

::: dom/media/MediaStreamTrack.h:72
(Diff revision 1)
> +  virtual already_AddRefed<Promise> AddVideoMonitor(JSContext* aCx, WorkerPrivate& aWorker, ErrorResult& aRv) {

Why aren't we just adding addVideoMonitor/removeVideoMonitor to VideoStreamTrack instead of MediaStreamTrack?

::: dom/media/TrackUnionStream.h:47
(Diff revision 1)
> +  void RemoveVideoMonitor(Maybe<nsRefPtr<dom::Promise>> aPromise, WorkerPrivate* aWorker, Maybe<TrackID> aTrackID, bool aNeedRemoveFeature = true);

Can't we just pass "nullptr" as the promise instead of using Maybe here?

::: dom/media/TrackUnionStream.h:54
(Diff revision 1)
> +  class VideoWorkerFeature final : public WorkerFeature

Add a comment explaining what this class is for

::: dom/media/VideoStreamTrack.cpp:33
(Diff revision 1)
> +  aWorker.ModifyBusyCount(nullptr, true);

Duplicated code?

::: dom/webidl/MediaStreamTrack.webidl:39
(Diff revision 1)
> +// For FoxEye project.

Don't need this comment.

::: dom/webidl/MediaStreamTrack.webidl:43
(Diff revision 1)
> +    [Pref="foxeye.enabled", Throws]

Call this pref media.videomonitor.enabled

::: dom/webidl/MediaStreamTrack.webidl:46
(Diff revision 1)
> +    Promise<void> removeVideoMonitor(Worker worker);

Can you document here when the promise is resolved?

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8655214 [details]
MozReview Request: Bug 1108950 part.4 - Integrated with MediaStreamGraph. r?roc

https://reviewboard.mozilla.org/r/17889/#review16157

::: dom/media/MediaStreamGraph.cpp:2421
(Diff revision 1)
> +void MediaStream::AppendDOMTarckIDMapping(TrackID aTrackID, const nsString& aDOMTrackID)

Fix "Tarck"

::: dom/media/TrackUnionStream.h:105
(Diff revision 1)
> +    bool mOnFlyStatus;

The comment doesn't really explain what mOnFlyStatus means.

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8655215 [details]
MozReview Request: Bug 1108950 part.5 - Fix code alignment. r?roc

https://reviewboard.mozilla.org/r/17891/#review16159

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8655216 [details]
MozReview Request: Bug 1108950 part.6 - Test cases. r?roc

https://reviewboard.mozilla.org/r/17893/#review16161

::: dom/media/test/remove-video-monitor.js:14
(Diff revision 1)
> +    }, 1000);

Shrink this to 200ms so the test runs faster.

::: dom/media/test/test_mediastream_add_multiple_video_monitor.html:28
(Diff revision 1)
> +      is(event.target, worker1, 'The worker is wrong one.');

Better to write messages like "check that the worker is the right one"

::: dom/media/test/test_mediastream_remove_video_monitor.html:25
(Diff revision 1)
> +      p1.then( 

fix trailing whitespace

::: dom/media/test/test_mediastream_remove_video_monitor.html:29
(Diff revision 1)
> +      });

Where's the timeout parameter?

::: dom/media/test/test_mediastream_remove_video_monitor.html:31
(Diff revision 1)
> +      ok(!event.data, "The worker assocaitaion is not removed.");

"check that the work association has been removed"

Comment 41

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

In current code path, ResolvePromiseAtMainThread will be released on main thread only.
See:
 ResolvePromiseAtMainThread(nsRefPtr<Promise>& aPromise)
      : mPromise(nullptr)
    {
      // Extend the life cycle.
      mPromise.swap(aPromise);
    }
The Promise will be held in JSContext and transfer from MSG thread to main thread.
I can add ASSERT in |ResolvePromiseAtMainThread::Run|. 

But after IRC with roc, he suggest to fix the way video works in MSG first and he will file a bug for it. So that will be a big impact for this bug. I will work on the refactoring first. :)

(In reply to Olli Pettay [:smaug] (reviewing overload. not reviewing for couple of days. If you want a review from me, please send email) from comment #32)
> Comment on attachment 8655213 [details]
> MozReview Request: Bug 1108950 part.3 - Add WorkerMonitor related functions
> into MediaStreamTrack. r?smaug, roc
> 
> I think I'd like to see a new version of this patch once the event interface
> is implemented using code generator (since that changes a bit how the event
> is initialized).
> 
> >  virtual already_AddRefed<Promise> RemoveVideoMonitor(JSContext* aCx, >WorkerPrivate& aWorker, ErrorResult& aRv) {
> >    aRv.Throw(NS_ERROR_DOM_TYPE_MISMATCH_ERR);
> >    nsRefPtr<Promise> promise;
> >    promise = Promise::Create(GetOwnerGlobal(), aRv);
> >    promise->MaybeReject(aRv);
> >    return promise.forget();
> >  }
> webidl bindings layer should automatically create and reject a promise if
> you throw from a Promise returning method. Same with AddVideoMonitor
> 
> ResolvePromiseAtMainThread doesn't seem to guarantee mPromise is actually
> always released on the main thread. What if the non-main-thread still has a
> reference to the runnable when ::Run is called, and then last release is
> called on non-main-thread and that leads mPromise to be release on that
> thread.

Comment 42

[Bulk Bug Changes for mreavy's org]

Mass change P2->P3 to align with new Mozilla triage process.