Refactor create MediaDataDecoder arguments into CreateDecoderParams struct

RESOLVED FIXED in Firefox 50

Status

()

Core
Audio/Video: Playback
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

(Blocks: 1 bug)

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
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/
Attachment #8764406 - Flags: review?(gsquelart)
Attachment #8764407 - Flags: review?(gsquelart)
Attachment #8764408 - Flags: review?(gsquelart)
Attachment #8764409 - Flags: review?(gsquelart)
Attachment #8764410 - Flags: review?(gsquelart)
Attachment #8764411 - Flags: review?(gsquelart)
(Assignee)

Comment 2

2 years ago
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/
(Assignee)

Comment 3

2 years ago
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/
(Assignee)

Comment 4

2 years ago
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/
(Assignee)

Comment 5

2 years ago
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/
(Assignee)

Comment 6

2 years ago
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/
(Assignee)

Comment 7

2 years ago
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/
(Assignee)

Comment 8

2 years ago
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/
Blocks: 1263836
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. ;-)
Attachment #8764406 - Flags: review?(gsquelart) → review+
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 on attachment 8764407 [details]
Bug 1281632 - P2: Reduce GMPAudioDecoder/GMPVideoDecoder ctor count.

https://reviewboard.mozilla.org/r/60314/#review57154
Attachment #8764407 - Flags: review?(gsquelart) → review+
Comment on attachment 8764408 [details]
Bug 1281632 - P3: Move EMEAudioDecoder/EMEVideoDecoder ctors.

https://reviewboard.mozilla.org/r/60316/#review57156
Attachment #8764408 - Flags: review?(gsquelart) → review+
Comment on attachment 8764409 [details]
Bug 1281632 - P4: Add VideoInfo ctor that takes nsIntSize.

https://reviewboard.mozilla.org/r/60318/#review57158
Attachment #8764409 - Flags: review?(gsquelart) → review+
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.
Attachment #8764410 - Flags: review?(gsquelart) → review+
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().
Attachment #8764411 - Flags: review?(gsquelart) → review+
Priority: -- → P2
(Assignee)

Comment 17

2 years ago
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/
(Assignee)

Comment 18

2 years ago
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/
(Assignee)

Comment 19

2 years ago
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/
(Assignee)

Comment 20

2 years ago
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/
(Assignee)

Comment 21

2 years ago
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/
(Assignee)

Comment 22

2 years ago
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/
(Assignee)

Comment 23

2 years ago
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/
(Assignee)

Comment 24

2 years ago
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/
(Assignee)

Comment 25

2 years ago
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/
(Assignee)

Comment 26

2 years ago
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/
(Assignee)

Comment 27

2 years ago
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/
(Assignee)

Comment 28

2 years ago
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/
(Assignee)

Comment 29

2 years ago
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/
(Assignee)

Comment 30

2 years ago
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/
(Assignee)

Comment 31

2 years ago
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/
(Assignee)

Comment 32

2 years ago
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/
(Assignee)

Comment 33

2 years ago
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/
(Assignee)

Comment 34

2 years ago
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/
(Assignee)

Comment 36

2 years ago
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/
(Assignee)

Comment 37

2 years ago
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/
(Assignee)

Comment 38

2 years ago
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/
(Assignee)

Comment 39

2 years ago
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/
(Assignee)

Comment 40

2 years ago
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/
(Assignee)

Comment 41

2 years ago
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

2 years ago
Pushed by cpearce@mozilla.com:
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
(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.
You need to log in before you can comment on or make changes to this bug.