Closed Bug 1281090 Opened 8 years ago Closed 8 years ago

Add the ability to send synchronous notifications to MediaEventSource

Categories

(Core :: Audio/Video: Playback, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(5 files)

It would be useful for MediaEventSource to be able to send event data directly to the listeners without dispatching them to the target thread.

This will allow us to replace MediaDecoderReaderWrapper::Set{Audio,Video}Callback with MediaEventSource so we have only one tool to publish events across threads or directly to the listeners. This will reduce the code and help maintenance effort.
Assignee: nobody → jwwang
Would that only be allowed if the dispatchee thread is the same as the dispatcher thread? Otherwise maintaining thread safety becomes difficult (and dangerous)
Yes. I add comments to note that the class is not thread safe and a different class name is given (which is MediaCallback) to note the user that the behavior is quite different from that of MediaEventSource (which is thread safe) although they share much of the underlying implementation.
Why not simply check if the current thread is the same as the target thread and if so run the task immediately rather than queue it?

Then you only have one class to worry about, and it is always thread safe.
That was my original design. But I decided to prefer explicit over implicit so the client code will never get unexpected asynchronous notifications while expecting synchronous ones.
Review commit: https://reviewboard.mozilla.org/r/60184/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60184/
Attachment #8764134 - Flags: review?(gsquelart)
Attachment #8764135 - Flags: review?(gsquelart)
Attachment #8764136 - Flags: review?(kaku)
Attachment #8764137 - Flags: review?(gsquelart)
Attachment #8764138 - Flags: review?(gsquelart)
Attachment #8764134 - Flags: review?(gsquelart) → review+
Attachment #8764135 - Flags: review?(gsquelart) → review+
Comment on attachment 8764137 [details]
Bug 1281090. Part 4 - prune disconnected listeners more aggressively to avoid hitting the assertion. .

https://reviewboard.mozilla.org/r/60190/#review57030

r+ after this is fixed (or explained away) :

::: dom/media/MediaEventSource.h:411
(Diff revision 1)
>  
>    template <typename Method>
>    using TakeArgs = detail::TakeArgs<Method>;
>  
> +  void PruneListeners() {
> +    for (int32_t i = mListeners.Length() - 1; i >= 0; --i) {

Length() is unsigned and possibly 64-bit, you decrement it and assign it to a signed 32-bit.
Even though it should just work as intended, I'd feel better if you did an explicit cast to int32_t first; or please find another type-safe way to iterate.
Attachment #8764137 - Flags: review?(gsquelart) → review+
Comment on attachment 8764138 [details]
Bug 1281090. Part 5 - don't lock while sending synchronous notification to avoid deadlock. .

https://reviewboard.mozilla.org/r/60192/#review57036

r+ with minor nits/questions:

::: dom/media/MediaEventSource.h:520
(Diff revision 1)
> +  typename EnableIf<P == DispatchPolicy::Sync, void>::Type
> +  NotifyInternal(IntegralConstant<DispatchPolicy, P>, Ts&&... aEvents) {
> +    // Move |mListeners| to a new container before iteration to prevent
> +    // |mListeners| from being disrupted if the listener calls Connect() to
> +    // modify |mListeners| in the callback function.
> +    nsTArray<UniquePtr<Listener>> listeners = Move(mListeners);

Not my favorite thing, but from discussions in m.d.platform, I thought it was decided that Move() should only be applied when the moved-from object wouldn't be used anymore?
(For example, if mListener was some kind of array object with embedded storage, it could choose to copy its elements and therefore wouldn't get emptied when moved-from.)

You could use SwapElements() instead.

::: dom/media/MediaEventSource.h:521
(Diff revision 1)
> +  NotifyInternal(IntegralConstant<DispatchPolicy, P>, Ts&&... aEvents) {
> +    // Move |mListeners| to a new container before iteration to prevent
> +    // |mListeners| from being disrupted if the listener calls Connect() to
> +    // modify |mListeners| in the callback function.
> +    nsTArray<UniquePtr<Listener>> listeners = Move(mListeners);
> +    for (auto&& l : listeners) {

I don't understand the use of 'auto&&' in this loop and the next. Why not just 'auto&'?

::: dom/media/MediaEventSource.h:530
(Diff revision 1)
> +    // Perform clean-up and sanity checks.
> +    PruneListeners();

I think you could move this PruneListeners just before the 2nd loop, so that it won't need to go through the non-revoked elements that that loop may add.

::: dom/media/MediaEventSource.h:574
(Diff revision 1)
>   */
>  template <>
>  class MediaEventProducer<void> : public MediaEventSource<void> {
>  public:
>    void Notify() {
> -    this->NotifyInternal(true /* dummy */);
> +    MediaEventSource<void>::Notify(true /* dummy */);

Fly-by suggestion: Why not pass 'false' instead, which is 0 and may be easier to optimize? (But I'm not sure if that's true these days!)
Attachment #8764138 - Flags: review?(gsquelart) → review+
https://reviewboard.mozilla.org/r/60192/#review57036

> I don't understand the use of 'auto&&' in this loop and the next. Why not just 'auto&'?

auto&& works like perfect forwarding so I don't need to worry about whether it is an rvalue or lvalue to bind.

> I think you could move this PruneListeners just before the 2nd loop, so that it won't need to go through the non-revoked elements that that loop may add.

Will do.

> Fly-by suggestion: Why not pass 'false' instead, which is 0 and may be easier to optimize? (But I'm not sure if that's true these days!)

Will do.
Comment on attachment 8764136 [details]
Bug 1281090. Part 3 - replace MediaDecoderReaderWrapper::Set{Audio,Video}Callback with MediaCallback. .

https://reviewboard.mozilla.org/r/60188/#review57044

::: dom/media/AccurateSeekTask.cpp:629
(Diff revision 1)
> -                                    &AccurateSeekTask::OnAudioNotDecoded);
> -
> -  mVideoCallbackID =
> -    mReader->SetVideoCallback(this, &AccurateSeekTask::OnVideoDecoded,
> -                                    &AccurateSeekTask::OnVideoNotDecoded);
> +    if (aData.is<MediaData*>()) {
> +      OnAudioDecoded(aData.as<MediaData*>());
> +    } else {
> +      OnAudioNotDecoded(aData.as<MediaDecoderReader::NotDecodedReason>());
> +    }

http://searchfox.org/mozilla-central/source/mfbt/Variant.h#353 suggests using a matcher insted of going through each variant type with is<T>(). However, since we have only two types and should will only be, it should be fine.

::: dom/media/MediaDecoderReaderWrapper.h:15
(Diff revision 1)
>  #include "mozilla/AbstractThread.h"
>  #include "mozilla/RefPtr.h"
>  #include "nsISupportsImpl.h"
>  
>  #include "MediaDecoderReader.h"
> -#include "MediaCallbackID.h"
> +#include "MediaEventSource.h"

We can just remove "MediaCallbackID.h", I think.

::: dom/media/MediaDecoderReaderWrapper.cpp
(Diff revision 1)
> -void
>  MediaDecoderReaderWrapper::RequestAudioData()
>  {
>    MOZ_ASSERT(mOwnerThread->IsCurrentThreadIn());
>    MOZ_ASSERT(!mShutdown);
> -  MOZ_ASSERT(mRequestAudioDataCB, "Request audio data without callback!");

Shouldn't we keep this check? Same to the RequestVideoData() and WaitForData().

::: dom/media/MediaDecoderStateMachine.cpp:936
(Diff revision 1)
>  
> -  mWaitVideoCallbackID =
> -    mReader->SetWaitVideoCallback(
> -      [self] (MediaData::Type aType) -> void {
> -        self->EnsureVideoDecodeTaskQueued();
> -      },
> +  mVideoCallback = mReader->VideoCallback().Connect(
> +    mTaskQueue, [this] (VideoCallbackData aData) {
> +    typedef Tuple<MediaData*, TimeStamp> Type;
> +    if (aData.is<Type>()) {
> +      auto&& v = aData.as<Type>();

Not sure why do you use a universal reference here, the as<T>() always return a l-value reference, so the auto&& will always be deduced to be T&, right?
Attachment #8764136 - Flags: review?(kaku) → review+
https://reviewboard.mozilla.org/r/60188/#review57044

> We can just remove "MediaCallbackID.h", I think.

I will remove the file in next clean-up bug.

> Shouldn't we keep this check? Same to the RequestVideoData() and WaitForData().

For now there is no way to check the number of listeners connected to an event source.

> Not sure why do you use a universal reference here, the as<T>() always return a l-value reference, so the auto&& will always be deduced to be T&, right?

See the explanation of comment 14.
Comment on attachment 8764134 [details]
Bug 1281090. Part 1 - rename ListenerMode to ListenerPolicy. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60184/diff/1-2/
Comment on attachment 8764135 [details]
Bug 1281090. Part 2 - add synchronous notification to MediaEventSourceImpl. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60186/diff/1-2/
Comment on attachment 8764136 [details]
Bug 1281090. Part 3 - replace MediaDecoderReaderWrapper::Set{Audio,Video}Callback with MediaCallback. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60188/diff/1-2/
Comment on attachment 8764137 [details]
Bug 1281090. Part 4 - prune disconnected listeners more aggressively to avoid hitting the assertion. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60190/diff/1-2/
Comment on attachment 8764138 [details]
Bug 1281090. Part 5 - don't lock while sending synchronous notification to avoid deadlock. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60192/diff/1-2/
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f95809c26eb2
Part 1 - rename ListenerMode to ListenerPolicy. r=gerald.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e1fb308cf51
Part 2 - add synchronous notification to MediaEventSourceImpl. r=gerald.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3f947956594
Part 3 - replace MediaDecoderReaderWrapper::Set{Audio,Video}Callback with MediaCallback. r=kaku.
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb37efe61494
Part 4 - prune disconnected listeners more aggressively to avoid hitting the assertion. r=gerald.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb5b05298007
Part 5 - don't lock while sending synchronous notification to avoid deadlock. r=gerald.
See Also: → 1360423
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: