If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

MediaSource.isTypeSupported("video/mp4; codecs=vp9;") returns false

RESOLVED FIXED in Firefox 54

Status

()

Core
Audio/Video: Playback
P3
normal
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

MediaSource.isTypeSupported("video/mp4; codecs=vp9;") returns false, even though we support VP9 in MSE.

HTMLMediaElement.canPlayType("video/mp4; codecs=vp9;") also returns the empty string.

Note, to complicate matters, we  only support VP9 in fragmented MP4, not in un-fragmented MP4.
Created attachment 8835264 [details]
Testcase
Comment hidden (mozreview-request)

Comment 3

7 months ago
mozreview-review
Comment on attachment 8835281 [details]
Bug 1338032 - Report VP9 in MP4 not supported in HTMLMediaElement.canPlayType, but supported in MediaSource.isTypeSupported().

https://reviewboard.mozilla.org/r/110986/#review112256

Hope you enjoyed the long MediaTypes names. :-)
One comment on this subject:

::: dom/media/mediasource/MediaSource.cpp:98
(Diff revision 1)
> +  for (const auto& codec : aContainerType.ExtendedType().Codecs().Range()) {
> +    if (IsVP9CodecString(codec)) {
> +      return true;
> +    }
> +  }

Note that this way of testing will return true if *at least one of* the requested codecs is VP9, but there could be other codecs present in the list. Is that intended?

