Closed
Bug 1330284
Opened 7 years ago
Closed 7 years ago
Start using MediaContentType in DecoderTraits and immediate dependencies
Categories
(Core :: Audio/Video: Playback, defect)
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: mozbugz, Assigned: mozbugz)
References
(Blocks 1 open bug)
Details
Attachments
(17 files)
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jya
:
review+
|
Details |
DecoderTraits is a giant hub of activities where MIME types are passed around as strings. This bug will start using MediaContentType, MediaMIMEType, etc. there, and in the decoder APIs called from there.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8825750 [details] Bug 1330284 - Use MediaContentType in MP4Decoder - https://reviewboard.mozilla.org/r/103832/#review104678 ::: dom/media/DecoderTraits.cpp:271 (Diff revision 1) > return CANPLAY_MAYBE; > } > if (IsWaveSupportedType(mimeType.Type().AsString())) { > return CANPLAY_MAYBE; > } > - if (DecoderTraits::IsMP4TypeAndEnabled(mimeType.Type().AsString(), aDiagnostics)) { > +#ifdef MOZ_FMP4 Why not call the DecoderTraits::IsMP4Supported that already is conditional on MoZ_FMP4 Seems more elegsnt to me and easier to maintain as only place would need to be changed.
Attachment #8825750 -
Flags: review?(jyavenard) → review+
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8825746 [details] Bug 1330284 - Use MediaContentType in MediaKeySystemAccess - https://reviewboard.mozilla.org/r/103824/#review104684
Attachment #8825746 -
Flags: review?(jyavenard) → review+
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8825747 [details] Bug 1330284 - Use MediaContentType in DecoderTraits/InstantiateDecoder - https://reviewboard.mozilla.org/r/103826/#review104688
Attachment #8825747 -
Flags: review?(jyavenard) → review+
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8825748 [details] Bug 1330284 - Use MediaContentType in OggDecoder - https://reviewboard.mozilla.org/r/103828/#review104694 ::: dom/media/DecoderTraits.cpp (Diff revision 1) > } > return false; > } > > static bool > -IsOggSupportedType(const nsACString& aType, This makes it a tad inconsistent with the rest.. all tests are wrapped in a convenience method (which makes sense as its often conditional on a define) but no longer
Attachment #8825748 -
Flags: review?(jyavenard) → review+
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8825749 [details] Bug 1330284 - Use MediaContentType in WebMDecoder - https://reviewboard.mozilla.org/r/103830/#review104700 ::: dom/media/webm/WebMDecoder.h:14 (Diff revision 1) > #include "MediaDecoder.h" > #include "MediaFormatReader.h" > > namespace mozilla { > > +class MediaContentType; I had mentioned that earlier, but personally, i dont like forward declaration of a type used by a public method... the use of that method should only require to include the header containing that method definition.
Attachment #8825749 -
Flags: review?(jyavenard) → review+
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8825751 [details] Bug 1330284 - Use MediaContentType in MP3Decoder - https://reviewboard.mozilla.org/r/103834/#review104704 Same deal with forward declaration
Attachment #8825751 -
Flags: review?(jyavenard) → review+
Comment 24•7 years ago
|
||
mozreview-review |
Comment on attachment 8825752 [details] Bug 1330284 - Use MediaContentType in ADTSDecoder - https://reviewboard.mozilla.org/r/103836/#review104708
Attachment #8825752 -
Flags: review?(jyavenard) → review+
Comment 25•7 years ago
|
||
mozreview-review |
Comment on attachment 8825753 [details] Bug 1330284 - Use MediaContentType in WaveDecoder - https://reviewboard.mozilla.org/r/103838/#review104710
Attachment #8825753 -
Flags: review?(jyavenard) → review+
Comment 26•7 years ago
|
||
mozreview-review |
Comment on attachment 8825754 [details] Bug 1330284 - Use MediaContentType in FlacDecoder - https://reviewboard.mozilla.org/r/103840/#review104712
Attachment #8825754 -
Flags: review?(jyavenard) → review+
Comment 27•7 years ago
|
||
mozreview-review |
Comment on attachment 8825755 [details] Bug 1330284 - Use MediaContentType in DirectShowDecoder - https://reviewboard.mozilla.org/r/103842/#review104714
Attachment #8825755 -
Flags: review?(jyavenard) → review+
Comment 28•7 years ago
|
||
mozreview-review |
Comment on attachment 8825756 [details] Bug 1330284 - Use MediaContentType in AndroidMediaPluginHost::FindDecoder - https://reviewboard.mozilla.org/r/103844/#review104718 ::: dom/media/android/AndroidMediaPluginHost.cpp:230 (Diff revision 1) > AndroidMediaPluginHost::~AndroidMediaPluginHost() { > mResourceServer->Stop(); > MOZ_COUNT_DTOR(AndroidMediaPluginHost); > } > > -bool AndroidMediaPluginHost::FindDecoder(const nsACString& aMimeType, const char* const** aCodecs) > +bool AndroidMediaPluginHost::FindDecoder(const MediaContentType& aMimeType, const char* const** aCodecs) cool make tjis style compliant while at it :)
Attachment #8825756 -
Flags: review?(jyavenard) → review+
Comment 29•7 years ago
|
||
mozreview-review |
Comment on attachment 8825757 [details] Bug 1330284 - Use MediaContentType in AndroidMediaPluginHost::CreateDecoder, AndroidMediaDecoder, AndroidMediaReader - https://reviewboard.mozilla.org/r/103846/#review104720
Attachment #8825757 -
Flags: review?(jyavenard) → review+
Comment 30•7 years ago
|
||
mozreview-review |
Comment on attachment 8825758 [details] Bug 1330284 - Use MediaContentType in DecoderTraits:IsAndroidMediaType() - https://reviewboard.mozilla.org/r/103848/#review104722
Attachment #8825758 -
Flags: review?(jyavenard) → review+
Comment 31•7 years ago
|
||
mozreview-review |
Comment on attachment 8825759 [details] Bug 1330284 - Use MediaCodecs::Range() in DecoderTraits:CanHandleCodecsType() - https://reviewboard.mozilla.org/r/103850/#review104724
Attachment #8825759 -
Flags: review?(jyavenard) → review+
Comment 32•7 years ago
|
||
mozreview-review |
Comment on attachment 8825760 [details] Bug 1330284 - Use MediaContentType in CreateReader and MediaBufferDecoder - https://reviewboard.mozilla.org/r/103852/#review104726
Attachment #8825760 -
Flags: review?(jyavenard) → review+
Comment 33•7 years ago
|
||
mozreview-review |
Comment on attachment 8825761 [details] Bug 1330284 - Use MediaCodecs in DirectShowDecoder and AndroidMediaPluginHost - https://reviewboard.mozilla.org/r/103854/#review104728
Attachment #8825761 -
Flags: review?(jyavenard) → review+
Comment 34•7 years ago
|
||
mozreview-review |
Comment on attachment 8825762 [details] Bug 1330284 - Use MediaContentType in DecoderTraits:IsHttpLiveStreamingType - https://reviewboard.mozilla.org/r/103856/#review104730
Attachment #8825762 -
Flags: review?(jyavenard) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 52•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8825748 [details] Bug 1330284 - Use MediaContentType in OggDecoder - https://reviewboard.mozilla.org/r/103828/#review104694 > This makes it a tad inconsistent with the rest.. all tests are wrapped in a convenience method (which makes sense as its often conditional on a define) > but no longer I'm going to do this for all of them, except for a few that can't be relocated, or that really need a macro check right here.
Assignee | ||
Comment 53•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8825749 [details] Bug 1330284 - Use MediaContentType in WebMDecoder - https://reviewboard.mozilla.org/r/103830/#review104700 > I had mentioned that earlier, but personally, i dont like forward declaration of a type used by a public method... > > the use of that method should only require to include the header containing that method definition. I'm just following the C++ practices: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices "Forward-declare classes in your header files instead of including them whenever possible." I asked on mozilla.dev.platform, and people confirmed they really want forward declarations instead of includes where possible. There's even a special bug to reduce "include hell" :-) https://bugzilla.mozilla.org/show_bug.cgi?id=includehell
Assignee | ||
Comment 54•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8825750 [details] Bug 1330284 - Use MediaContentType in MP4Decoder - https://reviewboard.mozilla.org/r/103832/#review104678 > Why not call the DecoderTraits::IsMP4Supported that already is conditional on MoZ_FMP4 > > Seems more elegsnt to me and easier to maintain as only place would need to be changed. The other places already had #ifdef MOZ_FMP4, so I thought one more would be fine and consistent. As discussed elsewhere, we may in fact want to get rid of MOZ_FMP4 completely, as the container can now/later carry patent-free codecs. There is already a bug to remove --disable-fmp4 (which controls the MOZ_FMP4 macro) : bug 1257145 comment 2.
Assignee | ||
Comment 55•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8825756 [details] Bug 1330284 - Use MediaContentType in AndroidMediaPluginHost::FindDecoder - https://reviewboard.mozilla.org/r/103844/#review104718 > cool make tjis style compliant while at it :) Are you talking about the 80-column limit? This file is full of long lines, so I went with the trend! But yeah I can easily cut this one, and in the next commit as well.
Assignee | ||
Comment 56•7 years ago
|
||
I had forgotten to publish my review responses, sorry. I've already requested autoland, so if you see anything that's still a concern, I'll open a follow-up bug.
Comment 57•7 years ago
|
||
Pushed by gsquelart@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/426109521b9a Use MediaContentType in MediaKeySystemAccess - r=jya https://hg.mozilla.org/integration/autoland/rev/93c969920377 Use MediaContentType in DecoderTraits/InstantiateDecoder - r=jya https://hg.mozilla.org/integration/autoland/rev/45673c5c9e20 Use MediaContentType in OggDecoder - r=jya https://hg.mozilla.org/integration/autoland/rev/571b1d64748c Use MediaContentType in WebMDecoder - r=jya https://hg.mozilla.org/integration/autoland/rev/f65d9b2df6ee Use MediaContentType in MP4Decoder - r=jya https://hg.mozilla.org/integration/autoland/rev/e0dd6cd20db1 Use MediaContentType in MP3Decoder - r=jya https://hg.mozilla.org/integration/autoland/rev/7350253f2484 Use MediaContentType in ADTSDecoder - r=jya https://hg.mozilla.org/integration/autoland/rev/6437fa28aae1 Use MediaContentType in WaveDecoder - r=jya https://hg.mozilla.org/integration/autoland/rev/a8b342ba64a3 Use MediaContentType in FlacDecoder - r=jya https://hg.mozilla.org/integration/autoland/rev/540717f0dc54 Use MediaContentType in DirectShowDecoder - r=jya https://hg.mozilla.org/integration/autoland/rev/71dea91ccdda Use MediaContentType in AndroidMediaPluginHost::FindDecoder - r=jya https://hg.mozilla.org/integration/autoland/rev/3271745425d7 Use MediaContentType in AndroidMediaPluginHost::CreateDecoder, AndroidMediaDecoder, AndroidMediaReader - r=jya https://hg.mozilla.org/integration/autoland/rev/7456fe797771 Use MediaContentType in DecoderTraits:IsAndroidMediaType() - r=jya https://hg.mozilla.org/integration/autoland/rev/e0c2f4a53ca1 Use MediaCodecs::Range() in DecoderTraits:CanHandleCodecsType() - r=jya https://hg.mozilla.org/integration/autoland/rev/f58848e55b8c Use MediaContentType in CreateReader and MediaBufferDecoder - r=jya https://hg.mozilla.org/integration/autoland/rev/cab900d5963e Use MediaCodecs in DirectShowDecoder and AndroidMediaPluginHost - r=jya https://hg.mozilla.org/integration/autoland/rev/6f91f2f99c3c Use MediaContentType in DecoderTraits:IsHttpLiveStreamingType - r=jya
Comment 58•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/426109521b9a https://hg.mozilla.org/mozilla-central/rev/93c969920377 https://hg.mozilla.org/mozilla-central/rev/45673c5c9e20 https://hg.mozilla.org/mozilla-central/rev/571b1d64748c https://hg.mozilla.org/mozilla-central/rev/f65d9b2df6ee https://hg.mozilla.org/mozilla-central/rev/e0dd6cd20db1 https://hg.mozilla.org/mozilla-central/rev/7350253f2484 https://hg.mozilla.org/mozilla-central/rev/6437fa28aae1 https://hg.mozilla.org/mozilla-central/rev/a8b342ba64a3 https://hg.mozilla.org/mozilla-central/rev/540717f0dc54 https://hg.mozilla.org/mozilla-central/rev/71dea91ccdda https://hg.mozilla.org/mozilla-central/rev/3271745425d7 https://hg.mozilla.org/mozilla-central/rev/7456fe797771 https://hg.mozilla.org/mozilla-central/rev/e0c2f4a53ca1 https://hg.mozilla.org/mozilla-central/rev/f58848e55b8c https://hg.mozilla.org/mozilla-central/rev/cab900d5963e https://hg.mozilla.org/mozilla-central/rev/6f91f2f99c3c
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•