1672072 - Make creation of decoder an asynchronous API

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

Currently, to create a decoder we call the PDMFactory::CreateDecoder and this is a synchronous call.

The PDMFactory will sequentially queries all registered PDM to determine which one supports the type we need.

For this the PDMFactory will call PlatformDecoderModule::Supports on each PDM.

The forces Supports to be a sync API and the RemoteDecoderModule will need a Supports sync IPC which isn't very desirable and has negative performance impact.

The primary aim is to make the RemoteDecoderManagerChild::Supports be asynchronous ; and to achieve this we need each potential caller to also be asynchronous, starting with PDMFactory::CreateDecoder.

This bug will track that progress.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1672072 - P1. Add async wrapper. r=mattwoodrow!,bryce!,mjf!,padenot!

This method will become the main entry point of a PDM and allow to create a decoder asynchronously for PDM not supporting that interface.

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1672072 - P2. Remove unused DecoderDoctorDiagnostic from CreateDecoderParam. r=bryce!,padenot!

The DecoderDoctorDiagnostic is only ever used with PDM::Supports; when creating a decoder this object would always be nullptr.
Attempting to set it would have resulted in UAF as a DiagnosticDecoderDecoder is typically an object allocated on the stack and most of the code creating the decoders is async.

Depends on D96347

Comment 3

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1672072 - P3. Remove IRemoteDecoderChild.h class. r=mattwoodrow!,padenot!,mjf!,bryce!

There's no longer have a GpuDecoderModule and RemoteGpuDecoder.

So we no longer need the IRemoteDecoderChild abstraction layer.

Depends on D96348

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1672072 - P4. Pass lambda rather than raw pointer to object. r=mattwoodrow!,bryce!,padenot!

Passing a pointer to an object across several thread was dubious at best to ensure a use after free was never in place.
This complexity becomes even greater should we want to create the decoder asynchronously.

So we pass instead a lambda that holds a strong reference to the object that holds the event producer.

Depends on D96349

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1672072 - P1. Add async wrapper. r=mattwoodrow!,bryce!,mjf!,padenot!

This method will become the main entry point of a PDM and allow to create a decoder asynchronously for PDM not supporting that interface.

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1672072 - P2. Remove unused DecoderDoctorDiagnostic from CreateDecoderParam. r=bryce!,padenot!

The DecoderDoctorDiagnostic is only ever used with PDM::Supports; when creating a decoder this object would always be nullptr.
Attempting to set it would have resulted in UAF as a DiagnosticDecoderDecoder is typically an object allocated on the stack and most of the code creating the decoders is async.

Depends on D96351

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1672072 - P3. Remove IRemoteDecoderChild.h class. r=mattwoodrow!,padenot!,mjf!,bryce!

There's no longer have a GpuDecoderModule and RemoteGpuDecoder.

So we no longer need the IRemoteDecoderChild abstraction layer.

Depends on D96352

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1672072 - P4. Pass lambda rather than raw pointer to object. r=mattwoodrow!,bryce!,padenot!

Passing a pointer to an object across several thread was dubious at best to ensure a use after free was never in place.
This complexity becomes even greater should we want to create the decoder asynchronously.

So we pass instead a lambda that holds a strong reference to the object that holds the event producer.

Depends on D96353

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1672072 - P6. Don't attempt to create a video decoder if we don't have a KnowsCompositor set. r=mattwoodrow!,bryce!,mjf!,padenot!

Comment 10

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1672072 - P1. Add async wrapper. r=mattwoodrow!,bryce!,mjf!,padenot!

This method will become the main entry point of a PDM and allow to create a decoder asynchronously for PDM not supporting that interface.

Comment 11

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1672072 - P2. Remove unused DecoderDoctorDiagnostic from CreateDecoderParam. r=mattwoodrow!,bryce!,padenot!

The DecoderDoctorDiagnostic is only ever used with PDM::Supports; when creating a decoder this object would always be nullptr.
Attempting to set it would have resulted in UAF as a DiagnosticDecoderDecoder is typically an object allocated on the stack and most of the code creating the decoders is async.

