Closed
Bug 1378304
Opened 7 years ago
Closed 7 years ago
Remove AbstractMediaDecoder::GetResource()
Categories
(Core :: Audio/Video: Playback, enhancement, P3)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: jwwang, Assigned: jwwang)
References
Details
Attachments
(4 files)
A step toward bug 1378295.
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 #8883781 -
Flags: review?(jyavenard)
Attachment #8883782 -
Flags: review?(jyavenard)
Attachment #8883783 -
Flags: review?(jyavenard)
Attachment #8883784 -
Flags: review?(jyavenard)
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8883781 [details] Bug 1378304. P1 - pass a MediaResource* to DecoderTraits::CreateReader() to reduce the use of AbstractMediaDecoder::GetResource(). https://reviewboard.mozilla.org/r/154726/#review159792
Attachment #8883781 -
Flags: review?(jyavenard) → review+
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8883782 [details] Bug 1378304. P2 - replace GetResource() with mResource in MediaDecoder sub-classes. https://reviewboard.mozilla.org/r/154728/#review159796
Attachment #8883782 -
Flags: review?(jyavenard) → review+
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8883783 [details] Bug 1378304. P3 - store MediaResource in MediaDecoderReader and replace the use of GetResource() with mResource. https://reviewboard.mozilla.org/r/154730/#review159798 ::: dom/media/MediaDecoderReader.cpp:76 (Diff revision 1) > + MediaResource* aResource) > : mAudioCompactor(mAudioQueue) > , mDecoder(aDecoder) > - , mTaskQueue(new TaskQueue( > + , mTaskQueue(new TaskQueue(GetMediaThreadPool(MediaThreadType::PLAYBACK), > - GetMediaThreadPool(MediaThreadType::PLAYBACK), > - "MediaDecoderReader::mTaskQueue", > + "MediaDecoderReader::mTaskQueue", all those formatting changes are out of scope when all youre doing is adding a new member. also, please use the formatting generated by clang-format ::: dom/media/android/AndroidMediaReader.cpp:32 (Diff revision 1) > > -AndroidMediaReader::AndroidMediaReader(AbstractMediaDecoder *aDecoder, > - const MediaContainerType& aContainerType) : > - MediaDecoderReader(aDecoder), > - mType(aContainerType), > - mPlugin(nullptr), > +AndroidMediaReader::AndroidMediaReader(AbstractMediaDecoder* aDecoder, > + const MediaContainerType& aContainerType, > + MediaResource* aResource) > + : MediaDecoderReader(aDecoder, aResource) > + , mType(aContainerType) out of scope.
Attachment #8883783 -
Flags: review?(jyavenard) → review+
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8883783 [details] Bug 1378304. P3 - store MediaResource in MediaDecoderReader and replace the use of GetResource() with mResource. https://reviewboard.mozilla.org/r/154730/#review159798 > all those formatting changes are out of scope when all youre doing is adding a new member. > > also, please use the formatting generated by clang-format Yes, the format change is already done by |mach clang-format|. |mach clang-format| only works for the current change. How can I move the format change to another patch?
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8883784 [details] Bug 1378304. P4 - remove AbstractMediaDecoder::GetResource(). https://reviewboard.mozilla.org/r/154732/#review159800 you mentioned in your bug description that it would help using MediaDecoderReader with webrtc, what are you plans there? I don't see how.
Attachment #8883784 -
Flags: review?(jyavenard) → review+
Assignee | ||
Comment 10•7 years ago
|
||
It would be easier to reuse MediaDecoderReader without supplying a AbstractMediaDecoder. BufferDecoder is an example whose implementation is mostly empty. It exists only for we can pass something to the constructor of MediaDecoderReader.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•7 years ago
|
||
Thanks for the review!
Comment 14•7 years ago
|
||
Pushed by jwwang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/66cfc213ea2c P1 - pass a MediaResource* to DecoderTraits::CreateReader() to reduce the use of AbstractMediaDecoder::GetResource(). r=jya https://hg.mozilla.org/integration/autoland/rev/b290ce30c00d P2 - replace GetResource() with mResource in MediaDecoder sub-classes. r=jya https://hg.mozilla.org/integration/autoland/rev/b76b993079f0 P3 - store MediaResource in MediaDecoderReader and replace the use of GetResource() with mResource. r=jya https://hg.mozilla.org/integration/autoland/rev/c20113d6cac6 P4 - remove AbstractMediaDecoder::GetResource(). r=jya
Comment 15•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/66cfc213ea2c https://hg.mozilla.org/mozilla-central/rev/b290ce30c00d https://hg.mozilla.org/mozilla-central/rev/b76b993079f0 https://hg.mozilla.org/mozilla-central/rev/c20113d6cac6
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•