814284 - Refactor the code necessary for detection of codec support out of nsHTMLMediaElement

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[(no longer active)]

I need this code in the decodeAudioData implementation, so let's put it in a shared class called DecoderTraits or something.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 684279 [details] [diff] [review]
Patch (v1)

Review of attachment 684279 [details] [diff] [review]:
-----------------------------------------------------------------

You sir, have a knack for writing patches that I'm half way through writing myself!

r=cpearce with the following changes.

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ -2375,1 @@
>    if (status == CANPLAY_NO)

It would be good if you could push the logic that uses supportedCodecs down into DecoderTraits::CanHandleMediaType() as well. Then you don't need to return the codecs list either.

::: content/media/DecoderTraits.cpp
@@ +172,5 @@
> +
> +bool
> +DecoderTraits::IsH264Type(const nsACString& aType)
> +{
> +  for (uint32_t i = 0; i < ArrayLength(gH264Types); ++i) {

All these loops over these arrays can be refactored into a single function.

Basically CodecListContains from nsHTMLMediaElement.cpp.

@@ +266,5 @@
> +}
> +#endif
> +
> +/* static */
> +bool DecoderTraits::ShouldHandleMediaType(const char* aMIMEType)

This can simply be:

#ifdef MOZ_WAVE
  if (IsWaveType(aMIMEType)) {
    // $Explanation_comment...
    return false;
  }
#endif
return CanHandleMediaStatus(...) != CANPLAY_NO.

::: content/media/DecoderTraits.h
@@ +34,5 @@
> +  // false here even if CanHandleMediaType would return true.
> +  static bool ShouldHandleMediaType(const char* aMIMEType);
> +
> +#ifdef MOZ_RAW
> +  static const char gRawTypes[2][16];

None of the g*Types and g*Codecs need to be exposed public; they can be declared static inside DecoderTraits.cpp.

Comment 3

[(no longer active)]

Comment on attachment 684279 [details] [diff] [review]
Patch (v1)

Review of attachment 684279 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ -2375,1 @@
>    if (status == CANPLAY_NO)

Hmm, this turned out to be a much more complicated patch than I expected, and I haven't managed to get it right.  I'll submit that as a separate patch.

::: content/media/DecoderTraits.cpp
@@ +172,5 @@
> +
> +bool
> +DecoderTraits::IsH264Type(const nsACString& aType)
> +{
> +  for (uint32_t i = 0; i < ArrayLength(gH264Types); ++i) {

This depends on the previous part.

@@ +266,5 @@
> +}
> +#endif
> +
> +/* static */
> +bool DecoderTraits::ShouldHandleMediaType(const char* aMIMEType)

Hmm, I don't follow how these are equivalent?

::: content/media/DecoderTraits.h
@@ +34,5 @@
> +  // false here even if CanHandleMediaType would return true.
> +  static bool ShouldHandleMediaType(const char* aMIMEType);
> +
> +#ifdef MOZ_RAW
> +  static const char gRawTypes[2][16];

Right, I'll do that.

Comment 4

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=fc3e4810f150

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 684279 [details] [diff] [review]
Patch (v1)

Review of attachment 684279 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/DecoderTraits.cpp
@@ +266,5 @@
> +}
> +#endif
> +
> +/* static */
> +bool DecoderTraits::ShouldHandleMediaType(const char* aMIMEType)

These are equivalent because CanHandleMediaType() returns != CANPLAY_NO when one of the Is*Type() functions return true.

Whereas ShouldHandleMediaType() (as you've written it) returns true when one of the Is*Type() functions return true. Except for Wave.

Does that make sense?

Comment 6

[(no longer active)]

(In reply to Ehsan Akhgari [:ehsan] from comment #3)
> Comment on attachment 684279 [details] [diff] [review]
> Patch (v1)
> 
> Review of attachment 684279 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/html/content/src/nsHTMLMediaElement.cpp
> @@ -2375,1 @@
> >    if (status == CANPLAY_NO)
> 
> Hmm, this turned out to be a much more complicated patch than I expected,
> and I haven't managed to get it right.  I'll submit that as a separate patch.

Actually, this didn't turn out to be very difficult!

Comment 7

[(no longer active)]

(In reply to Chris Pearce (:cpearce) from comment #5)
> Comment on attachment 684279 [details] [diff] [review]
> Patch (v1)
> 
> Review of attachment 684279 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/DecoderTraits.cpp
> @@ +266,5 @@
> > +}
> > +#endif
> > +
> > +/* static */
> > +bool DecoderTraits::ShouldHandleMediaType(const char* aMIMEType)
> 
> These are equivalent because CanHandleMediaType() returns != CANPLAY_NO when
> one of the Is*Type() functions return true.
> 
> Whereas ShouldHandleMediaType() (as you've written it) returns true when one
> of the Is*Type() functions return true. Except for Wave.
> 
> Does that make sense?

Yes.  Will do.

Comment 8

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/abfcf6bd97a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/2020b35ea3b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/27e1c4fba7e6
https://hg.mozilla.org/integration/mozilla-inbound/rev/37626d0f43fc

(I addressed each major review comment in its own patch in order to keep the patches simple.)

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/abfcf6bd97a4
https://hg.mozilla.org/mozilla-central/rev/2020b35ea3b4
https://hg.mozilla.org/mozilla-central/rev/27e1c4fba7e6
https://hg.mozilla.org/mozilla-central/rev/37626d0f43fc