Comment 12

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1672072 - P3. Remove IRemoteDecoderChild.h class. r=mattwoodrow!,padenot!,mjf!,bryce!

There's no longer have a GpuDecoderModule and RemoteGpuDecoder.

So we no longer need the IRemoteDecoderChild abstraction layer.

Comment 13

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1672072 - P4. Pass lambda rather than raw pointer to object. r=mattwoodrow!,bryce!,padenot!

Passing a pointer to an object across several thread was dubious at best to ensure a use after free was never in place.
This complexity becomes even greater should we want to create the decoder asynchronously.

So we pass instead a lambda that holds a strong reference to the object that holds the event producer.

Comment 14

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1672072 - P5. Add CreateDecoderParamsForAsync struct. r=mattwoodrow!,bryce!,padenot!

For now, creation of the decoders is a synchronous operation, so we can deal with a convenience, stack-only structure to pass all the parameters required.
This will change once creation becomes asynchronous and we must ensure that the parameters lifetime will overlap the entire process.

It also allows to simplify the few areas that did have to manually copy all those parameters and save them individually.

Comment 15

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1672072 - P6. Don't attempt to create a video decoder if we don't have a KnowsCompositor set. r=mattwoodrow!,bryce!,mjf!,padenot!

Comment 16

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1672072 - P7. Remove the guarantee that the TrackInfo reference will always be valid. r=mattwoodrow!,bryce!,padenot!

Until now, it was up to the caller to ensure that the VideoInfo or AudioInfo passed to the PlatformDecoderModule would remain alive until the created decoder was shut down.
This was achieved by the MediaFormatReader through great care and complexity and ensuring that the MediaFormatReader would never complete its own shutdown unless the decoder did the same.

Asynchronously creating the decoder add an even greater of complexity. We have to ensure that should the caller shut down while the decoder creation promise hasn't been resolved ; that it stays alive the entire time.

It could be achieved by making the TrackInfo object refcounted, or creating a cycle between the caller and the PDM/decoder.

Each of those options have their own downsides.

Either of those options add an unwarranted level of complexity, when we can just have each decoder copy the arguments it receives. This is simpler and more straightforward and the required copy isn't expensive.

Depends on D96362

Comment 17

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1672072 - P8. Create decoder asynchronously. r=mattwoodrow!,bryce!,padenot!,mjf!

PDMFactory::CreateDecoder is changed to return a MozPromise that will contain the MediaDataDecoder once created.

This will allow to later make RemoteDecoderManager fully asynchronous and no longer require an IPC sync call to start the RDD process.

We also modify the WebrtcMediaDataDecoderCodec to never create a decoder on the main thread, which could cause deadlocks under some circumstances.

Depends on D96363

Comment 18

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1672072 - P9. Make EnsureRDDProcessAndCreateBridge an async API. r=mattwoodrow!,bryce!,mjf!,padenot!

We can now chaincreation of the decoder to the launch of the RDD process as needed and setting up the PRemoteDecoderManager

Depends on D96364

Comment 19

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1672072 - P10. Promisify launch of RDD process and remove sync dispatch. r=mattwoodrow!,bryce!,mjf!,padenot!

Depends on D96365

Comment 20

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1672072 - P11. Make MediaFormatReader support ThreadSafeWeakPtr. r=mattwoodrow!,bryce!,padenot!

P4 introduced a cycle between the MediaFormatReader and the onWaitingForKeyEvent lambda.
Normally, this cycle would be broken by a call to Shutdown.

However, should the MediaFormatReader shuts down before the CreateDecoderPromise be resolved, it would have no way of shutting down the decoder keeping the cycle alive.

Theoretically, keeping a plain pointer to the MFR::mOnTrackWaitingForKey like we did before P4 is safe, as the EME code making use of it, only does so while we are decoding data and the MFR has the ability to properly shutdown the decoder.

However, for the sake of clarity and simplification, we introduce an easier way to check of the MFR has been deleted already by making it support SafeThreadWeakPtr; and the OnWaitingForKey keep a weak pointer to it and checking the value before calling it.

Depends on D96366

Comment 21

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1672072 - P12. Add constructor for WeakPtr(RefPtr). r?sg,gerald

