Closed Bug 1176218 Opened 9 years ago Closed 8 years ago

[Youtube][HTML5] 8K videos do not work at all, or Audio only

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla52
Tracking Status
platform-rel --- ?
firefox39 --- affected
firefox40 --- affected
firefox41 --- affected
firefox42 --- affected
firefox52 --- verified

People

(Reporter: FlorinMezei, Assigned: mozbugz, Mentored)

References

(Blocks 2 open bugs, )

Details

(Whiteboard: [webvr] [platform-rel-Youtube])

Attachments

(13 files, 1 obsolete file)

58 bytes, text/x-review-board-request
jya
: review+
Details
58 bytes, text/x-review-board-request
jya
: review+
Details
58 bytes, text/x-review-board-request
jya
: review+
Details
58 bytes, text/x-review-board-request
jya
: review+
Details
58 bytes, text/x-review-board-request
jya
: review+
Details
58 bytes, text/x-review-board-request
jya
: review+
Details
58 bytes, text/x-review-board-request
jya
: review+
Details
58 bytes, text/x-review-board-request
jya
: review+
Details
58 bytes, text/x-review-board-request
froydnj
: review+
Details
58 bytes, text/x-review-board-request
jya
: review+
Details
58 bytes, text/x-review-board-request
jya
: review+
Details
58 bytes, text/x-review-board-request
jya
: review+
Details
58 bytes, text/x-review-board-request
jya
: review+
Details
Reproducible with: 
- Firefox 39 Beta 7 - BuildID:  20150618135210
- Firefox 40 Aurora latest - BuildID: 20150619004003
- Firefox 41 Nightly latest - BuildID: 20150618030206

Reproducible on: Windows 7 x64, Win 8.1 x64, Win 10 x86, Win 10 x64, Mac OS X 10.9.5 (Ubuntu doesn't support such high resolution videos)

Steps to reproduce:
1. Open Firefox with a fresh profile and go to https://www.youtube.com/.
2. Play a video at 4320p 8K (e.g. https://www.youtube.com/watch?v=sLprVF6d7Ug).

Expected results:
Video and Audio plays (even if at reduced frame rate).

Actual results:
- Audio plays, but Video is black - Windows 7 x64, Win 8.1 x64, Win 10 x86 - Chrome delivers both Video and Audio
OR
- Video and Audio do not play at all, as a spinner displays forever - Win 10 x64, Mac OS X 10.9.5 - Chrome works on Win 10, but also does NOT work on Mac OS X 10.9.5

8K is fairly new to Youtube, and not really expected to work well on most machines, but this is yet another new thing that Chrome supports while we don't (bug 1143988 - 360 degrees videos - is another example).
I also get stuck on the spinner when trying to watch the 8K video in Nightly on OS X. I see the following error message in the console:

Media resource mediasource:https://www.youtube.com/ee83e74d-4adb-8245-a816-b2e1ff6d20eb could not be decoded.

Curiously, Chrome 43 on OS X also gets stuck on the spinner when trying to play this 8K video. Safari tries to play the 8K video, flashes an error message, falls back to 480p, and removes the 8K choice from the settings menu.

@Anthony, is this a limitation of the HW decoders? Can we fall back to software?
Blocks: youtube-mse
Flags: needinfo?(ajones)
Jean-Yves - is this a simple fix?
Flags: needinfo?(ajones) → needinfo?(jyavenard)
The limitation is likely a hardware one.

Win7 supports a maximum of 1080p in hardware.
No GPU that I know will decode 8K. The latest Intel broadwell is limited to 4K.

Chrome will do it on windows because they use software decoding and VP8/VP9 by default.
Flags: needinfo?(jyavenard)
(In reply to Jean-Yves Avenard [:jya] from comment #3)
> The limitation is likely a hardware one.

This just means we need to fall back to software for 8k, right?
Anthony offered to debug this to see whether Firefox or the YouTube player is responsible for not falling back from HW to SW correctly.

On my MacBook Pro, neither Nightly 42 nor Chrome 43 are able to play the following video in 8K. The progress spinner just spins forever. The video *is* able to play (jankily) in Chrome Beta 44 and Canary 46. YouTube devs said some video decoder improvements landed in Chrome 44.

https://www.youtube.com/watch?v=sLprVF6d7Ug
Flags: needinfo?(ajones)
Summary: [Youtube][HTML5] 8k videos do not work at all, or Audio only → [Youtube][HTML5] 8K videos do not work at all, or Audio only
(In reply to Chris Peterson [:cpeterson] from comment #5)
> On my MacBook Pro, neither Nightly 42 nor Chrome 43 are able to play the
> following video in 8K. The progress spinner just spins forever. The video
> *is* able to play (jankily) in Chrome Beta 44 and Canary 46. YouTube devs
> said some video decoder improvements landed in Chrome 44.

Chrome will probably be using VP9 which is a software path.
Flags: needinfo?(ajones)
Depends on: 1191525
Component: Audio/Video → Audio/Video: Playback
Whiteboard: [webvr]
Depends on: 1222145
platform-rel: --- → ?
Whiteboard: [webvr] → [webvr] [platform-rel-Youtube]
Works for me in both H.264 and VP9.

Florin - are you happy to close this now?
Flags: needinfo?(florin.mezei)
Hi,
I have tested the Youtube 8K videos using Nightly 50.0a1 (2016-07-10) on the following OS's:

Windows 10 x86 & x64
Windows 8.1 x86 & x64
Windows 7 x64
Ubuntu 14.04 x64
Ubuntu 16.04 x64
OSx 10.9.5
OSx 10.10.1
OSx 10.11.5
OSx 10.12 Preview.

8K videos  - DO NOT WORK on any Apple OSx build (an error message is displayed) and Windows 7 x64,
           - Work on Ubuntu and Windows 10 x64,
	   - Have infinite loading on both Windows 8.1 builds and on Windows 10 x86.

Considering all the above I consider that this issue SHOULD NOT be closed yet.
Flags: needinfo?(florin.mezei)
I tried this as well on Windows 7 x64 using the latest Firefox 50 Nightly and Firefox 48 Beta 9.
- Dirty Profile - OK (both video and sound)
   Mime Type: video/webm; codecs="vp9"
   DASH: yes (313/251)

- Clean Profile - Does NOT work (infinite spinner)
   Mime Type: video/mp4; codecs="avc1.640033"
   DASH: yes (138/251)

So it seems to me that this is a matter of getting the right codecs. Any thoughts Anthony?
Flags: needinfo?(ajones)
I'm going to mark this as P1 because it looks straightforward and could break 360 degree video. Maybe we can fall back to software for 8k or just tell YouTube that we can't support 4k scanlines. The codec type is wrong for 8K if it thinks it is avc1.640033 because that is level 5.1 which is 4k 30fps.
Flags: needinfo?(ajones)
Priority: -- → P1
Assignee: nobody → jyavenard
Assignee: jyavenard → gsquelart
Mentor: jyavenard
As a quick experiment, if we reply false (can't play) when Youtube asks isTypeSupported for a width or height of 99999, it then requests the following video/mp4 codecs&sizes in this order:
> codecs="avc1.4d4015"; width=426; height=240
> codecs="avc1.4d401e"; width=640; height=360
> codecs="avc1.4d401e"; width=854; height=480
> codecs="avc1.4d401f"; width=1280; height=720
> codecs="avc1.640028"; width=1920; height=1080
> codecs="avc1.640033"; width=7680; height=4320
So it should be possible to decline any of these that we can't or don't want to play, based on codecs and/or sizes.
We have a bug to support the width/height parameters provided by YouTube. I've put a patch testing those too IIRC.

Note that those parameters are not standard. YouTube stated that they use those for some TV clients.
(In reply to Jean-Yves Avenard [:jya] from comment #12)
> We have a bug to support the width/height parameters provided by YouTube.
> Note that those parameters are not standard. YouTube stated that they use those for some TV clients.
Thank you. Found it: bug 1227017.
Matt - there was a bug relating to querying the video driver for its maximum supported parameters. Where did that get to?
Flags: needinfo?(matt.woodrow)
Not as straightforward as initially thought, but also not that critical at the moment -> P3.
Priority: P1 → P3
I assume you mean bug 1239098?

We already have code to query DXVA for it's maximum supported sizes, we just don't query for framerates (since none of the drivers actually used the framerate parameter).

DXVA2Manager::SupportsConfig checks with DXVA for the given size, we just need to check if the AMD driver version if new enough and then also pass the framerate into DXVA rather than using the hardcoded check.
Flags: needinfo?(matt.woodrow)
I had a bit of a look into this.

It looks like we need to start with bug 1227017 (and maybe propose the spec change to get this added).

It looks simple, we just need to add parser.GetParameter("width") (and same for height) in MediaSource::IsTypeSupported and then pass those down into CanHandleCodecsType -> IsMP4SupportedType -> PDMFactory::SupportsMimeType -> PlatformDecoderModule::SupportsMimeType.

For the WMF implementation of PlatformDecoderModule::SupportsMimeType we seem to have two options:

 * Hardcode the max sizes allowed (documented at [1] as being 4096 Ă— 2304, or 1920 Ă— 1088 for win7, unsure how reliable/futureproof these are).
 * Attempt to create and initialize a dummy decoder with the specified size and see if it works.

WMF doesn't actually use the size we initialize with currently, so the second option won't work as is.

When we setup the MediaFoundation input/output types [2], we can also specify a callback that configures the size (using mImageSize), we do this for the DXVA MF transform here [3].

I believe doing that would cause Init() to fail, and then we'd successfully report to content that we can't play 8k h264 video.



[1] https://msdn.microsoft.com/en-us/library/windows/desktop/dd797815(v=vs.85).aspx
[2] http://searchfox.org/mozilla-central/source/dom/media/platforms/wmf/WMFVideoMFTManager.cpp#517
[3] http://searchfox.org/mozilla-central/source/dom/media/platforms/wmf/DXVA2Manager.cpp#905
(In reply to Matt Woodrow (:mattwoodrow) from comment #17)
> I had a bit of a look into this.
> 
> It looks like we need to start with bug 1227017 (and maybe propose the spec
> change to get this added).
> 
> It looks simple, we just need to add parser.GetParameter("width") (and same
> for height) in MediaSource::IsTypeSupported and then pass those down into
> CanHandleCodecsType -> IsMP4SupportedType -> PDMFactory::SupportsMimeType ->
> PlatformDecoderModule::SupportsMimeType.
> 
> For the WMF implementation of PlatformDecoderModule::SupportsMimeType we
> seem to have two options:
> 
>  * Hardcode the max sizes allowed (documented at [1] as being 4096 Ă— 2304,
> or 1920 Ă— 1088 for win7, unsure how reliable/futureproof these are).
>  * Attempt to create and initialize a dummy decoder with the specified size
> and see if it works.
> 
> WMF doesn't actually use the size we initialize with currently, so the
> second option won't work as is.

Setting up the size would be an easy thing to do, and in fact something I would like to do so we don't rely on the WMF own SPS parsing.

When the input type is created, you can set the frame size. This cause the stream_change message to not be called, which is where we currently set the dxva decoder.

We could do all this prior creating the MFT.

In fact this is the recommended approach by MS documentations: provide as much info known about the stream 

It would also greatly simplify the code. With no need to check again that the dimensions is valid. Recalculate the aspect ratio.
This work has already been done by the SPS parser and H264Converter. Additionally, it would make it consistent across all decoders.
I agree, then we can just assert the geometry changed error message happens.

It seems a little tricky though, look at this code: http://searchfox.org/mozilla-central/source/dom/media/platforms/wmf/WMFUtils.cpp#90

We need to make sure we have all those possible values set in the media type.

We'd need to find/make files that exercise those paths to make sure our version is correct.
Sorry, assert that it *never* happens.
(In reply to Matt Woodrow (:mattwoodrow) from comment #19)
> I agree, then we can just assert the geometry changed error message happens.
> 
> It seems a little tricky though, look at this code:
> http://searchfox.org/mozilla-central/source/dom/media/platforms/wmf/WMFUtils.
> cpp#90
> 
> We need to make sure we have all those possible values set in the media type.

That info is all the stuff found in the SPS.
The cropping is in the https://dxr.mozilla.org/mozilla-central/source/media/libstagefright/binding/H264.cpp#164

The various weird SAR the WMFUtils handle is found in the vui data and handle there:
https://dxr.mozilla.org/mozilla-central/source/media/libstagefright/binding/H264.cpp#237

When I wrote the SPS parser, I actually ran my windows copy with an assert that the value returned by the MFT were identical to the one found in the SPS. It never got triggered.

This is describe there:
https://msdn.microsoft.com/en-us/library/windows/desktop/dd797815(v=vs.85).aspx

if you set MF_MT_FRAME_RATE and MF_MT_FRAME_SIZE with the value found in the SPS, (I don't believe MF_MT_PIXEL_ASPECT_RATIO matter) then ProcessOutput never returns MF_E_TRANSFORM_STREAM_CHANGE.

So we will have to change how things are created/initialise.
I probably should have explained the cunning plan before pushing for review!

The patches under review here are the very first part, which only uses the extra size parameters to prevent too-high MP4 resolutions on Windows, based on known limitations. In particular, it will only allow 8K Youtube videos on Windows 8 and later.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff41d391263dd234fab69ce6b0ba00b36c5ca4be

After this lands, I will open a follow-up bug to continue this work further if we want to.
Comment on attachment 8796836 [details]
Bug 1176218 - p3. Add DecoderTraits::CanHandleContentType -

https://reviewboard.mozilla.org/r/82558/#review81152

I don't think we want this step.

::: dom/media/DecoderTraits.h:36
(Diff revision 1)
>  public:
> +  // Returns the CanPlayStatus indicating if we can handle this content type.
> +  static CanPlayStatus CanHandleContentType(const nsAString& aType,
> +                                            DecoderDoctorDiagnostics* aDiagnostics);
> +  // Returns the CanPlayStatus indicating if we can handle this parsed content type.
> +  static CanPlayStatus CanHandleContentType(nsContentTypeParser& aParsedType,

i would reshuffle things around instead.

i dont believe we should pass a reference to nsContentTypeParser. instead use your new MediaContentType everywhere.

Make your 2nd patch the first one in the series

I find nsContentTypeParser woefully inadequate, especially as at first glance I see no reason why you don't use a const reference.
Attachment #8796836 - Flags: review?(jyavenard) → review-
Comment on attachment 8796837 [details]
Bug 1176218 - p4. Use DecoderTraits::CanHandleContentType in HTMLMediaElement -

https://reviewboard.mozilla.org/r/82560/#review81154

Same as P1... Using the MediaContentType (or whatever name it will end up being)
Attachment #8796837 - Flags: review?(jyavenard) → review-
Comment on attachment 8796836 [details]
Bug 1176218 - p3. Add DecoderTraits::CanHandleContentType -

https://reviewboard.mozilla.org/r/82558/#review81152

> i would reshuffle things around instead.
> 
> i dont believe we should pass a reference to nsContentTypeParser. instead use your new MediaContentType everywhere.
> 
> Make your 2nd patch the first one in the series
> 
> I find nsContentTypeParser woefully inadequate, especially as at first glance I see no reason why you don't use a const reference.

The 2nd patch depends on the first one, so swapping them doesn't make sense to me (I always try to keep things compilable when applying patches one-by-one.)

nsContentTypeParser's methods are not const, that's why I needed to pass it as non-const reference.

But anyway, I guess using the new MediaContentType everywhere makes sense; I was just trying to limit its spread in this bug.
So I'll rework all this and re-submit...
Comment on attachment 8796841 [details]
Bug 1176218 - p2. New MediaContentType class to parse content type string -

https://reviewboard.mozilla.org/r/82568/#review81150

I think those should be used everywhere we need to determine a mimetype or with function returning being able to play or handle. Be it a content mimetype or a codec mimetype (see bug 1041861).

This should be your first patch. And have everything else use that new type. Shouldn't be using nsContentTypeParser especially if you can't pass it as const.

::: dom/media/MediaDecoder.h:22
(Diff revision 1)
>  #include "mozilla/dom/AudioChannelBinding.h"
>  
>  #include "necko-config.h"
>  #include "nsAutoPtr.h"
>  #include "nsCOMPtr.h"
> +#include "nsContentTypeParser.h"

this seems out of scope for this patch.
Attachment #8796841 - Flags: review?(jyavenard) → review+
Comment on attachment 8796844 [details]
Bug 1176218 - Reject resolutions that are too big for Windows -

https://reviewboard.mozilla.org/r/82574/#review81158
Attachment #8796844 - Flags: review?(jyavenard) → review+
See Also: → 1041861
Comment on attachment 8796836 [details]
Bug 1176218 - p3. Add DecoderTraits::CanHandleContentType -

https://reviewboard.mozilla.org/r/82558/#review81152

> The 2nd patch depends on the first one, so swapping them doesn't make sense to me (I always try to keep things compilable when applying patches one-by-one.)
> 
> nsContentTypeParser's methods are not const, that's why I needed to pass it as non-const reference.
> 
> But anyway, I guess using the new MediaContentType everywhere makes sense; I was just trying to limit its spread in this bug.
> So I'll rework all this and re-submit...

I meant the MediaContentType one
Comment on attachment 8796844 [details]
Bug 1176218 - Reject resolutions that are too big for Windows -

https://reviewboard.mozilla.org/r/82574/#review81208

::: dom/media/fmp4/MP4Decoder.cpp:143
(Diff revision 2)
>        // Some unsupported codec.
>        return false;
>      }
>    }
>  
> +#ifdef XP_WIN

Open discussion.

The job to know on what a platform can be decoded should be left to the inner PlatformDecoderModule / MediaDataDecoder.
Attachment #8796844 - Flags: review+ → review-
Comment on attachment 8796836 [details]
Bug 1176218 - p3. Add DecoderTraits::CanHandleContentType -

https://reviewboard.mozilla.org/r/82558/#review81210

::: dom/media/DecoderTraits.h:19
(Diff revision 2)
>  
>  namespace mozilla {
>  
>  class AbstractMediaDecoder;
>  class DecoderDoctorDiagnostics;
> +class MediaContentType;

I believe that any type required by a public member defined in a header should be fully qualified in that header.

To use CanHandleContentType defined in DecoderTraits.h; I shouldn't have to also have to do #include MediaContentType.h

(that goes for DecoderDoctorDiagnostic btw)

::: dom/media/DecoderTraits.cpp:505
(Diff revision 2)
> +{
> +  if (!aContentType.IsValid()) {
> +    return CANPLAY_NO;
> +  }
> +
> +  return DecoderTraits::CanHandleMediaType(aContentType.GetMIMEType().Data(),

you don't need the DecoderTraits:: ; you're already in that namespace.
Attachment #8796836 - Flags: review?(jyavenard) → review+
Comment on attachment 8796837 [details]
Bug 1176218 - p4. Use DecoderTraits::CanHandleContentType in HTMLMediaElement -

https://reviewboard.mozilla.org/r/82560/#review81212

::: dom/html/HTMLMediaElement.cpp:94
(Diff revision 2)
>  #include "mozilla/dom/TextTrack.h"
>  #include "nsIContentPolicy.h"
>  #include "mozilla/Telemetry.h"
>  #include "DecoderDoctorDiagnostics.h"
> +#include "DecoderTraits.h"
> +#include "MediaContentType.h"

you shouldn't have to include MediaContentType.h IMHO.
Attachment #8796837 - Flags: review?(jyavenard) → review+
Comment on attachment 8796838 [details]
Bug 1176218 - p5. Use DecoderTraits::CanHandleContentType in MediaSource -

https://reviewboard.mozilla.org/r/82562/#review81214

::: dom/media/mediasource/MediaSource.cpp:101
(Diff revision 2)
> -        if (hasCodecs &&
> -            DecoderTraits::CanHandleCodecsType(mimeTypeUTF8.get(),
> -                                               codecs,
> -                                               aDiagnostics) == CANPLAY_NO) {
> +
> +  // Now we know that this media type could be played.
> +  // MediaSource imposes extra restrictions, and some prefs.
> +  const nsACString& mimeType = contentType.GetMIMEType();
> +  if (mimeType.EqualsASCII("video/mp4") || mimeType.EqualsASCII("audio/mp4")) {
> +    if (!Preferences::GetBool("media.mediasource.mp4.enabled", false)) {

can't we do this within a single if ?

::: dom/media/mediasource/MediaSource.cpp:238
(Diff revision 2)
> -  if (NS_FAILED(rv)) {
>      aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
>      return nullptr;
>    }
> -  RefPtr<SourceBuffer> sourceBuffer = new SourceBuffer(this, NS_ConvertUTF16toUTF8(mimeType));
> +  const nsACString& mimeType = contentType.GetMIMEType();
> +  RefPtr<SourceBuffer> sourceBuffer = new SourceBuffer(this, mimeType);

Would be good to pass the full information to the sourcebuffer too..
Attachment #8796838 - Flags: review?(jyavenard) → review+
Comment on attachment 8796839 [details]
Bug 1176218 - p6. Move ShouldHandleMediaType down the cpp -

https://reviewboard.mozilla.org/r/82564/#review81216
Attachment #8796839 - Flags: review?(jyavenard) → review+
Comment on attachment 8796840 [details]
Bug 1176218 - p7. Hide CanHandle{Media|Codecs}Type -

https://reviewboard.mozilla.org/r/82566/#review81218
Attachment #8796840 - Flags: review?(jyavenard) → review+
Comment on attachment 8796842 [details]
Bug 1176218 - p8. Use MediaContentType in DecoderTraits -

https://reviewboard.mozilla.org/r/82570/#review81220

::: dom/media/DecoderTraits.cpp:389
(Diff revision 2)
>      return CANPLAY_MAYBE;
>    }
>  
>    // See http://www.rfc-editor.org/rfc/rfc4281.txt for the description
>    // of the 'codecs' parameter
> -  nsCharSeparatedTokenizer tokenizer(aRequestedCodecs, ',');
> +  nsCharSeparatedTokenizer tokenizer(aType.GetCodecs(), ',');

couldn't we move this job to the MediaContentType?
Attachment #8796842 - Flags: review?(jyavenard) → review+
Comment on attachment 8796843 [details]
Bug 1176218 - p9. Pass MediaContentType to MP4Decoder::CanHandleMediaType -

https://reviewboard.mozilla.org/r/82572/#review81222

::: dom/media/DecoderTraits.cpp:263
(Diff revision 2)
> +                   DecoderDoctorDiagnostics* aDiagnostics)
> +{
> +  return MP4Decoder::CanHandleMediaType(aParsedType, aDiagnostics);
> +}
> +static bool
>  IsMP4SupportedType(const nsACString& aType,

I think we should stop using nsACString to pass all this stuff (that applies to the previous patch too).

Why not modify all those functions to take a MediaContentType ?

MediaContentType to rule them all!

::: dom/media/fmp4/MP4Decoder.h:15
(Diff revision 2)
>  #include "MediaFormatReader.h"
>  #include "mozilla/dom/Promise.h"
>  
>  namespace mozilla {
>  
> +class MediaContentType;

same thing about forward declaration for a type used in a public member.
Attachment #8796843 - Flags: review?(jyavenard) → review+
Comment on attachment 8796836 [details]
Bug 1176218 - p3. Add DecoderTraits::CanHandleContentType -

https://reviewboard.mozilla.org/r/82558/#review81210

> I believe that any type required by a public member defined in a header should be fully qualified in that header.
> 
> To use CanHandleContentType defined in DecoderTraits.h; I shouldn't have to also have to do #include MediaContentType.h
> 
> (that goes for DecoderDoctorDiagnostic btw)

This rule makes sense in principle, but:
- For consistency I'd have to change all the other forward declarations to #include's?
- In the case of DecoderDoctorDiagnostics, I would argue that a forward declaration is fine anyway, because it is allowed to pass a nullptr; i.e. the caller may not necessarily need to construct a full object.
- And in other cases, when passing by pointer or reference, the immediate caller may actually be forwarding a pointer/reference as well; i.e., the caller may not necessarily need to construct a full object either.
- Checkmate: Our Coding Style explicitly prefer forward declarations to #include's:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices

So I'll keep the forward declarations for now, unless you can get a change of coding style enacted. ;-)
Comment on attachment 8796838 [details]
Bug 1176218 - p5. Use DecoderTraits::CanHandleContentType in MediaSource -

https://reviewboard.mozilla.org/r/82562/#review81214

> can't we do this within a single if ?

We have 'if (test1) { if (test2) { return X; } return Y; }, so I don't think we can combine the two tests here.

> Would be good to pass the full information to the sourcebuffer too..

Agreed, but this would be a massive change (or open the floodgates), and I think it is definitely out of scope in this bug.

How about we open a meta-bug to replace all MIME/codec/etc. strings with proper types with even better APIs, and this would be one of the sub-bugs? I'd be up for this project!
Comment on attachment 8796842 [details]
Bug 1176218 - p8. Use MediaContentType in DecoderTraits -

https://reviewboard.mozilla.org/r/82570/#review81220

> couldn't we move this job to the MediaContentType?

I didn't want to redo the whole processing in this bug, trying to keep changes less risky, but this would be a great job as part of the "replace all MIME strings with types" work.
Comment on attachment 8796843 [details]
Bug 1176218 - p9. Pass MediaContentType to MP4Decoder::CanHandleMediaType -

https://reviewboard.mozilla.org/r/82572/#review81222

> I think we should stop using nsACString to pass all this stuff (that applies to the previous patch too).
> 
> Why not modify all those functions to take a MediaContentType ?
> 
> MediaContentType to rule them all!

This one is used by other public methods that still take strings (e.g. InstantiateDecoder, CreateReader, etc.).
I'm sure it will naturally happen in the upcoming "change all MIME strings to types" project. ;-)
Comment on attachment 8796844 [details]
Bug 1176218 - Reject resolutions that are too big for Windows -

https://reviewboard.mozilla.org/r/82574/#review81208

> Open discussion.
> 
> The job to know on what a platform can be decoded should be left to the inner PlatformDecoderModule / MediaDataDecoder.

Makes sense, yes.
Let's discuss with the team, whether we want to isolate this non-standard change to MediaSource::IsTypeSupported(), or bury the check deeper inside the platform decoder. Then I'll rework this part.
Comment on attachment 8796844 [details]
Bug 1176218 - Reject resolutions that are too big for Windows -

https://reviewboard.mozilla.org/r/82574/#review81208

> Makes sense, yes.
> Let's discuss with the team, whether we want to isolate this non-standard change to MediaSource::IsTypeSupported(), or bury the check deeper inside the platform decoder. Then I'll rework this part.

regardless of limiting where the check is done (MSE or plain media), it is the responsibility of the PDM to know what it can play.
Not have extra code in an unrelated location like MP4Decoder
Comment on attachment 8796906 [details]
Bug 1176218 - p1. Constify nsContentTypeParser::Get... methods -

https://reviewboard.mozilla.org/r/82598/#review81380

I'm not really a DOM peer, but I feel pretty safe r+'ing this code.  r=me with the below coding style fixes addressed.

::: dom/base/nsContentUtils.cpp:6132
(Diff revision 1)
>    NS_IF_RELEASE(mService);
>  }
>  
>  nsresult
>  nsContentTypeParser::GetParameter(const char* aParameterName, nsAString& aResult)
> +  const

Nit: this is unusual indentation; `git grep -C 1 '^ *const$'` shows a very small number of instances of this pattern throughout the tree, though I can understand the line-breaking here.  I think the more usual convention would be:

```
nsContentTypeParser::GetParameter(const char* ...,
                                  nsAString& ...) const
```

::: dom/base/nsContentUtils.cpp:6142
(Diff revision 1)
>                                      aResult);
>  }
>  
>  nsresult
>  nsContentTypeParser::GetType(nsAString& aResult)
> +  const

Nit: same thing here.
Attachment #8796906 - Flags: review?(nfroyd) → review+
Attachment #8796844 - Attachment is obsolete: true
Comment on attachment 8797377 [details]
Bug 1176218 - p11. PDM's Supports(TrackInfo) -

https://reviewboard.mozilla.org/r/82978/#review81986

::: dom/media/platforms/PDMFactory.h:15
(Diff revision 1)
>  #include "PlatformDecoderModule.h"
>  #include "mozilla/Function.h"
>  #include "mozilla/StaticMutex.h"
>  
>  class CDMProxy;
> +class TrackInfo;

Its already included in the PDM.H

::: dom/media/platforms/PDMFactory.h:64
(Diff revision 1)
>    bool StartupPDM(PlatformDecoderModule* aPDM);
>    // Returns the first PDM in our list supporting the mimetype.
>    already_AddRefed<PlatformDecoderModule>
>    GetDecoder(const nsACString& aMimeType,
>               DecoderDoctorDiagnostics* aDiagnostics) const;
> +  already_AddRefed<PlatformDecoderModule>

GETdecoder is private. I dont see the need for having another implementation. please remove the one using a string and use the one taking a trackinfo

::: dom/media/platforms/PDMFactory.cpp:301
(Diff revision 1)
>    return !!current;
>  }
>  
> +bool
> +PDMFactory::Supports(const TrackInfo& aTrackInfo,
> +                     DecoderDoctorDiagnostics* aDiagnostics) const

All that duplicated code is ugly. And make us having to support two versions of the same thing. Please make them share the common code

::: dom/media/platforms/PDMFactory.cpp:439
(Diff revision 1)
> +  if (aDiagnostics) {
> +    // If libraries failed to load, the following loop over mCurrentPDMs
> +    // will not even try to use them. So we record failures now.
> +    if (mWMFFailedToLoad) {
> +      aDiagnostics->SetWMFFailedToLoad();
> +    }

Same here
Attachment #8797377 - Flags: review?(jyavenard) → review-
Comment on attachment 8797378 [details]
Bug 1176218 - p12. Use new PDM's Supports(Trackinfo) in MP4Decoder -

https://reviewboard.mozilla.org/r/82980/#review82002

::: dom/media/fmp4/MP4Decoder.cpp:84
(Diff revision 1)
>            profile == H264_PROFILE_HIGH);
>  }
>  
> +// Build a VideoInfo with the given MIME type, and whatever else is present
> +// in the given MediaContentType (except for MIME and codecs).
> +static UniquePtr<TrackInfo>

you should put this in MediaInfo.* as it will be useful everywhere else. Make that a specislised onstructor of AudioInfo and VideoInfo.
That way you can simplify the PDMFactory::GetDecoder private function

::: dom/media/fmp4/MP4Decoder.cpp:105
(Diff revision 1)
> +
> +// Build a VideoInfo with the given MIME type, and whatever else is present
> +// in the given MediaContentType (except for MIME and codecs).
> +static UniquePtr<TrackInfo>
> +BuildAudioInfo(const char* aMIME,
> +               const MediaContentType& aExtraContentType)

That sound non intuitive to me. Why having to pass two objects with both containing a mimetype.
Can't you only pass MediaContentType? building it as necessary (add a constructor taking a plain string)

If they are different mimetype (coded vs container) then a comment need to reflect that as well as the parameter name
Attachment #8797378 - Flags: review?(jyavenard) → review-
Comment on attachment 8797379 [details]
Bug 1176218 - p13. Reject Resolutions that are too big for Windows' decoder -

https://reviewboard.mozilla.org/r/82982/#review82010
Attachment #8797379 - Flags: review?(jyavenard) → review+
Comment on attachment 8797377 [details]
Bug 1176218 - p11. PDM's Supports(TrackInfo) -

https://reviewboard.mozilla.org/r/82978/#review81986

> GETdecoder is private. I dont see the need for having another implementation. please remove the one using a string and use the one taking a trackinfo

Because it will be a huge pain to build the correct VideoInfo or TrackInfo based on a string!
I'd prefer temporary duplication, rather than having to create a new mess of code that we will discard soon anyway.

> All that duplicated code is ugly. And make us having to support two versions of the same thing. Please make them share the common code

...

> Same here

Same thing, it's not trivial to create common code here, which we'll have to remove later on.
Comment on attachment 8797378 [details]
Bug 1176218 - p12. Use new PDM's Supports(Trackinfo) in MP4Decoder -

https://reviewboard.mozilla.org/r/82980/#review82002

> you should put this in MediaInfo.* as it will be useful everywhere else. Make that a specislised onstructor of AudioInfo and VideoInfo.
> That way you can simplify the PDMFactory::GetDecoder private function

I don't quite understand what you mean, you're pointing at 'static UniquePtr<TrackInfo>'.
I'm not sure a specialized constructor (or do you mean static method?) that returns a UniquePtr<TrackInfo> is that useful.

> That sound non intuitive to me. Why having to pass two objects with both containing a mimetype.
> Can't you only pass MediaContentType? building it as necessary (add a constructor taking a plain string)
> 
> If they are different mimetype (coded vs container) then a comment need to reflect that as well as the parameter name

Well, I need to combine a new MIME type with some of the information from the passed MediaContentType.
I guess I could make a copy of the original and modify its MIME type & codecs, is that what you mean?
Comment on attachment 8796841 [details]
Bug 1176218 - p2. New MediaContentType class to parse content type string -

https://reviewboard.mozilla.org/r/82568/#review82648

::: dom/media/MediaContentType.h:36
(Diff revision 4)
> +  bool HaveCodecs() const { return mHaveCodecs; }
> +  // Codecs. May be empty if not provided or explicitly provided as empty.
> +  const nsAString& GetCodecs() const { return mCodecs; }
> +
> +  // Sizes and rates. -1 if not provided.
> +  int32_t GetWidth() const { return mWidth; }

Sorry I should have mentioned this earlier.

The accepted convention is that a GetBlah is only for functions that could fail and return an error

So please rename those:
Width()
Height()
FrameRate()
Bitrate()
Comment on attachment 8798644 [details]
Bug 1176218 - p10. VideoUtils' CreateTrackInfo*() -

https://reviewboard.mozilla.org/r/84092/#review82650

I think MediaContentType shouldn't be using different mimetype content, they are unrelated. It only adds to confusion.

e.g. a mp4 containing an aac track would have a mimetype of: "audio/mp4; codecs=mp4a.40.29" ; the corresponding codec mimetype is audio/mp4a-latm

It's confusive enough already and a great source of errors.

::: dom/media/MediaContentType.h:44
(Diff revision 1)
>    int32_t GetHeight() const { return mHeight; }
>    int32_t GetFramerate() const { return mFramerate; }
>    int32_t GetBitrate() const { return mBitrate; }
>  
> +  // Try and create an appropriate TrackInfo (codecs are ignored).
> +  UniquePtr<TrackInfo> CreateTrackInfo() const;

GetTrackInfo() then...

::: dom/media/MediaContentType.h:48
(Diff revision 1)
> +  // Try and create an appropriate TrackInfo (codecs are ignored).
> +  UniquePtr<TrackInfo> CreateTrackInfo() const;
> +  // Try and create an appropriate TrackInfo, but with a different MIME type
> +  // (and codecs are ignored).
> +  UniquePtr<TrackInfo>
> +  CreateTrackInfoWithMIMEType(const nsACString& aMIMEType) const;

this should be a static entry, it makes no use of this.

But having said that.. I think this is the wrong place to have it..

ContentMediaType is used for a mimetype as defined in RFC2046 (https://tools.ietf.org/html/rfc2046).

TrackInfo's mimetype is a totally different kind of format, and only used to identify a codec. I don't believe there's a single RFC to define those, instead it's a registry where codecs are registered.
Attachment #8798644 - Flags: review?(jyavenard) → review-
Comment on attachment 8797377 [details]
Bug 1176218 - p11. PDM's Supports(TrackInfo) -

https://reviewboard.mozilla.org/r/82978/#review82654

so long as we don't use any reference to ContentMediaType there...
What about a new class CodecMediaType instead?

::: dom/media/platforms/PDMFactory.cpp:298
(Diff revision 2)
>  bool
>  PDMFactory::SupportsMimeType(const nsACString& aMimeType,
>                               DecoderDoctorDiagnostics* aDiagnostics) const
>  {
> +  UniquePtr<TrackInfo> trackInfo =
> +    MediaContentType(aMimeType).CreateTrackInfo();

unfortunate it has to be created on the heap, but I can't think of a better way...
Attachment #8797377 - Flags: review?(jyavenard) → review+
Comment on attachment 8797378 [details]
Bug 1176218 - p12. Use new PDM's Supports(Trackinfo) in MP4Decoder -

https://reviewboard.mozilla.org/r/82980/#review82656

I'll just look into this one once MediaContentType is no longer used to reference codec mimetype
Attachment #8797378 - Flags: review?(jyavenard) → review-
Comment on attachment 8796841 [details]
Bug 1176218 - p2. New MediaContentType class to parse content type string -

https://reviewboard.mozilla.org/r/82568/#review82648

> Sorry I should have mentioned this earlier.
> 
> The accepted convention is that a GetBlah is only for functions that could fail and return an error
> 
> So please rename those:
> Width()
> Height()
> FrameRate()
> Bitrate()

As discussed, for me these getters do have a failure value, which is -1 here. I think 'Get' helps users take note of that so they don't expect these numbers to be correct.
(In reply to Gerald Squelart [:gerald] from comment #99)
> As discussed, for me these getters do have a failure value, which is -1
> here. I think 'Get' helps users take note of that so they don't expect these
> numbers to be correct.

Perhaps you could return Maybe<ValueType> so that the compiler encourages the user to remember?
(In reply to Nathan Froyd [:froydnj] from comment #100)
> (In reply to Gerald Squelart [:gerald] from comment #99)
> > As discussed, for me these getters do have a failure value, which is -1
> > here. I think 'Get' helps users take note of that so they don't expect these
> > numbers to be correct.
> 
> Perhaps you could return Maybe<ValueType> so that the compiler encourages
> the user to remember?

I was hoping no one would notice...

I'm actually using the fact that the failure/nothing value is -1, because I'm only interested if these numbers are too big (and -1 is never too big).
I'll consider it, thank you for the advice.

Now, following this logic, does it mean pointer-getters should always return Maybe<NotNull<PointerType>>? :-P
(In reply to Gerald Squelart [:gerald] from comment #101)
> Now, following this logic, does it mean pointer-getters should always return
> Maybe<NotNull<PointerType>>? :-P

Tony Hoare would be happy. :p  (https://en.wikipedia.org/wiki/Tony_Hoare#Apologies_and_retractions)
Comment on attachment 8798644 [details]
Bug 1176218 - p10. VideoUtils' CreateTrackInfo*() -

https://reviewboard.mozilla.org/r/84092/#review82650

> GetTrackInfo() then...

It's not a getter. :-)

> this should be a static entry, it makes no use of this.
> 
> But having said that.. I think this is the wrong place to have it..
> 
> ContentMediaType is used for a mimetype as defined in RFC2046 (https://tools.ietf.org/html/rfc2046).
> 
> TrackInfo's mimetype is a totally different kind of format, and only used to identify a codec. I don't believe there's a single RFC to define those, instead it's a registry where codecs are registered.

Ok I'll move it to a free function in VideoUtils.h.
Comment on attachment 8798644 [details]
Bug 1176218 - p10. VideoUtils' CreateTrackInfo*() -

https://reviewboard.mozilla.org/r/84092/#review82718

::: dom/media/VideoUtils.h:361
(Diff revision 2)
> +CreateTrackInfoWithMIMEType(const nsACString& aMIMEType);
> +
> +// Try and create a TrackInfo with a given codec MIME type, and optional extra
> +// parameters from a content type (its MIME type and codecs are ignored).
> +UniquePtr<TrackInfo>
> +CreateTrackInfoWithMIMETypeAndContentTypeExtraParameters(

I think the ExtraParameters doesnt add much, and make the name impossible to remember :)

::: dom/media/VideoUtils.h:362
(Diff revision 2)
> +
> +// Try and create a TrackInfo with a given codec MIME type, and optional extra
> +// parameters from a content type (its MIME type and codecs are ignored).
> +UniquePtr<TrackInfo>
> +CreateTrackInfoWithMIMETypeAndContentTypeExtraParameters(
> +  const nsACString& aMIMEType,

could we start calling them CodingMIME from now on?

::: dom/media/VideoUtils.cpp:513
(Diff revision 2)
> +{
> +  UniquePtr<TrackInfo> trackInfo = CreateTrackInfoWithMIMEType(aMIMEType);
> +  if (trackInfo) {
> +    VideoInfo* videoInfo = trackInfo->GetAsVideoInfo();
> +    if (videoInfo) {
> +      if (aContentType.GetWidth() > 0) {

What happened to the Maybe<uint32_t> ???
Attachment #8798644 - Flags: review?(jyavenard) → review+
Comment on attachment 8797378 [details]
Bug 1176218 - p12. Use new PDM's Supports(Trackinfo) in MP4Decoder -

https://reviewboard.mozilla.org/r/82980/#review82720

::: dom/media/fmp4/MP4Decoder.cpp:156
(Diff revision 3)
>    }
>  
>    // Verify that we have a PDM that supports the whitelisted types.
>    RefPtr<PDMFactory> platform = new PDMFactory();
> -  for (const nsCString& codecMime : codecMimes) {
> -    if (!platform->SupportsMimeType(codecMime, aDiagnostics)) {
> +  for (const auto& trackInfo : trackInfos) {
> +    if (!trackInfo || !platform->Supports(*trackInfo, aDiagnostics)) {

how could trackInfo ever be null?
Attachment #8797378 - Flags: review?(jyavenard) → review+
Comment on attachment 8797379 [details]
Bug 1176218 - p13. Reject Resolutions that are too big for Windows' decoder -

https://reviewboard.mozilla.org/r/82982/#review82722

::: dom/media/platforms/wmf/WMFDecoderModule.h:28
(Diff revision 4)
>    CreateVideoDecoder(const CreateDecoderParams& aParams) override;
>  
>    already_AddRefed<MediaDataDecoder>
>    CreateAudioDecoder(const CreateDecoderParams& aParams) override;
>  
>    bool SupportsMimeType(const nsACString& aMimeType,

can't you remove this method now?

and PDM::SupportsMimeType have a default implementation calling Supports?
Comment on attachment 8798644 [details]
Bug 1176218 - p10. VideoUtils' CreateTrackInfo*() -

https://reviewboard.mozilla.org/r/84092/#review82718

> I think the ExtraParameters doesnt add much, and make the name impossible to remember :)

It makes it very clear that we only use the *extra parameters* from the given MediaContentType (until we can build a separate type that would only contain these parameters).
So please propose another name that would avoid the possible confusion of giving both a MIME type and a MediaContentType, otherwise I'll just keep that one for now.

> could we start calling them CodingMIME from now on?

Do you mean 'aCodecMIMEType'?

> What happened to the Maybe<uint32_t> ???

I conveniently forgot about it. Ok I'll change it...
Comment on attachment 8797378 [details]
Bug 1176218 - p12. Use new PDM's Supports(Trackinfo) in MP4Decoder -

https://reviewboard.mozilla.org/r/82980/#review82720

> how could trackInfo ever be null?

CreateTrackInfoWithMIMETypeAndContentTypeExtraParameters could return a null UniquePtr.
In this particular case, I know it should not be possible (as all hard-coded strings start with 'audio/' or 'video/'), but I don't want to risk a future change to start crashing randomly with null dereferences, for the sake of skipping one null-check.
Comment on attachment 8797379 [details]
Bug 1176218 - p13. Reject Resolutions that are too big for Windows' decoder -

https://reviewboard.mozilla.org/r/82982/#review82722

> can't you remove this method now?
> 
> and PDM::SupportsMimeType have a default implementation calling Supports?

I'd prefer to keep PlatformDecoderModule::SupportsMimeType() a pure virtual method, so that a PDM developer won't forget to implement it.
Can we deal with removing SupportsMimeType in another bug?
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5c82a43c23a8
p1. Constify nsContentTypeParser::Get... methods - r=froydnj
https://hg.mozilla.org/integration/autoland/rev/0bed1d9a4bb2
p2. New MediaContentType class to parse content type string - r=jya
https://hg.mozilla.org/integration/autoland/rev/989afc4cbc44
p3. Add DecoderTraits::CanHandleContentType - r=jya
https://hg.mozilla.org/integration/autoland/rev/0578fd79f0f0
p4. Use DecoderTraits::CanHandleContentType in HTMLMediaElement - r=jya
https://hg.mozilla.org/integration/autoland/rev/6e810bc9458a
p5. Use DecoderTraits::CanHandleContentType in MediaSource - r=jya
https://hg.mozilla.org/integration/autoland/rev/98a2b908e5bf
p6. Move ShouldHandleMediaType down the cpp - r=jya
https://hg.mozilla.org/integration/autoland/rev/83eef2a47390
p7. Hide CanHandle{Media|Codecs}Type - r=jya
https://hg.mozilla.org/integration/autoland/rev/65057cb2febf
p8. Use MediaContentType in DecoderTraits - r=jya
https://hg.mozilla.org/integration/autoland/rev/7e010a48eb17
p9. Pass MediaContentType to MP4Decoder::CanHandleMediaType - r=jya
https://hg.mozilla.org/integration/autoland/rev/f04e39f68372
p10. VideoUtils' CreateTrackInfo*() - r=jya
https://hg.mozilla.org/integration/autoland/rev/2dd8be39ec85
p11. PDM's Supports(TrackInfo) - r=jya
https://hg.mozilla.org/integration/autoland/rev/82be75a84288
p12. Use new PDM's Supports(Trackinfo) in MP4Decoder - r=jya
https://hg.mozilla.org/integration/autoland/rev/4429c8701a84
p13. Reject Resolutions that are too big for Windows' decoder - r=jya
Depends on: 1321051
Flags: qe-verify+
I have reproduced this bug on Firefox Nightly according to(2015-06-19)

Fixing bug is verified on latest Firefox Beta -- Build ID: (20170220070057), User Agent: Mozilla/5.0 (Windows NT 10.0; rv:52.0) Gecko/20100101 Firefox/52.0

Tested OS-- Windows10 32bit
QA Whiteboard: [bugday-20170222]
Also confirming that this issue no longer reproduces on Mac OS X 10.11 and Ubuntu 14.04 x64 using Firefox 52 beta 9 (build ID: 20170223185858).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.