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)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: cpearce, Assigned: cpearce)
References
(Blocks 1 open bug)
Details
Attachments
(6 files)
58 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
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.
Assignee | ||
Comment 1•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/60316/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60316/
Assignee | ||
Comment 4•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/60318/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60318/
Assignee | ||
Comment 5•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/60320/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60320/
Assignee | ||
Comment 6•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/60322/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/60322/
Assignee | ||
Comment 7•7 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•7 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/
Comment 9•7 years ago
|
||
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. ;-)
Updated•7 years ago
|
Attachment #8764406 -
Flags: review?(gsquelart) → review+
Comment 10•7 years ago
|
||
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•7 years ago
|
||
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 12•7 years ago
|
||
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 13•7 years ago
|
||
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 14•7 years ago
|
||
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 15•7 years ago
|
||
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+
Updated•7 years ago
|
Priority: -- → P2
Assignee | ||
Comment 16•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c78613e50506
Assignee | ||
Comment 17•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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•7 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 35•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=577eeeeb0698
Assignee | ||
Comment 36•7 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•7 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•7 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•7 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•7 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•7 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•7 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
Comment 43•7 years ago
|
||
(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•7 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•