Closed
Bug 1340942
Opened 7 years ago
Closed 7 years ago
Pass Data& internally inside DecoderFactory
Categories
(Core :: Audio/Video: Playback, defect, P3)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: jwwang, Assigned: jwwang)
References
Details
Attachments
(4 files)
We can pass Data& instead of TrackType to save some |aTrack == TrackInfo::kAudioTrack ? mAudio : mVideo| checks. This also makes bug 1340940 easier to do.
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8839018 -
Flags: review?(gsquelart)
Attachment #8839019 -
Flags: review?(gsquelart)
Attachment #8839020 -
Flags: review?(gsquelart)
Attachment #8839021 -
Flags: review?(gsquelart)
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8839018 [details] Bug 1340942. Part 1 - add some members so we don't have to pass the track type around. https://reviewboard.mozilla.org/r/113774/#review115324 r+ with an optional fly-by suggestion: ::: dom/media/MediaFormatReader.cpp:258 (Diff revision 1) > > void RunStage(TrackType aTrack); > MediaResult DoCreateDecoder(TrackType aTrack); > void DoInitDecoder(TrackType aTrack); > > MediaFormatReader* const mOwner; // guaranteed to be valid by the owner. Since this mOwner pointer cannot be null, it could be a reference instead. If you agree with the idea in principle: I realize it may be too much of a change here, so it could be done in a separate (lower-priority) bug. Alternatively and/or in the meantime, you could make it a NotNull<MediaFormatReader*> for a bit of extra safety.
Attachment #8839018 -
Flags: review?(gsquelart) → review+
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8839019 [details] Bug 1340942. Part 2 - pass Data& to RunStage(). https://reviewboard.mozilla.org/r/113776/#review115326 r+ with nit: ::: dom/media/MediaFormatReader.cpp:323 (Diff revision 1) > }; > > void > -MediaFormatReader::DecoderFactory::RunStage(TrackType aTrack) > +MediaFormatReader::DecoderFactory::RunStage(Data& aData) > { > - auto& data = aTrack == TrackInfo::kAudioTrack ? mAudio : mVideo; > + auto& data = aData; I don't think this reference-of-a-reference is necessary; just use 'aData' below.
Attachment #8839019 -
Flags: review?(gsquelart) → review+
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8839020 [details] Bug 1340942. Part 3 - pass Data& to DoCreateDecoder. https://reviewboard.mozilla.org/r/113778/#review115328 r+ with same nit: ::: dom/media/MediaFormatReader.cpp:382 (Diff revision 1) > > MediaResult > -MediaFormatReader::DecoderFactory::DoCreateDecoder(TrackType aTrack) > +MediaFormatReader::DecoderFactory::DoCreateDecoder(Data& aData) > { > - auto& ownerData = mOwner->GetDecoderData(aTrack); > - auto& data = aTrack == TrackInfo::kAudioTrack ? mAudio : mVideo; > + auto& ownerData = aData.mOwnerData; > + auto& data = aData; Same as previous patch, this reference is unnecessary, just use 'aData' below.
Attachment #8839020 -
Flags: review?(gsquelart) → review+
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8839021 [details] Bug 1340942. Part 4 - pass Data& to DoInitDecoder(). https://reviewboard.mozilla.org/r/113780/#review115330 r+ with same nit again: ::: dom/media/MediaFormatReader.cpp:447 (Diff revision 1) > > void > -MediaFormatReader::DecoderFactory::DoInitDecoder(TrackType aTrack) > +MediaFormatReader::DecoderFactory::DoInitDecoder(Data& aData) > { > - auto& ownerData = mOwner->GetDecoderData(aTrack); > - auto& data = aTrack == TrackInfo::kAudioTrack ? mAudio : mVideo; > + auto& ownerData = aData.mOwnerData; > + auto& data = aData; Same as previous patches, this reference is unnecessary, just use 'aData' below.
Attachment #8839021 -
Flags: review?(gsquelart) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•7 years ago
|
||
Thanks for the reviews!
Comment 14•7 years ago
|
||
Pushed by jwwang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ad17a0e96f9e Part 1 - add some members so we don't have to pass the track type around. r=gerald https://hg.mozilla.org/integration/autoland/rev/22cdad8b34c9 Part 2 - pass Data& to RunStage(). r=gerald https://hg.mozilla.org/integration/autoland/rev/7a69db46bddc Part 3 - pass Data& to DoCreateDecoder. r=gerald https://hg.mozilla.org/integration/autoland/rev/9806145e3ddd Part 4 - pass Data& to DoInitDecoder(). r=gerald
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ad17a0e96f9e https://hg.mozilla.org/mozilla-central/rev/22cdad8b34c9 https://hg.mozilla.org/mozilla-central/rev/7a69db46bddc https://hg.mozilla.org/mozilla-central/rev/9806145e3ddd
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•