If instead you just wanted to check for only one codec to be present in the VP9 case (as I've seen it done in other cases, e.g. mp3), you could do:
`return IsVP9CodecString(aContainerType.ExtendedType().Codecs().AsString());`
Assignee: nobody → cpearce
Comment hidden (mozreview-request)
(In reply to Gerald Squelart [:gerald] from comment #3)
> If instead you just wanted to check for only one codec to be present in the
> VP9 case (as I've seen it done in other cases, e.g. mp3), you could do:
> `return IsVP9CodecString(aContainerType.ExtendedType().Codecs().AsString());`

Thanks, that's nicer.

Comment 6

7 months ago
mozreview-review
Comment on attachment 8835281 [details]
Bug 1338032 - Report VP9 in MP4 not supported in HTMLMediaElement.canPlayType, but supported in MediaSource.isTypeSupported().

https://reviewboard.mozilla.org/r/110986/#review112630

not a fan of this approach really... but i guess for now

::: dom/media/mediasource/MediaSource.cpp:95
(Diff revision 2)
> +IsVP9InMP4(const MediaContainerType& aContainerType)
> +{
> +  const MediaContainerType mimeType(aContainerType.Type());
> +  return MP4Decoder::IsSupportedType(mimeType,
> +                                     /* DecoderDoctorDiagnostics* */ nullptr) &&
> +         IsVP9CodecString(aContainerType.ExtendedType().Codecs().AsString());

&& beginning of the line

::: dom/media/mediasource/MediaSource.cpp:112
(Diff revision 2)
>    if (!containerType) {
>      return NS_ERROR_DOM_NOT_SUPPORTED_ERR;
>    }
>  
>    if (DecoderTraits::CanHandleContainerType(*containerType, aDiagnostics)
> -      == CANPLAY_NO) {
> +      == CANPLAY_NO &&

&& at the beginning of the line
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators

::: dom/media/mediasource/MediaSource.cpp:118
(Diff revision 2)
> +      // We don't have a demuxer that can handle VP9 in non-fragmented MP4.
> +      // So special-case VP9 in MSE here, which will presumably be used
> +      // in fragmented MP4 anyway. Note we don't report in canPlayType
> +      // that VP9 in MP4 is supported, but that's OK since canPlayType is
> +      // typically used for non-fragmented anyway, and we don't support that
> +      // yet.

then canPlayType should report maybe no?

Regardless, I think it would be better to have DecoderTraits::CanHandleContainerType handle all of this.

there should be no need to modify anything in MediaSource.cpp

MediaSource.cpp handles containers, not codecs.

We worked to make it codec agnostic, and now we're adding it.
Attachment #8835281 - Flags: review?(jyavenard) → review+
(Assignee)

Comment 7

7 months ago
mozreview-review-reply
Comment on attachment 8835281 [details]
Bug 1338032 - Report VP9 in MP4 not supported in HTMLMediaElement.canPlayType, but supported in MediaSource.isTypeSupported().

https://reviewboard.mozilla.org/r/110986/#review112630

> then canPlayType should report maybe no?
> 
> Regardless, I think it would be better to have DecoderTraits::CanHandleContainerType handle all of this.
> 
> there should be no need to modify anything in MediaSource.cpp
> 
> MediaSource.cpp handles containers, not codecs.
> 
> We worked to make it codec agnostic, and now we're adding it.

As I said in the comment, canPlayType is normally used only to determine whether non-fragmented MP4 playback is supported. 

If we wanted DecoderTraits::CanHandleContainerType to be able to have this behaviour, we'd have to tell it whether we're calling it from MediaSource, which would be uglier.

We can make canPlayType report VP9 in MP4 is supported once we support VP9 in non-fragmented MP4.

Comment 8

7 months ago
mozreview-review-reply
Comment on attachment 8835281 [details]
Bug 1338032 - Report VP9 in MP4 not supported in HTMLMediaElement.canPlayType, but supported in MediaSource.isTypeSupported().

https://reviewboard.mozilla.org/r/110986/#review112630

> As I said in the comment, canPlayType is normally used only to determine whether non-fragmented MP4 playback is supported. 
> 
> If we wanted DecoderTraits::CanHandleContainerType to be able to have this behaviour, we'd have to tell it whether we're calling it from MediaSource, which would be uglier.
> 
> We can make canPlayType report VP9 in MP4 is supported once we support VP9 in non-fragmented MP4.

not really..
canPlayType is used everywhere MSE is used, this is what's used to assess what container and codec we can play.

but i see your point...
(Assignee)

Updated

7 months ago
Blocks: 1339204
Comment hidden (mozreview-request)

Comment 10

7 months ago
mozreview-review
Comment on attachment 8835281 [details]
Bug 1338032 - Report VP9 in MP4 not supported in HTMLMediaElement.canPlayType, but supported in MediaSource.isTypeSupported().

https://reviewboard.mozilla.org/r/110986/#review113942

r+ with nits:

I'm assuming you'll remove the Note in the commit description, I don't think it would make sense outside of this review.

::: dom/html/HTMLMediaElement.cpp:4521
(Diff revision 3)
>  
> +static bool
> +IsVP9InMP4(const MediaContainerType& aContainerType)
> +{
> +  const MediaContainerType mimeType(aContainerType.Type());
> +  return MP4Decoder::IsSupportedType(mimeType,

MP4Decoder only exists if MOZ_FMP4 is #defined.
Though that macro may just disappear any day now, see bug 1257145.

But just in case, I would suggest you use DecoderTraits::IsSupportedType, which always exists and does the macro check.
(If you do that, don't forget to remove the #include you added at line 99.)

And when bug 1257145 gets looked at, we'll probably remove this DecoderTraits clutch and reinstate MP4Decoder direct calls where needed.

::: dom/html/HTMLMediaElement.cpp:4521
(Diff revision 3)
> +  return MP4Decoder::IsSupportedType(mimeType,
> +    /* DecoderDoctorDiagnostics* */ nullptr)
> +    && IsVP9CodecString(aContainerType.ExtendedType().Codecs().AsString());

Style: I would suggest aligning the 2nd line '/*...' with 'mimeType', and the 3rd line '&&...' with 'MP4Decoder...', it would feel more logical to me, and should still fit in 80 columns.
Attachment #8835281 - Flags: review?(gsquelart) → review+
(Assignee)

Comment 11

7 months ago
mozreview-review-reply
Comment on attachment 8835281 [details]
Bug 1338032 - Report VP9 in MP4 not supported in HTMLMediaElement.canPlayType, but supported in MediaSource.isTypeSupported().

https://reviewboard.mozilla.org/r/110986/#review113942

> MP4Decoder only exists if MOZ_FMP4 is #defined.
> Though that macro may just disappear any day now, see bug 1257145.
> 
> But just in case, I would suggest you use DecoderTraits::IsSupportedType, which always exists and does the macro check.
> (If you do that, don't forget to remove the #include you added at line 99.)
> 
> And when bug 1257145 gets looked at, we'll probably remove this DecoderTraits clutch and reinstate MP4Decoder direct calls where needed.

How about DecoderTraits::IsMP4SupportedType()?
Comment hidden (mozreview-request)
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd4348e41d7b23d7062000dcaebff388d7608ccd
Bug 1338032 - Report VP9 in MP4 not supported in HTMLMediaElement.canPlayType, but supported in MediaSource.isTypeSupported(). r=gerald

Comment 14

7 months ago
mozreview-review-reply
Comment on attachment 8835281 [details]
Bug 1338032 - Report VP9 in MP4 not supported in HTMLMediaElement.canPlayType, but supported in MediaSource.isTypeSupported().

https://reviewboard.mozilla.org/r/110986/#review113942

> How about DecoderTraits::IsMP4SupportedType()?

Oh, that's what I meant, of course. ;-)
I see you've used it, great, thank you.

Comment 15

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bd4348e41d7b
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Jay pointed out that the coding standard says:

"Break long conditions after && and || logical connectives. See below for the rule for other operators."

[...]

"Overlong expressions not joined by && and || should break so the operator starts on the second line and starts in the same column as the start of the expression in the first line."

Emphasis on the "not joined by && and ||".

So I think we're interpreting the coding standard wrong, and && and || operators should go on the end of the line.
You need to log in before you can comment on or make changes to this bug.