This allows for nice template type deducation.

Comment 22

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1672072 - P13. Always shut down decoder after creation. r=mattwoodrow!,bryce!,padenot!

We also ensure that the MediaFormatReader will only shutdown once the CreateDecoderPromise has been resolved (or rejected)

This makes a few of the earlier changes redundant; as we can now guarantee all the objects provided to the PDMFactory will stay valid for the entire lifetime of the decoder. So we could do away with P4 and P7.

It's better that we keep those however, as it greatly simplify other usage of the PDMFactory. Ensuring that the user of the PDMFactory survives any decoder isn't a trivial affair.

Depends on D96368

Comment 23

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1672072 - P12. Add constructor for WeakPtr(RefPtr). r?sg,gerald

This allows for nice template argument deducation.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65cd3b9de5c6
P1. Add async wrapper. r=mattwoodrow,bryce,mjf,padenot
https://hg.mozilla.org/integration/autoland/rev/fbf0b5117655
P2. Remove unused DecoderDoctorDiagnostic from CreateDecoderParam. r=mattwoodrow,bryce,padenot
https://hg.mozilla.org/integration/autoland/rev/1ef9d6d10508
P3. Remove IRemoteDecoderChild.h class. r=mattwoodrow,padenot,mjf,bryce
https://hg.mozilla.org/integration/autoland/rev/b59655b7a595
P4. Pass lambda rather than raw pointer to object. r=mattwoodrow,bryce,padenot
https://hg.mozilla.org/integration/autoland/rev/6f1d99e9c3a1
P5. Add CreateDecoderParamsForAsync struct. r=mattwoodrow,bryce,padenot
https://hg.mozilla.org/integration/autoland/rev/9905650335ef
P6. Don't attempt to create a video decoder if we don't have a KnowsCompositor set. r=mattwoodrow,bryce,mjf,padenot
https://hg.mozilla.org/integration/autoland/rev/09adad7416e1
P7. Remove the guarantee that the TrackInfo reference will always be valid. r=mattwoodrow,bryce,padenot
https://hg.mozilla.org/integration/autoland/rev/9cb71fd4b43b
P8. Create decoder asynchronously. r=mattwoodrow,bryce,padenot,mjf,ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/89d9a12c0737
P9. Make EnsureRDDProcessAndCreateBridge an async API. r=mattwoodrow,bryce,mjf,padenot,ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/091b9449c786
P10. Promisify launch of RDD process and remove sync dispatch. r=mattwoodrow,bryce,mjf,padenot
https://hg.mozilla.org/integration/autoland/rev/ff8fe1b856b3
P11. Make MediaFormatReader support ThreadSafeWeakPtr. r=mattwoodrow,bryce,padenot
https://hg.mozilla.org/integration/autoland/rev/28db8276ec2b
P12. Add constructor for WeakPtr(RefPtr). r=sg
https://hg.mozilla.org/integration/autoland/rev/f093b7969e8b
P13. Always shut down decoder after creation. r=mattwoodrow,bryce,padenot

Comment 25

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out 13 changesets (bug 1672072) for short.mp4.firstframe.html.

Push with failure: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=Oeu3ymwbRU2BNiSpJEn77A.0&fromchange=36d64f1d9d3dff687e2a5dc8871a5e3cb858a7ff&searchStr=os%2Cx%2C10.14%2Cwebrender%2Copt%2Creftests%2Ctest-macosx1014-64-qr%2Fopt-reftest-e10s%2Cr7&tochange=23b8e2facfb9860427b51b8fbad686def2d2ac29

Backout link: https://hg.mozilla.org/integration/autoland/rev/23b8e2facfb9860427b51b8fbad686def2d2ac29

Failure log: https://treeherder.mozilla.org/logviewer?job_id=321661300&repo=autoland&lineNumber=7781

