Closed Bug 1418213 Opened 2 years ago Closed 2 years ago

Always run MediaCacheStream::NotifyDataReceived() off the main thread

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

58 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(1 file)

https://searchfox.org/mozilla-central/rev/9bab9dc5a9472e3c163ab279847d2249322c206e/dom/media/ChannelMediaResource.cpp#304-311

ChannelMediaResource::OnDataAvailable() will run on the main thread if retarget->RetargetDeliveryTo() failed. We want to dispatch an event to run MediaCacheStream::NotifyDataReceived() off the main thread if this is the case.
Assignee: nobody → jwwang
Blocks: 1369263
Priority: -- → P3
Attachment #8929967 - Flags: review?(bechen)
Comment on attachment 8929967 [details]
Bug 1418213 - always run MediaCacheStream::NotifyDataReceived() off the main thread.

https://reviewboard.mozilla.org/r/201132/#review206606
Attachment #8929967 - Flags: review?(bechen) → review+
Attachment #8929967 - Flags: review?(gsquelart)
Comment on attachment 8929967 [details]
Bug 1418213 - always run MediaCacheStream::NotifyDataReceived() off the main thread.

https://reviewboard.mozilla.org/r/201132/#review206642

::: dom/media/ChannelMediaResource.cpp:439
(Diff revision 1)
> +    Data(Data&& aOther)
> +      : mResource(aOther.mResource.forget())
> +      , mData(Move(aOther.mData))
> +    {
> +    }

I think using Move(aOther.mResource) instead of aOther.mResource.forget() would be more appropriate here, and possibly more efficient.
In which case you may as well just make the whole move constructor `= default`.
Attachment #8929967 - Flags: review?(gsquelart) → review+
Comment on attachment 8929967 [details]
Bug 1418213 - always run MediaCacheStream::NotifyDataReceived() off the main thread.

https://reviewboard.mozilla.org/r/201132/#review206642

> I think using Move(aOther.mResource) instead of aOther.mResource.forget() would be more appropriate here, and possibly more efficient.
> In which case you may as well just make the whole move constructor `= default`.

Thanks. That's a good idea. However, the code would be tidier if move capture in lambda has been supported.
Comment on attachment 8929967 [details]
Bug 1418213 - always run MediaCacheStream::NotifyDataReceived() off the main thread.

https://reviewboard.mozilla.org/r/201132/#review206642

> Thanks. That's a good idea. However, the code would be tidier if move capture in lambda has been supported.

But is it! C++14 has now been available for a full 4 days! :-) See bug 1325632 and bug 1418047.
Thanks for the reviews!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ed68d2d0cb05
always run MediaCacheStream::NotifyDataReceived() off the main thread. r=bechen,gerald
https://hg.mozilla.org/mozilla-central/rev/ed68d2d0cb05
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.