1281632 - Refactor create MediaDataDecoder arguments into CreateDecoderParams struct

Status: RESOLVED FIXED

Description

[Chris Pearce [:cpearce (GMT+13)]]

To make it easier to add arguments to PDM::Create*Decoder(), we can change the signature of those functions to instead of taking a list of arguments, take a struct which we fill out with the arguments.

We can also clean up the mozilla::GMP{Audio,Video}Decoder constructors.

This is split out of Dan's patches from bug 1267918.

Comment 1

[Chris Pearce [:cpearce (GMT+13)]]

Created attachment 8764406 [details]
Bug 1281632 - P1: Extract creation parameters and pass via struct.

Extract all the parameters passed to CreateAudioDecoder/CreateVideoDecoder and
place them into a structure that is passed down to the creation of the actually
decoder, where the relevant parameters can be extracted.

This makes it easier to add more arguments to the Create*Decoder calls in future.

Review commit: https://reviewboard.mozilla.org/r/60312/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60312/

Comment 2

[Chris Pearce [:cpearce (GMT+13)]]

Created attachment 8764407 [details]
Bug 1281632 - P2: Reduce GMPAudioDecoder/GMPVideoDecoder ctor count.

This means we expose fewer constructors, and the definition is in the .cpp file
so changing it won't cause lots of things to recompile.

Review commit: https://reviewboard.mozilla.org/r/60314/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60314/

Comment 3

[Chris Pearce [:cpearce (GMT+13)]]

Created attachment 8764408 [details]
Bug 1281632 - P3: Move EMEAudioDecoder/EMEVideoDecoder ctors.

Review commit: https://reviewboard.mozilla.org/r/60316/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60316/

Comment 4

[Chris Pearce [:cpearce (GMT+13)]]

Created attachment 8764409 [details]
Bug 1281632 - P4: Add VideoInfo ctor that takes nsIntSize.

Review commit: https://reviewboard.mozilla.org/r/60318/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60318/

Comment 5

[Chris Pearce [:cpearce (GMT+13)]]

Created attachment 8764410 [details]
Bug 1281632 - P5: Extract parameters to GMPVideoDecoder into struct.

Review commit: https://reviewboard.mozilla.org/r/60320/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60320/

Comment 6

[Chris Pearce [:cpearce (GMT+13)]]

Created attachment 8764411 [details]
Bug 1281632 - P6: Extract parameters to GMPAudioDecoder into struct.

Review commit: https://reviewboard.mozilla.org/r/60322/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60322/

Comment 7

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764410 [details]
Bug 1281632 - P5: Extract parameters to GMPVideoDecoder into struct.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60320/diff/1-2/

Comment 8

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764411 [details]
Bug 1281632 - P6: Extract parameters to GMPAudioDecoder into struct.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60322/diff/1-2/

Comment 9

[Gerald Squelart [:gerald]]

https://reviewboard.mozilla.org/r/60312/#review57150

r+ with the fix to the commit description, and the one issue below. I'll let you decide about the suggestions (now/later/never).

In the commit description: 'actually' -> 'actual'.

