Closed Bug 1330284 Opened 3 years ago Closed 3 years ago

Start using MediaContentType in DecoderTraits and immediate dependencies

Categories

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

49 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: gerald, Assigned: gerald)

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 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 on attachment 8825746 [details]
Bug 1330284 - Use MediaContentType in MediaKeySystemAccess -

https://reviewboard.mozilla.org/r/103824/#review104684
Attachment #8825746 - Flags: review?(jyavenard) → 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 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 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 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 on attachment 8825752 [details]
Bug 1330284 - Use MediaContentType in ADTSDecoder -

https://reviewboard.mozilla.org/r/103836/#review104708
Attachment #8825752 - Flags: review?(jyavenard) → 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 on attachment 8825754 [details]
Bug 1330284 - Use MediaContentType in FlacDecoder -

https://reviewboard.mozilla.org/r/103840/#review104712
Attachment #8825754 - Flags: review?(jyavenard) → 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 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 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 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 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 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 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 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 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.
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
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.
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.
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.
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
Blocks: 1331289
Blocks: 1331770
You need to log in before you can comment on or make changes to this bug.