[task 2020-11-13T03:59:07.474Z] 03:59:07     INFO - REFTEST TEST-LOAD | http://localhost:51404/1605239947464/1/reftest/short.mp4.firstframe.html | 0 / 8 (0%)
[task 2020-11-13T03:59:07.591Z] 03:59:07     INFO - REFTEST INFO | drawWindow flags = DRAWWINDOW_DRAW_CARET | DRAWWINDOW_DRAW_VIEW | DRAWWINDOW_USE_WIDGET_LAYERS; window size = 800,1000; test browser size = 800,1000
[task 2020-11-13T03:59:07.674Z] 03:59:07     INFO - _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.
[task 2020-11-13T03:59:08.139Z] 03:59:08     INFO - REFTEST TEST-LOAD | http://localhost:51404/1605239947464/1/reftest/short.mp4.firstframe-ref.html | 0 / 8 (0%)
[task 2020-11-13T03:59:08.139Z] 03:59:08     INFO - JavaScript error: resource://gre/actors/PictureInPictureChild.jsm, line 452: TypeError: can't access property "requestIdleCallback", this.contentWindow is null
[task 2020-11-13T03:59:08.139Z] 03:59:08     INFO - REFTEST INFO | REFTEST fuzzy test (0, 0) <= (80, 76800) <= (68, 76800)
[task 2020-11-13T03:59:08.232Z] 03:59:08     INFO - REFTEST TEST-UNEXPECTED-FAIL | dom/media/test/reftest/short.mp4.firstframe.html == dom/media/test/reftest/short.mp4.firstframe-ref.html | image comparison, max difference: 80, number of differing pixels: 76800
[task 2020-11-13T03:59:08.232Z] 03:59:08     INFO - REFTEST   IMAGE 1 (TEST): data:image/png;base64,iVBORw0K...
[task 2020-11-13T03:59:08.232Z] 03:59:08     INFO - REFTEST   IMAGE 2 (REFERENCE): data:image/png;base64,iVBORw0K...
[task 2020-11-13T03:59:08.233Z] 03:59:08     INFO - REFTEST TEST-END | dom/media/test/reftest/short.mp4.firstframe.html == dom/media/test/reftest/short.mp4.firstframe-ref.html
[task 2020-11-13T03:59:08.233Z] 03:59:08     INFO - REFTEST TEST-START | dom/media/test/reftest/short.mp4.lastframe.html == dom/media/test/reftest/short.mp4.lastframe-ref.html
[task 2020-11-13T03:59:08.233Z] 03:59:08     INFO - REFTEST TEST-LOAD | http://localhost:51404/1605239947464/2/reftest/short.mp4.lastframe.html | 1 / 8 (12%)
[task 2020-11-13T03:59:08.593Z] 03:59:08     INFO - REFTEST TEST-LOAD | http://localhost:51404/1605239947464/2/reftest/short.mp4.lastframe-ref.html | 1 / 8 (12%)
[task 2020-11-13T03:59:08.596Z] 03:59:08     INFO - JavaScript error: resource://gre/actors/PictureInPictureChild.jsm, line 568: TypeError: can't access property "removeEventListener", this.contentWindow is null
[task 2020-11-13T03:59:08.641Z] 03:59:08     INFO - REFTEST INFO | REFTEST fuzzy test (0, 0) <= (87, 76795) <= (60, 76797)
[task 2020-11-13T03:59:08.801Z] 03:59:08     INFO - REFTEST TEST-UNEXPECTED-FAIL | dom/media/test/reftest/short.mp4.lastframe.html == dom/media/test/reftest/short.mp4.lastframe-ref.html | image comparison, max difference: 87, number of differing pixels: 76795
[task 2020-11-13T03:59:08.801Z] 03:59:08     INFO - REFTEST   IMAGE 1 (TEST): data:image/png;base64,iVBORw0KGgoAAAANSUhE...
[task 2020-11-13T03:59:08.801Z] 03:59:08     INFO - REFTEST   IMAGE 2 (REFERENCE): data:image/png;base64,iVBORw0KG...
[task 2020-11-13T03:59:08.802Z] 03:59:08     INFO - REFTEST TEST-END | dom/media/test/reftest/short.mp4.lastframe.html == dom/media/test/reftest/short.mp4.lastframe-ref.html

Comment 26

[Bogdan Tara[:bogdan_tara | bogdant]]

The following seems to start perma failing with the backed out changes: https://treeherder.mozilla.org/logviewer?job_id=321656073&repo=autoland&lineNumber=1793

Comment 27

[Jean-Yves Avenard [:jya]]

(In reply to Bogdan Tara[:bogdan_tara | bogdant] from comment #26)

The following seems to start perma failing with the backed out changes: https://treeherder.mozilla.org/logviewer?job_id=321656073&repo=autoland&lineNumber=1793

doesn't seem so, on that same push there's two run of that job, and the 2nd one is green

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/781cbe96daef
P1. Add async wrapper. r=mattwoodrow,bryce,mjf,padenot
https://hg.mozilla.org/integration/autoland/rev/df40384bc11a
P2. Remove unused DecoderDoctorDiagnostic from CreateDecoderParam. r=mattwoodrow,bryce,padenot
https://hg.mozilla.org/integration/autoland/rev/070e63e03b0d
P3. Remove IRemoteDecoderChild.h class. r=mattwoodrow,padenot,mjf,bryce
https://hg.mozilla.org/integration/autoland/rev/1ebf3b578f0e
P4. Pass lambda rather than raw pointer to object. r=mattwoodrow,bryce,padenot
https://hg.mozilla.org/integration/autoland/rev/5f32a75ad1be
P5. Add CreateDecoderParamsForAsync struct. r=mattwoodrow,bryce,padenot
https://hg.mozilla.org/integration/autoland/rev/b014e619965f
P6. Don't attempt to create a video decoder if we don't have a KnowsCompositor set. r=mattwoodrow,bryce,mjf,padenot
https://hg.mozilla.org/integration/autoland/rev/17a5d812f4cb
P7. Remove the guarantee that the TrackInfo reference will always be valid. r=mattwoodrow,bryce,padenot
https://hg.mozilla.org/integration/autoland/rev/2ad5d48f1452
P8. Create decoder asynchronously. r=mattwoodrow,bryce,padenot,mjf,ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/134879789eca
P9. Make EnsureRDDProcessAndCreateBridge an async API. r=mattwoodrow,bryce,mjf,padenot,ipc-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/d6c6d6fc3918
P10. Promisify launch of RDD process and remove sync dispatch. r=mattwoodrow,bryce,mjf,padenot
https://hg.mozilla.org/integration/autoland/rev/3bad20fc960d
P11. Make MediaFormatReader support ThreadSafeWeakPtr. r=mattwoodrow,bryce,padenot
https://hg.mozilla.org/integration/autoland/rev/e938ec569adb
P12. Add constructor for WeakPtr(RefPtr). r=sg
https://hg.mozilla.org/integration/autoland/rev/7c39a1568c5e
P13. Always shut down decoder after creation. r=mattwoodrow,bryce,padenot

Comment 29

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/781cbe96daef
https://hg.mozilla.org/mozilla-central/rev/df40384bc11a
https://hg.mozilla.org/mozilla-central/rev/070e63e03b0d
https://hg.mozilla.org/mozilla-central/rev/1ebf3b578f0e
https://hg.mozilla.org/mozilla-central/rev/5f32a75ad1be
https://hg.mozilla.org/mozilla-central/rev/b014e619965f
https://hg.mozilla.org/mozilla-central/rev/17a5d812f4cb
https://hg.mozilla.org/mozilla-central/rev/2ad5d48f1452
https://hg.mozilla.org/mozilla-central/rev/134879789eca
https://hg.mozilla.org/mozilla-central/rev/d6c6d6fc3918
https://hg.mozilla.org/mozilla-central/rev/3bad20fc960d
https://hg.mozilla.org/mozilla-central/rev/e938ec569adb
https://hg.mozilla.org/mozilla-central/rev/7c39a1568c5e

Comment 30

[Ryan VanderMeulen [:RyanVM]]

This seems like a lot to land during a soft freeze. Was there a reason it couldn't wait until after the 85 version bump?

Comment 31

[Jean-Yves Avenard [:jya]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #30)

This seems like a lot to land during a soft freeze. Was there a reason it couldn't wait until after the 85 version bump?

Either we landed this or we would have to backout all the work done on RDD in 84. In particular bug 1595994.
I felt this was the safest approach.