::: dom/media/MediaFormatReader.cpp:408
(Diff revision 1)
>    switch (aTrack) {
> -    case TrackType::kAudioTrack:
> +  case TrackType::kAudioTrack: {

According to our coding style guide, 'case' statements should be indented relative to their 'switch'.
(Check your IDE settings, as it may have unindented this block without you noticing!)

::: dom/media/platforms/PlatformDecoderModule.h:35
(Diff revision 1)
> +struct CreateDecoderParams {
> +  explicit CreateDecoderParams(const TrackInfo& aConfig)

Because all members of CreateDecoderParams have distinct types, it is possible to use variadic templates to more succinctly construct a params struct in one call.
I'll let you decide whether you want to implement this here.

Pros:
- Concise calls.
For example the 4 lines in Benchmark.cpp become a one-liner:
    mDecoder = platform->CreateDecoder({aInfo, mDecoderTaskQueue, reinterpret_cast<MediaDataDecoderCallback*>(this)});
(The reinterpret cast is needed in this case because of private inheritance.)
- New member types will only need to have a corresponding 'Set()' method added.
- Variadic templates are all the rage!

Cons:
- Call sites might be a bit confusing to readers, since they may receive inconsistent argument lists. But I would think not more confusing than they already were.
- If in the future the struct contains two members sharing the same type, this trick won't work for them and they'll require distinct setting.
- In some cases, casts may be required at call sites.

We could choose to implement this in a later bug, but it would mean revisiting all call sites there again, so I would suggest we do it here while we're touching all these files.

Proposed implementation:
    template <typename T1, typename... Ts>
    CreateDecoderParams(const TrackInfo& aConfig, T1 a1, Ts... as)
      : mConfig(aConfig)
      , mTaskQueue(nullptr)
      , mCallback(nullptr)
      , mDiagnostics(nullptr)
      , mImageContainer(nullptr)
      , mLayersBackend(layers::LayersBackend::LAYERS_NONE)
    {
      Set(a1, as...);
    }
    // Set's for 1 argument.
    void Set(TaskQueue* aTaskQueue) { mTaskQueue = aTaskQueue; }
    void Set(MediaDataDecoderCallback* aCallback) { mCallback = aCallback; }
    void Set(DecoderDoctorDiagnostics* aDiagnostics) { mDiagnostics = aDiagnostics; }
    void Set(layers::ImageContainer* aImageContainer) { mImageContainer = aImageContainer; }
    void Set(layers::LayersBackend aLayersBackend) { mLayersBackend = aLayersBackend; }
    // Set for 2 or more arguments.
    template <typename T1, typename T2, typename... Ts>
    void Set(T1 a1, T2 a2, Ts... as)
    {
      // Parameter pack expansion trick, to call Set() on each argument.
      using expander = int[];
      (void)expander{(Set(a1), 0), (Set(a2), 0), (Set(as), 0)...};
    }

::: dom/media/platforms/agnostic/AgnosticDecoderModule.cpp:30
(Diff revision 1)
> -  if (VPXDecoder::IsVPX(aConfig.mMimeType)) {
> -    m = new VPXDecoder(*aConfig.GetAsVideoInfo(),
> +  const TrackInfo& config = aParams.mConfig;
> +  if (VPXDecoder::IsVPX(config.mMimeType)) {

I see you've used that pattern a lot, where you first reference an aParams member with an explicit type, and then use it through that reference.

Instead you could just collapse these in one statement, e.g. in this case:
    if (IsVPX(aParams.mConfig.mMimeType)) {

It's just a style preference I suppose, I thought some of these cases don't really lose their maintainability, but I'm not fuss either way...

::: dom/media/platforms/agnostic/AgnosticDecoderModule.cpp:43
(Diff revision 1)
> -  if (VorbisDataDecoder::IsVorbis(aConfig.mMimeType)) {
> -    m = new VorbisDataDecoder(*aConfig.GetAsAudioInfo(),
> +  const TrackInfo& config = aParams.mConfig;
> +  if (VorbisDataDecoder::IsVorbis(config.mMimeType)) {

... In this case, since you use 'config.mMimeType' multiple times, you may want to create a reference for that whole thing.

Or just not create any reference, as I suggested above. ;-)

Comment 10

[Gerald Squelart [:gerald]]

Comment on attachment 8764406 [details]
Bug 1281632 - P1: Extract creation parameters and pass via struct.

https://reviewboard.mozilla.org/r/60312/#review57152

(forgot r+ before)

Comment 11

[Gerald Squelart [:gerald]]

Comment on attachment 8764407 [details]
Bug 1281632 - P2: Reduce GMPAudioDecoder/GMPVideoDecoder ctor count.

https://reviewboard.mozilla.org/r/60314/#review57154

Comment 12

[Gerald Squelart [:gerald]]

Comment on attachment 8764408 [details]
Bug 1281632 - P3: Move EMEAudioDecoder/EMEVideoDecoder ctors.

https://reviewboard.mozilla.org/r/60316/#review57156

Comment 13

[Gerald Squelart [:gerald]]

Comment on attachment 8764409 [details]
Bug 1281632 - P4: Add VideoInfo ctor that takes nsIntSize.

https://reviewboard.mozilla.org/r/60318/#review57158

Comment 14

[Gerald Squelart [:gerald]]

Comment on attachment 8764410 [details]
Bug 1281632 - P5: Extract parameters to GMPVideoDecoder into struct.

https://reviewboard.mozilla.org/r/60320/#review57160

r+ with issue addressed (unless you don't think it makes sense).

::: dom/media/platforms/agnostic/gmp/GMPVideoDecoder.h:55
(Diff revision 2)
> +  explicit GMPVideoDecoderParams(const CreateDecoderParams& aParams);
> +  GMPVideoDecoderParams& WithCallback(MediaDataDecoderProxy* aWrapper);
> +  GMPVideoDecoderParams& WithAdapter(VideoCallbackAdapter* aAdapter);

Nice!

::: dom/media/platforms/agnostic/gmp/GMPVideoDecoder.cpp:116
(Diff revision 2)
> +GMPVideoDecoderParams&
> +GMPVideoDecoderParams::WithCallback(MediaDataDecoderProxy* aWrapper)
> +{
> +  MOZ_ASSERT(aWrapper);
> +  mCallback = aWrapper->Callback();
> +  mAdapter = nullptr;
> +  return *this;
> +}
> +
> +GMPVideoDecoderParams&
> +GMPVideoDecoderParams::WithAdapter(VideoCallbackAdapter* aAdapter)
> +{
> +  MOZ_ASSERT(aAdapter);
> +  mCallback = aAdapter->Callback();
> +  mAdapter = aAdapter;
> +  return *this;
> +}

Since not more than one of 'WithCallback' and 'WithAdapter' is intended to be used at construction time (right?), I think you should MOZ_ASSERT that mCallback and mAdapter are null in both, to prevent misuses.

Comment 15

[Gerald Squelart [:gerald]]

Comment on attachment 8764411 [details]
Bug 1281632 - P6: Extract parameters to GMPAudioDecoder into struct.

https://reviewboard.mozilla.org/r/60322/#review57162

::: dom/media/platforms/agnostic/gmp/GMPAudioDecoder.cpp:136
(Diff revision 2)
> +GMPAudioDecoderParams&
> +GMPAudioDecoderParams::WithCallback(MediaDataDecoderProxy* aWrapper)
> +{
> +  MOZ_ASSERT(aWrapper);
> +  mCallback = aWrapper->Callback();
> +  mAdapter = nullptr;
> +  return *this;
> +}
> +
> +GMPAudioDecoderParams&
> +GMPAudioDecoderParams::WithAdapter(AudioCallbackAdapter* aAdapter)
> +{
> +  MOZ_ASSERT(aAdapter);
> +  mCallback = aAdapter->Callback();
> +  mAdapter = aAdapter;
> +  return *this;
> +}

Same as the previous patch, I think you should MOZ_ASSERT(!mCallback && !mAdapter) to prevent misuses of WithX().

Comment 16

[Chris Pearce [:cpearce (GMT+13)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c78613e50506

Comment 17

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764406 [details]
Bug 1281632 - P1: Extract creation parameters and pass via struct.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60312/diff/1-2/

Comment 18

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764407 [details]
Bug 1281632 - P2: Reduce GMPAudioDecoder/GMPVideoDecoder ctor count.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60314/diff/1-2/

Comment 19

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764408 [details]
Bug 1281632 - P3: Move EMEAudioDecoder/EMEVideoDecoder ctors.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60316/diff/1-2/

Comment 20

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764409 [details]
Bug 1281632 - P4: Add VideoInfo ctor that takes nsIntSize.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60318/diff/1-2/

Comment 21

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764410 [details]
Bug 1281632 - P5: Extract parameters to GMPVideoDecoder into struct.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60320/diff/2-3/

Comment 22

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764411 [details]
Bug 1281632 - P6: Extract parameters to GMPAudioDecoder into struct.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60322/diff/2-3/

Comment 23

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764406 [details]
Bug 1281632 - P1: Extract creation parameters and pass via struct.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60312/diff/2-3/

Comment 24

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764407 [details]
Bug 1281632 - P2: Reduce GMPAudioDecoder/GMPVideoDecoder ctor count.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60314/diff/2-3/

Comment 25

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764408 [details]
Bug 1281632 - P3: Move EMEAudioDecoder/EMEVideoDecoder ctors.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60316/diff/2-3/

Comment 26

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764409 [details]
Bug 1281632 - P4: Add VideoInfo ctor that takes nsIntSize.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60318/diff/2-3/

Comment 27

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764410 [details]
Bug 1281632 - P5: Extract parameters to GMPVideoDecoder into struct.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60320/diff/3-4/

Comment 28

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764411 [details]
Bug 1281632 - P6: Extract parameters to GMPAudioDecoder into struct.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60322/diff/3-4/

Comment 29

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764406 [details]
Bug 1281632 - P1: Extract creation parameters and pass via struct.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60312/diff/3-4/

Comment 30

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764407 [details]
Bug 1281632 - P2: Reduce GMPAudioDecoder/GMPVideoDecoder ctor count.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60314/diff/3-4/

Comment 31

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764408 [details]
Bug 1281632 - P3: Move EMEAudioDecoder/EMEVideoDecoder ctors.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60316/diff/3-4/

Comment 32

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764409 [details]
Bug 1281632 - P4: Add VideoInfo ctor that takes nsIntSize.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60318/diff/3-4/

Comment 33

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764410 [details]
Bug 1281632 - P5: Extract parameters to GMPVideoDecoder into struct.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60320/diff/4-5/

Comment 34

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764411 [details]
Bug 1281632 - P6: Extract parameters to GMPAudioDecoder into struct.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60322/diff/4-5/

Comment 35

[Chris Pearce [:cpearce (GMT+13)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=577eeeeb0698

Comment 36

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764406 [details]
Bug 1281632 - P1: Extract creation parameters and pass via struct.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60312/diff/4-5/

Comment 37

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764407 [details]
Bug 1281632 - P2: Reduce GMPAudioDecoder/GMPVideoDecoder ctor count.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60314/diff/4-5/

Comment 38

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764408 [details]
Bug 1281632 - P3: Move EMEAudioDecoder/EMEVideoDecoder ctors.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60316/diff/4-5/

Comment 39

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764409 [details]
Bug 1281632 - P4: Add VideoInfo ctor that takes nsIntSize.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60318/diff/4-5/

Comment 40

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764410 [details]
Bug 1281632 - P5: Extract parameters to GMPVideoDecoder into struct.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60320/diff/5-6/

Comment 41

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8764411 [details]
Bug 1281632 - P6: Extract parameters to GMPAudioDecoder into struct.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60322/diff/5-6/

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf174e130188
P1: Extract creation parameters and pass via struct. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c41aac9acf8
P2: Reduce GMPAudioDecoder/GMPVideoDecoder ctor count. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/df7e22156381
P3: Move EMEAudioDecoder/EMEVideoDecoder ctors. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/5935f06b1545
P4: Add VideoInfo ctor that takes nsIntSize. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdbafe24a2b6
P5: Extract parameters to GMPVideoDecoder into struct. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/d84a9f97a46d
P6: Extract parameters to GMPAudioDecoder into struct. r=gerald

Comment 43

[Dan Glastonbury (:kamidphish) | needinfo me]

(In reply to Gerald Squelart [:gerald] from comment #14)
> > +  explicit GMPVideoDecoderParams(const CreateDecoderParams& aParams);
> > +  GMPVideoDecoderParams& WithCallback(MediaDataDecoderProxy* aWrapper);
> > +  GMPVideoDecoderParams& WithAdapter(VideoCallbackAdapter* aAdapter);
> 
> Nice!

Thank you.

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cf174e130188
https://hg.mozilla.org/mozilla-central/rev/6c41aac9acf8
https://hg.mozilla.org/mozilla-central/rev/df7e22156381
https://hg.mozilla.org/mozilla-central/rev/5935f06b1545
https://hg.mozilla.org/mozilla-central/rev/cdbafe24a2b6
https://hg.mozilla.org/mozilla-central/rev/d84a9f97a46d