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

Status: RESOLVED FIXED

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

Description

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

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.

Comment 1

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

Attachment: Testcase

Comment 2

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

Attachment: Bug 1338032 - Report VP9 in MP4 not supported in HTMLMediaElement.canPlayType, but supported in MediaSource.isTypeSupported().

We don't have an MP4 demuxer that can handle VP9 in non-fragmented MP4.  So
have only MediaSource.isTypeSupported() report that it can play VP9 in MP4, and
don't report that we can play VP9 in MP4 in HTMLMediaElement.canPlayType().

The upcoming Rust demuxer will be able to support VP9 in MP4, so once we've
enabled that, we can confidently report in canPlayType that we support VP9 in
MP4.

Review commit: https://reviewboard.mozilla.org/r/110986/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110986/

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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());`

Comment 4

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

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110986/diff/1-2/

Comment 5

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

(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

[Jean-Yves Avenard [:jya]]

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.

Comment 7

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

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

[Jean-Yves Avenard [:jya]]

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...

Comment 9

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

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110986/diff/2-3/

Comment 10

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 11

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

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 12

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

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110986/diff/3-4/

Comment 13

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

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

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bd4348e41d7b

Comment 16

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

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.