Closed Bug 1397307 Opened 7 years ago Closed 7 years ago

Invalid use of WMFVideoMFTManager::CanUseDXVA, and incorrect determination if DXVA is usable.

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Version:
55 Branch
Platform:
Unspecified
Windows
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 + fixed

People

(Reporter: jya, Assigned: jya)

References

Details

(Keywords: regression)

Attachments

(10 files)

Bug 1397307 - P1. Make method const.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1397307 - P2. Wrap boolean in structure to prevent unwanted conversion.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1397307 - P3. Remove unused method.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1397307 - P4. Fix style.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1397307 - P5. Avoid creating two decoders on first sample.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1397307 - P6. Calculate average video frame rate as video is playing.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1397307 - P7. Display video resolution and frame rate in debug data.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details
Bug 1397307 - P8. Pass averaged video frame rate to constructor.
59 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1397307 - P9. Pass video frame rate to RemoteVideoDecoder and GPU process.
59 bytes, text/x-review-board-request
mattwoodrow
: review+
Details
Bug 1397307 - P10. Remove uncessary loop.
59 bytes, text/x-review-board-request
mozbugz
: review+
Details 
In bug 1347101, we stopped relying on a particular event to determine if we could use DXVA. Instead this check is done during initialization.

The test to check if we can use DXVA is done in WMFVideoMFTManager::CanUseDXVA() and it uses the duration of the last input frame to determine if we can play it.

However, as the call to CanUseDXVA was moved to initialisation, we no longer know about the last input's duration, as such CanUseDXVA returns an incorrect value.

The change to bug 1347101 was incorrect.
Assignee: nobody → jyavenard
Actually, what truly broke the use is bug 1323703.

Where the sample duration was used to estimate the video frame rate.
following 1323703, the media duration is used.

(1 / duration) has never provided an estimated frame duration (unless we also knew the total number of frames the video is made of)
https://dxr.mozilla.org/mozilla-central/rev/37b95547f0d27565452136d16b2df2857be840f6/dom/media/platforms/wmf/DXVA2Manager.cpp#522
Blocks: 1323703
Seeing that the estimation of the refresh rate has been broken seen at least 53, I wonder if we should continue to block the use of the HW decoder depending on an estimated (typically wrongly) refresh rate.
Flags: needinfo?(matt.woodrow)
When I tested on these machines, 60fps video with DXVA was *really* bad.

I'm not sure why we haven't had more complaints, maybe youtube just drops the quality level automatically based on frame drop stats?

At the time I determined that there was real value in avoiding DXVA for 60fps videos on these machines, and without evidence to the contrary I think we should do our best to continue to do so.
Flags: needinfo?(matt.woodrow)
the incorrect code wasn't called.
No longer blocks: 1323703
Comment on attachment 8907234 [details]
Bug 1397307 - P8. Pass averaged video frame rate to constructor.

https://reviewboard.mozilla.org/r/178902/#review184024
Attachment #8907234 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8907235 [details]
Bug 1397307 - P9. Pass video frame rate to RemoteVideoDecoder and GPU process.

https://reviewboard.mozilla.org/r/178904/#review184058
Attachment #8907235 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8907228 [details]
Bug 1397307 - P2. Wrap boolean in structure to prevent unwanted conversion.

https://reviewboard.mozilla.org/r/178890/#review184066

::: dom/media/platforms/PlatformDecoderModule.h:58
(Diff revision 1)
>    using OptionSet = EnumSet<Option>;
>  
> +  struct UseNullDecoder
> +  {
> +    UseNullDecoder() = default;
> +    explicit UseNullDecoder(bool aUseNullDecoder) { mUse = aUseNullDecoder; }

Please use an initializer list to set mUse.
Attachment #8907228 - Flags: review?(gsquelart) → review+
Comment on attachment 8907231 [details]
Bug 1397307 - P5. Avoid creating two decoders on first sample.

https://reviewboard.mozilla.org/r/178896/#review184072

::: commit-message-d3aad:3
(Diff revision 1)
> +Bug 1397307 - P5. Avoid creating two decoders on first sample. r?gerald
> +
> +Don't unecessarily, create a decoder, flush, shutdown and create a new one on the first sample.

'unecessarily' -> 'unnecessarily'
Attachment #8907231 - Flags: review?(gsquelart) → review+
Comment on attachment 8907232 [details]
Bug 1397307 - P6. Calculate average video frame rate as video is playing.

https://reviewboard.mozilla.org/r/178898/#review184086

::: dom/media/MediaFormatReader.h:588
(Diff revision 1)
> +      float Update(const media::TimeUnit& aValue)
> +      {
> +        if (aValue != media::TimeUnit::Zero()) {
> +          mMean += (1.0f / aValue.ToSeconds() - mMean) / ++mCount;
> +        }
> +        return mMean;

That return value is not currently used. Unless you think it is likely to be used one day, you could just make this function void.

::: dom/media/platforms/PlatformDecoderModule.h:65
(Diff revision 1)
>    };
>  
> +  struct VideoFrameRate
> +  {
> +    VideoFrameRate() = default;
> +    explicit VideoFrameRate(float aFramerate) { mValue = aFramerate; }

Initializer list please ;-)
Attachment #8907232 - Flags: review?(gsquelart) → review+
Comment on attachment 8907233 [details]
Bug 1397307 - P7. Display video resolution and frame rate in debug data.

https://reviewboard.mozilla.org/r/178900/#review184090
Attachment #8907233 - Flags: review?(gsquelart) → review+
[Tracking Requested - why for this release]: A lot of users are not getting hardware acceleration when they should.
tracking-firefox57: --- → ?
Priority: -- → P2
Comment on attachment 8908090 [details]
Bug 1397307 - P10. Remove uncessary loop.

https://reviewboard.mozilla.org/r/179780/#review185000
Attachment #8908090 - Flags: review?(gsquelart) → review+
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c60b5da550ea
P1. Make method const. r=gerald
https://hg.mozilla.org/integration/autoland/rev/70899b90b6fe
P2. Wrap boolean in structure to prevent unwanted conversion. r=gerald
https://hg.mozilla.org/integration/autoland/rev/9d597b2bcf35
P3. Remove unused method. r=gerald
https://hg.mozilla.org/integration/autoland/rev/3fa8d29b3026
P4. Fix style. r=gerald
https://hg.mozilla.org/integration/autoland/rev/768bf2920dab
P5. Avoid creating two decoders on first sample. r=gerald
https://hg.mozilla.org/integration/autoland/rev/3fbbe2c4ec7b
P6. Calculate average video frame rate as video is playing. r=gerald
https://hg.mozilla.org/integration/autoland/rev/1df89fb6ff5d
P7. Display video resolution and frame rate in debug data. r=gerald
https://hg.mozilla.org/integration/autoland/rev/0cf29b887325
P8. Pass averaged video frame rate to constructor. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/7558fb4b1699
P9. Pass video frame rate to RemoteVideoDecoder and GPU process. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/ba8d3ecc2412
P10. Remove uncessary loop. r=gerald
Depends on: 1400778
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: