1259212 - Use AMD's CreateDecoder() extension to detect maximum playback decode capabilities for MediaSource.isTypeSupported()

Status: NEW

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

Description

[Chris Peterson [:cpeterson]]

AMD's new drivers (version 16.x and above) allow applications to query CreateDecoder() for the maximum accelerated framerate for any playback format and resolution. Firefox can use this information to implement YouTube's MediaSource.isTypeSupported() extensions (bug 1227017).

Anthony says cpearce or Matt Woodrow will probably work on this. jya has a patch to implement the MediaSource.isTypeSupported() side of this feature.

AMD Radeon Software Crimson Edition 16.3 release notes:

http://support.amd.com/en-us/kb-articles/Pages/AMD_Radeon_Software_Crimson_Edition_16.3.aspx

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Mass change P2 -> P3

Comment 2

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

Following bug 1176218 (which effectively implements the YouTube isTypeSupported width&height extensions described in bug 1227017), I'll have a go at this one.

Comment 3

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

Attachment: Bug 1259212 - Pass document from supports-type APIs down to WMF MP4 decoder -

This is needed because the DXVA drivers will require access to the compositor
(through the document) to determine whether certain resolutions&rates are
supported.

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

Comment 4

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

Attachment: Bug 1259212 - Add VideoInfo::mFramerate -

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

Comment 5

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

Attachment: Bug 1259212 - Extract framerate from MediaContentType into VideoInfo -

This will translate the framerate from container media types to decoder track
infos.

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

Comment 6

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

Attachment: Bug 1259212 - Small WMFDecoderModule::Supports refactoring -

No actual code change intended. This will make the next patch easier to review.

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

Comment 7

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

Attachment: Bug 1259212 - WMFDecoderModule::Supports tries to create decoder -

If we are given any size or rate information, outright reject insanely-high
values. This is actually used by YouTube to sense whether the browser reads
these parameters, in which case YouTube will then probe for exact size&rate
combinations.

If we are given a size&rate, try to create a decoder with the relevant track
informations, to see if the decoder really supports them.
Note that the actual decoder check will be implemented in later patches.

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

Comment 8

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

Attachment: Bug 1259212 - MediaPrefs::media.wmf.dxva.allow-sw-fallback -

This pref will control what to do when the DXVA (hardware) decoder is
available, but cannot decode a given size&rate, whether software callback
should be allowed.

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

Comment 9

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

Attachment: Bug 1259212 - Minor WMFVideoMFTManager refactor of output type creation -

No significant code change intended. This will make the next patch easier to
review.

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

Comment 10

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

Attachment: Bug 1259212 - WMFVideoMFTManager init checks for supported DXVA2 config -

If we are given a size&rate, and hardware acceleration is possible, check
through DXVA that this output configuration is actually supported.
Then depending on "media.wmf.dxva.allow-sw-fallback", either fail to initialize
the decoder (default), or revert to software decoding.

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

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8803742 [details]
Bug 1259212 - Pass document from supports-type APIs down to WMF MP4 decoder -

https://reviewboard.mozilla.org/r/87868/#review86842

::: dom/media/platforms/wmf/WMFDecoderModule.cpp:207
(Diff revision 1)
>  }
>  
>  bool
>  WMFDecoderModule::Supports(const TrackInfo& aTrackInfo,
> +                           nsIDocument* aDoc,
>                             DecoderDoctorDiagnostics* aDiagnostics) const

Since DecoderDoctorDiagnostics::StoreBlah also requires an nsIDocument* parameter, it might be worth to store it in DecoderDoctorDiagnostics so you don't have to change the signatures of many functions.

Comment 12

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8803743 [details]
Bug 1259212 - Add VideoInfo::mFramerate -

https://reviewboard.mozilla.org/r/87870/#review86844

::: dom/media/MediaInfo.h:304
(Diff revision 1)
>  
>    // Size of the decoded video's image.
>    nsIntSize mImage;
>  
> +  // Framerate in Hz. 0 if unknown.
> +  uint32_t mFramerate;

const?

Comment 13

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8803744 [details]
Bug 1259212 - Extract framerate from MediaContentType into VideoInfo -

https://reviewboard.mozilla.org/r/87872/#review86848

::: dom/media/VideoUtils.cpp:522
(Diff revision 1)
>        Maybe<int32_t> maybeHeight = aContentType.GetHeight();
>        if (maybeHeight && *maybeHeight > 0) {
>          videoInfo->mImage.height = *maybeHeight;
>        }
> +      Maybe<int32_t> maybeFramerate = aContentType.GetFramerate();
> +      if (maybeFramerate && *maybeFramerate > 0) {

Can be as simple as:
videoInfo->mFramerate = maybeFramerate.valueOr(0);

Comment 14

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

Comment on attachment 8803742 [details]
Bug 1259212 - Pass document from supports-type APIs down to WMF MP4 decoder -

https://reviewboard.mozilla.org/r/87868/#review86842

> Since DecoderDoctorDiagnostics::StoreBlah also requires an nsIDocument* parameter, it might be worth to store it in DecoderDoctorDiagnostics so you don't have to change the signatures of many functions.

It's an interesting idea, it would indeed be nice not to have to modify all these methods!

However I'm not sure it is possible here:
- In a few situations, there is no DecDoc (i.e., pointer is null), in which case we would not be able to access the document through it.
Now, it may be the case that we always have a DecDoc for the supports/can-play APIs, which is where we need this nsIDocument* for this bug; I'll need to investigate further before I can really be sure.

- DecDoc happens to need a document to do its work, but it seems a bit naughty to use that constraint as a way of passing a document around!
Though I guess if we changed the DecDoc interface to always be constructed with a document, and add a getter for that document, it would probably be ok in that case. I will also investigate this idea further.

Another possibility would be to do something similar to 'CreateDecoderParams':
Instead of passing 'DecoderDoctorDiagnostics*' around, we would pass a struct that can contain a set of parameters; In this case we would start with DecoderDoctorDiagnostics* and nsIDocument*'.
We would still need to change all these files, but it would be more trivial, and would allow adding more parameters in the future if needed.


I'll come back once I've had more time to think and test things.
In the meantime please let me know if you have any comments about the above.

Comment 15

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

Comment on attachment 8803743 [details]
Bug 1259212 - Add VideoInfo::mFramerate -

https://reviewboard.mozilla.org/r/87870/#review86844

> const?

The other public member variables (including the mImage size) are not const, so I'll stay consistent with that.

Comment 16

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

Comment on attachment 8803744 [details]
Bug 1259212 - Extract framerate from MediaContentType into VideoInfo -

https://reviewboard.mozilla.org/r/87872/#review86848

Thank you for all the reviews so far.

> Can be as simple as:
> videoInfo->mFramerate = maybeFramerate.valueOr(0);

True, but:
- I was trying to be consistent with the previous code.
- I would argue that the current tests are actually a bit more efficient (the '>' test will be short-circuited if there is no value).
- I think 'if (maybeX.valueOr(0) > 0)' is a bit "tricky", it makes it less obvious that we only really care if there's a value, and that value is strictly positive.
- I'd prefer to assign a value only if it's needed, otherwise I don't want to override what could be there.
- It is true that 0 is currently the default not-provided-framerate in VideoInfo, but if that were to change one day, our tricky 'valueOr(0)' could be missed!
So I'll keep this code as it is for now, unless you really think it would be worth the change.

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8803744 [details]
Bug 1259212 - Extract framerate from MediaContentType into VideoInfo -

https://reviewboard.mozilla.org/r/87872/#review86848

> True, but:
> - I was trying to be consistent with the previous code.
> - I would argue that the current tests are actually a bit more efficient (the '>' test will be short-circuited if there is no value).
> - I think 'if (maybeX.valueOr(0) > 0)' is a bit "tricky", it makes it less obvious that we only really care if there's a value, and that value is strictly positive.
> - I'd prefer to assign a value only if it's needed, otherwise I don't want to override what could be there.
> - It is true that 0 is currently the default not-provided-framerate in VideoInfo, but if that were to change one day, our tricky 'valueOr(0)' could be missed!
> So I'll keep this code as it is for now, unless you really think it would be worth the change.

For mFramerate to have a default value other than 0, you can do
videoInfo->mFramerate = maybeFramerate.valueOr(videoInfo->mFramerate);

It is more concise but less efficient. You call.

Comment 18

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8803745 [details]
Bug 1259212 - Small WMFDecoderModule::Supports refactoring -

https://reviewboard.mozilla.org/r/87874/#review87212

Comment 19

[Matt Woodrow (:mattwoodrow)]

CreateDecoder will fail on all drivers if the resolution is too big, and on newer-AMD drivers it will also fail for unsupported resolution+framerate combinations.

The main change in this bug is that we are going to start reporting that we don't support MP4/H264 if we can't support the requested resolution in hardware (and I guess we hope we have WebM/VP9 available to fall back to?).

That's quite a big change in behaviour, and not one that I feel comfortable reviewing. Not sure who wants to sign off on that, but let's start with Chris.

Comment 20

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8803746 [details]
Bug 1259212 - WMFDecoderModule::Supports tries to create decoder -

https://reviewboard.mozilla.org/r/87876/#review87214

::: dom/media/platforms/wmf/WMFDecoderModule.cpp:224
(Diff revision 1)
>      int32_t w = videoInfo->mImage.width;
>      int32_t h = videoInfo->mImage.height;
> +    uint32_t fr = videoInfo->mFramerate;
> +    if (w >= 99999 || h >= 99999 || fr >= 9999) {
> +      // Size/rate way too big.
> +      // Also signals to YouTube that we can deal with these values, so that

that we *can't* deal with these values?

::: dom/media/platforms/wmf/WMFDecoderModule.cpp:241
(Diff revision 1)
>        // Windows <=7 supports at most 1920x1088.
>        if (w > 1920 || h > 1088) {
>          return false;
>        }
>      }
> +    if (w > 0 && h > 0 && fr > 0) {

Treating any 0 as meaning the entire format is valid seems really weird to me.

Looking at comment 0 from bug 1227017, it looks like YouTube never passes width, height and framerate all at once, so won't this condition never be true?

Can we instead replace missing parameters with sensible defaults? This might mean using the specified dimension and a sane aspect ratio to compute the other dimension. It might also mean just using a small value like 32.

Comment 21

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8803747 [details]
Bug 1259212 - MediaPrefs::media.wmf.dxva.allow-sw-fallback -

https://reviewboard.mozilla.org/r/87878/#review87218

As mentioned in the bug, I don't want to make the call on what the default value of this pref is.

Comment 22

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8803748 [details]
Bug 1259212 - Minor WMFVideoMFTManager refactor of output type creation -

https://reviewboard.mozilla.org/r/87880/#review87220

Comment 23

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #19)
> CreateDecoder will fail on all drivers if the resolution is too big, and on
> newer-AMD drivers it will also fail for unsupported resolution+framerate
> combinations.
> 
> The main change in this bug is that we are going to start reporting that we
> don't support MP4/H264 if we can't support the requested resolution in
> hardware (and I guess we hope we have WebM/VP9 available to fall back to?).
> 
> That's quite a big change in behaviour, and not one that I feel comfortable
> reviewing. Not sure who wants to sign off on that, but let's start with
> Chris.

Thank you for your reviews so far, Matt.

Good point about the change in behaviour. I'd point out that comment 0 by Chris Peterson implied that that's what we wanted to do! And also discussing with Anthony.
And there is a pref to allow sw fallback (but then I'm not sure this whole bug would be useful if that was the default.)

Would you see another way to use the new AMD driver features For Good?

Comment 24

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8803749 [details]
Bug 1259212 - WMFVideoMFTManager init checks for supported DXVA2 config -

https://reviewboard.mozilla.org/r/87882/#review87222

This seems reasonable to me.

It would be nice if we can check for support during initialization, and then remove the other call to SupportsConfig. If we only ever use a single resolution with a given decoder (which I believe is true now), then we should be able to set the size initially, assert that the geometry changed callback never happens and get rid of the code for supporting it.

I think jya was looking into doing something like this, so I've added him as a reviewing to make sure you're not conflicting with his patches.

Comment 25

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

Comment on attachment 8803746 [details]
Bug 1259212 - WMFDecoderModule::Supports tries to create decoder -

https://reviewboard.mozilla.org/r/87876/#review87214

> that we *can't* deal with these values?

Maybe "deal" is the wrong verb, or "values" is the wrong object?
We want to let YouTube know that we do read these extra parameters, and that we will reply with appropriate yes/no answers.
I'll rephrase the comment.

> Treating any 0 as meaning the entire format is valid seems really weird to me.
> 
> Looking at comment 0 from bug 1227017, it looks like YouTube never passes width, height and framerate all at once, so won't this condition never be true?
> 
> Can we instead replace missing parameters with sensible defaults? This might mean using the specified dimension and a sane aspect ratio to compute the other dimension. It might also mean just using a small value like 32.

First, YouTube passes each parameter separately, with a crazy-high value (which is what lines 222-227 handle), this way it can "sniff" which of these, if any, the browser understands. E.g.: 'width=99999', then separately 'height=99999', etc.
Then, YouTube will give us complete combinations of actual resolution&rate it can offer, e.g. 'width=1024; height=768; framerate=30'. That's why I want all three values to be >0.

This code is really following YouTube's behaviour, I think we should stick with that for now, and possibly expand later on if other websites do different thing.

I'll also rephrase comments to make this whole process clearer.

Comment 26

[Matt Woodrow (:mattwoodrow)]

(In reply to Gerald Squelart [:gerald] from comment #23)

> Thank you for your reviews so far, Matt.
> 
> Good point about the change in behaviour. I'd point out that comment 0 by
> Chris Peterson implied that that's what we wanted to do! And also discussing
> with Anthony.
> And there is a pref to allow sw fallback (but then I'm not sure this whole
> bug would be useful if that was the default.)

I'm not saying that we shouldn't do it, I don't think I'm the right person to make the call (I'm not even a Media peer technically). If Anthony agrees, then I'm sure it's fine, just would be good to make the decision explicit in the bug.

> 
> Would you see another way to use the new AMD driver features For Good?

I just realized that we still don't use them yet.

We currently check SupportsConfig to decide if we can use DXVA for the current resolution, to avoid a bug where HD 60fps videos were decoded in hardware on older AMD cards, but performed awfully.

The implementations of SupportsConfig are using a hardcoded list of AMD device ids to detect cards that don't support HD 60fps. We should fix this to check if the AMD driver version is recent enough, and rely on CreateDecoder to detect this for us if it is.

Comment 27

[Matt Woodrow (:mattwoodrow)]

(In reply to Gerald Squelart [:gerald] from comment #25)
> 
> First, YouTube passes each parameter separately, with a crazy-high value
> (which is what lines 222-227 handle), this way it can "sniff" which of
> these, if any, the browser understands. E.g.: 'width=99999', then separately
> 'height=99999', etc.
> Then, YouTube will give us complete combinations of actual resolution&rate
> it can offer, e.g. 'width=1024; height=768; framerate=30'. That's why I want
> all three values to be >0.
> 
> This code is really following YouTube's behaviour, I think we should stick
> with that for now, and possibly expand later on if other websites do
> different thing.
> 
> I'll also rephrase comments to make this whole process clearer.

Alright, with that understanding then the current code makes a lot more sense :)

Probably worth making it clear in the comments that we're very much implementing the unspecified algorithm that YouTube uses for this detection. Is this something that could be added to a spec?

Comment 28

[Chris Peterson [:cpeterson]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #19)
> The main change in this bug is that we are going to start reporting that we
> don't support MP4/H264 if we can't support the requested resolution in
> hardware (and I guess we hope we have WebM/VP9 available to fall back to?).
> 
> That's quite a big change in behaviour, and not one that I feel comfortable
> reviewing. Not sure who wants to sign off on that, but let's start with
> Chris.

Good point. This is a big change. Rereading my comment 0, if we reject a video that is too big to be decoded by hardware, how do we know that the site will fall back to a lower, supported resolution? I guess this is safe if we assume any site (i.e. YouTube) that specifies isTypeSupported() resolution params will have multiple content resolutions available.

Comment 29

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

This is a YouTube specific feature and H.264 is only used in practice by people with slow machines that have hardware decode capability. We will end up with the following matrix:

Machines with blacklisted GPUs will expose VP9 and H.264 for all resolutions. YouTube will use VP9 for almost everything.

Fast machines (>=150fps@720p) expose VP9 for all resolutions. Whether we restrict H.264 to hardware capabilities doesn't matter much while this is YouTube specific. YouTube will use VP9 for almost everything.

Slow machines (<150fps@720p) will not expose VP9. They will expose H.264 for resolutions supported by hardware. YouTube will be limited to H.264 on resolutions supported by hardware decode.

Comment 30

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

Comment on attachment 8803746 [details]
Bug 1259212 - WMFDecoderModule::Supports tries to create decoder -

Removing review request for now, I need to revise the code, which may not happen for a little while yet...

Comment 31

[Ralph Giles (:rillian)]

BTW, there is also an `ICodecAPI` interface in Windows 8 and above which reports max resolution on Media Foundation codecs. https://msdn.microsoft.com/en-us/library/windows/desktop/ms701585(v=vs.85).aspx

Comment 32

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #19)
> CreateDecoder will fail on all drivers if the resolution is too big, and on
> newer-AMD drivers it will also fail for unsupported resolution+framerate
> combinations.
> 
> The main change in this bug is that we are going to start reporting that we
> don't support MP4/H264 if we can't support the requested resolution in
> hardware (and I guess we hope we have WebM/VP9 available to fall back to?).
> 
> That's quite a big change in behaviour, and not one that I feel comfortable
> reviewing. Not sure who wants to sign off on that, but let's start with
> Chris.

If I understand correctly, we'd only change behaviour for sites using the YouTube framerate extensions, which would then result in the desired behaviour for these sites, so this should be OK.

Comment 33

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

De-assigning myself from A/V bugs/tasks that I most probably won't touch anymore, so that someone else may have a look.