Closed Bug 1281632 Opened 7 years ago Closed 7 years ago

Refactor create MediaDataDecoder arguments into CreateDecoderParams struct

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

Bug 1281632 - P1: Extract creation parameters and pass via struct.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1281632 - P2: Reduce GMPAudioDecoder/GMPVideoDecoder ctor count.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1281632 - P3: Move EMEAudioDecoder/EMEVideoDecoder ctors.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1281632 - P4: Add VideoInfo ctor that takes nsIntSize.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1281632 - P5: Extract parameters to GMPVideoDecoder into struct.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1281632 - P6: Extract parameters to GMPAudioDecoder into struct.
58 bytes, text/x-review-board-request
mozbugz
: review+
Details 
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.
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)
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 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 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+
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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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/
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.