Closed Bug 1645671 Opened 4 years ago Closed 4 years ago

[Wayland] dmabuf-video-textures: Video is green the moment before video decoding starts

Categories

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

79 Branch
x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
81 Branch
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 --- unaffected
firefox77 --- unaffected
firefox78 --- unaffected
firefox79 --- disabled
firefox80 - fixed
firefox81 --- fixed

People

(Reporter: jan, Assigned: stransky)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: correctness, nightly-community, regression)

Attachments

(2 files)

This is a minor cosmetic issue. It affects software and VAAPI.
It occurs only once. To reproduce again, reload the tab or change video resolution on YouTube.
(With media.ffvpx.enabled:false this can be observed with VP9 videos.)

software: MOZ_LOG="PlatformDecoderModule:5" MOZ_ENABLE_WAYLAND=1 mozregression --launch 0c7df6f9b0c1 --pref gfx.webrender.all:true widget.wayland-dmabuf-video-textures.enabled:true media.ffvpx.enabled:false -a https://bug1619882.bmoattachments.org/attachment.cgi?id=9149605 -P stdout

hardware: MOZ_LOG="PlatformDecoderModule:5" LIBVA_DRIVER_NAME=i965 MOZ_ENABLE_WAYLAND=1 mozregression --launch 0c7df6f9b0c1 --pref gfx.webrender.all:true widget.wayland-dmabuf-vaapi.enabled:true widget.wayland-dmabuf-video-textures.enabled:true media.ffvpx.enabled:false -a https://bug1619882.bmoattachments.org/attachment.cgi?id=9149605 -P stdout

I can confirm this bug, started happening a few days ago.

I meet the similar issue with huya.com which is a live streaming website.

So this is a regression from Bug 1629788, correct?

Correct.
https://hg.mozilla.org/integration/autoland/shortlog/0c7df6f9b0c1
first affected build: MOZ_LOG="PlatformDecoderModule:5" MOZ_ENABLE_WAYLAND=1 mozregression --repo autoland --launch 0c7df6f9b0c1 --pref gfx.webrender.all:true widget.wayland-dmabuf-video-textures.enabled:true media.ffvpx.enabled:false -a https://bug1619882.bmoattachments.org/attachment.cgi?id=9149605 -P stdout
last unaffected build: MOZ_LOG="PlatformDecoderModule:5" MOZ_ENABLE_WAYLAND=1 mozregression --repo autoland --launch 6f6757bfd3298d8314d52730c3c016dc4c629f05 --pref gfx.webrender.all:true widget.wayland-dmabuf-video-textures.enabled:true media.ffvpx.enabled:false -a https://bug1619882.bmoattachments.org/attachment.cgi?id=9149605 -P stdout

Same for VAAPI:
first affected build: MOZ_LOG="PlatformDecoderModule:5" LIBVA_DRIVER_NAME=i965 MOZ_ENABLE_WAYLAND=1 mozregression --launch 0c7df6f9b0c1 --pref gfx.webrender.all:true widget.wayland-dmabuf-vaapi.enabled:true widget.wayland-dmabuf-video-textures.enabled:true media.ffvpx.enabled:false -a https://bug1619882.bmoattachments.org/attachment.cgi?id=9149605 -P stdout
last unaffected build: MOZ_LOG="PlatformDecoderModule:5" LIBVA_DRIVER_NAME=i965 MOZ_ENABLE_WAYLAND=1 mozregression --repo autoland --launch 6f6757bfd3298d8314d52730c3c016dc4c629f05 --pref gfx.webrender.all:true widget.wayland-dmabuf-vaapi.enabled:true widget.wayland-dmabuf-video-textures.enabled:true media.ffvpx.enabled:false -a https://bug1619882.bmoattachments.org/attachment.cgi?id=9149605 -P stdout

This started happening again after today's update...

This was never gone and the feature is not finished yet (bug 1629788 comment 27). Some other patches still await review.
VAAPI was just temporarily disabled: bug 1645706 disabled the dmabuf-video-textures pref that VAAPI now depends upon. You had to enable two prefs for VAAPI. bug 1646051 changed it to let the VAAPI pref also enable dmabuf-video-textures. You see this bug again because you are using VAAPI again.

(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #7)

This was never gone and the feature is not finished yet (bug 1629788 comment 27). Some other patches still await review.
VAAPI was just temporarily disabled: bug 1645706 disabled the dmabuf-video-textures pref that VAAPI now depends upon. You had to enable two prefs for VAAPI. bug 1646051 changed it to let the VAAPI pref also enable dmabuf-video-textures. You see this bug again because you are using VAAPI again.

Thanks for clarification! I thought it was gone for a few days.

(In reply to Marko from comment #8)

Thanks for clarification! I thought it was gone for a few days.

I never had this on my PC (AMD 5700XT / mesa 20.1.1 / Linux 5.7.2) and I was using VAAPI decoding for about a month. Just started happening today. Even with yesterdays nightly where I had to enable the dmabuf-video-textures pref, I was seeing no Green frames. I'm just commenting it in case it helps to identify what it is, but since it's work in progress I guess it will be resolved at some point.

For reference, this is also an issue on X11/EGL, even without media.ffmpeg.dmabuf-textures.enabled.

Why is this bug of such low priority, it is a regression that basically isn't present on 78 stable and will regress both 79 and even 80 it seems for all the vaapi users... is there any way that fix will be backported once it's fixed?

I agree with comment 12, now that firefox 79 is officially out this bug is very visible to anyone using hardware accelerated video playback on linux.

(In reply to Marko from comment #12)

Why is this bug of such low priority, it is a regression that basically isn't present on 78 stable and will regress both 79 and even 80 it seems for all the vaapi users... is there any way that fix will be backported once it's fixed?

First of all this feature is not yet enabled by default and requires users to jump through several hoops to enable it (enable EGL either for X11 or by using the Wayland backend, enable WR, enable several options). Secondly it's not a crash but just a glitch, and not a major one neither. Finally the feature was not developed by the FF core team but external contributors. This together results in a low priority. And no, fixes for disabled-by-default features will usually not get backported.

But don't worry, I'm sure Martin will work on it as time allows.

(In reply to Robert Mader [:rmader] from comment #14)

(In reply to Marko from comment #12)

Why is this bug of such low priority, it is a regression that basically isn't present on 78 stable and will regress both 79 and even 80 it seems for all the vaapi users... is there any way that fix will be backported once it's fixed?

First of all this feature is not yet enabled by default and requires users to jump through several hoops to enable it (enable EGL either for X11 or by using the Wayland backend, enable WR, enable several options). Secondly it's not a crash but just a glitch, and not a major one neither. Finally the feature was not developed by the FF core team but external contributors. This together results in a low priority. And no, fixes for disabled-by-default features will usually not get backported.

But don't worry, I'm sure Martin will work on it as time allows.

I know, but it's a regression... shouldn't regressions have higher priority than implementing new features like X11 backend etc... the part that is bad about this is that in 78 it works perfectly without any issues, and in 79+ is annoying to use again just like with frame skipping bug before, and yet there are new features being worked on without finishing old ones first.
Obviously it's up to Martin to decide since he is a contributor and FF devs don't care enough to provide tge feature, but as a linux laptop user, I would really like to see this having a higher priprity :) didn't mean to be rude or anything.

(In reply to Marko from comment #15)

I know, but it's a regression... shouldn't regressions have higher priority than implementing new features like X11 backend etc... the part that is bad about this is that in 78 it works perfectly without any issues, and in 79+ is annoying to use again just like with frame skipping bug before, and yet there are new features being worked on without finishing old ones first.
Obviously it's up to Martin to decide since he is a contributor and FF devs don't care enough to provide tge feature, but as a linux laptop user, I would really like to see this having a higher priprity :) didn't mean to be rude or anything.

The patch is on the way. I prefer to unify X11/Wayland VA-API/dmabuf backends and then do all the fixes for both together, that lowers the maintenance cost. This is really still an experimental feature so please accept potential breakage during the development (and I also have other duties at Red Hat).

(In reply to Martin Stránský [:stransky] from comment #17)

(In reply to Marko from comment #15)

I know, but it's a regression... shouldn't regressions have higher priority than implementing new features like X11 backend etc... the part that is bad about this is that in 78 it works perfectly without any issues, and in 79+ is annoying to use again just like with frame skipping bug before, and yet there are new features being worked on without finishing old ones first.
Obviously it's up to Martin to decide since he is a contributor and FF devs don't care enough to provide tge feature, but as a linux laptop user, I would really like to see this having a higher priprity :) didn't mean to be rude or anything.

The patch is on the way. I prefer to unify X11/Wayland VA-API/dmabuf backends and then do all the fixes for both together, that lowers the maintenance cost. This is really still an experimental feature so please accept potential breakage during the development (and I also have other duties at Red Hat).

Thnaks for the clarification about X11/wayland common backend.

Don't worry Martin, we owe you eternal gratitude for implementing this feature in Firefox after so many years the issue was neglected by other developers!

When DMABufSurfaceWrapper is added to nsTArray, a temporary local DMABufSurfaceWrapper object is created. When the temporary
object is deleted after the adding, associated dmabuf data is released which leads to rendering artifact during VA-API video playback.

As a fix in this patch we use UniquePtr<DMABufSurfaceWrapper> at nsTArray thus we don't create the temporary DMABufSurfaceWrapper object.
Also change GetUnusedDMABufSurfaceWrapper() to GetUnusedDMABufSurfaceWrapperIndex() and reference the surface wrapper by indexe instead of pointer.

Assignee: nobody → stransky
Status: NEW → ASSIGNED
Attachment #9166559 - Attachment description: Bug 1645671 [Linux/VA-API] Use UniquePtr<DMABufSurfaceWrapper> instead of a plain object at dmabuf surface cache, r?jya → Bug 1645671 [Linux/VA-API] Create DMABufSurfaceWrapper directly at nsTAttay and disable DMABufSurfaceWrapper class copy and assignment constructors, r?jya
Pushed by abutkovits@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bacacb384008
[Linux/VA-API] Create DMABufSurfaceWrapper directly at nsTAttay and disable DMABufSurfaceWrapper class copy and assignment constructors, r=jya
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

[Tracking Requested - why for this release]:

This regression affects:

  • MOZ_ENABLE_WAYLAND=1 users with Firefox 79,
  • MOZ_X11_EGL=1 and MOZ_ENABLE_WAYLAND=1 users with Firefox 80.

Everyone is excited that Firefox 80 finally supports VAAPI hardware decoding on X11, it got news coverage, etc., but those who tested it know that it suffers from at least two fatal bugs. (bug 1645672 is the other one.) Wayland/MOZ_X11_EGL and VAAPI are still experimental and disabled-by-default, but it's a killer feature that brings users back. Therefore please consider uplifing this into beta, and even into a dot release, if there is any.

Downstream Fedora applies this patch on Firefox 79: https://src.fedoraproject.org/rpms/firefox/c/cd18e999f576523b6b3f2076bca625d5e04d1178?branch=master

https://www.reddit.com/r/archlinux/comments/i0ireu/firefox_developer_edition_hardware_video/fzpojad/
https://www.reddit.com/r/flatpak/comments/i157dm/green_flashes_in_video_in_firefox/
https://www.reddit.com/r/linux/comments/i04cvr/psa_firefox_79_appears_to_break_vaapi_decoding_on/
https://www.reddit.com/r/firefox/comments/i1p2ct/linux_vaapi_video_decode_acceleration_unreliable/

Not tracking as this is still disabled.

The patch landed in nightly and beta is affected.
:stransky, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(stransky)

Comment on attachment 9166559 [details]
Bug 1645671 [Linux/VA-API] Create DMABufSurfaceWrapper directly at nsTAttay and disable DMABufSurfaceWrapper class copy and assignment constructors, r?jya

Beta/Release Uplift Approval Request

  • User impact if declined: Broken VA-API video playback on Wayland/X11.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Changes a way how elements are stored in nsTArray, to in-place assignment.
  • String changes made/needed:
Flags: needinfo?(stransky)
Attachment #9166559 - Flags: approval-mozilla-beta?

Comment on attachment 9166559 [details]
Bug 1645671 [Linux/VA-API] Create DMABufSurfaceWrapper directly at nsTAttay and disable DMABufSurfaceWrapper class copy and assignment constructors, r?jya

wayland/vaapi fix, approved for 80.0b5

Attachment #9166559 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

It's likely that you see bug 1658035 - should be fixed by next nightly.

Please file a new bug about it and also attach a media log.
Run Firefox on terminal with MOZ_LOG="PlatformDecoderModule:5" to get the media log.
My guess is that we don't import dmabuf surfaces correctly, the decoder may use a different format or so.
Thanks.

Yes, bug 1658035 is about videos being completely green until the next Nightly update.

Well that was fast :D . Fix confirmed in today's Nightly 81.0a1/20200808093545, VAAPI acceleration is effective for me under Xorg/Intel, and videos are no longer completely green. Thanks for the fix, Robert, and thanks for the Xorg push, Martin!

With that, not creating a new bug. If you nevertheless still would like the info you requested in Comment 38, Martin, do ping me and I'll create one.

(In reply to Ronan Jouchet from comment #40)

Well that was fast :D . Fix confirmed in today's Nightly 81.0a1/20200808093545, VAAPI acceleration is effective for me under Xorg/Intel, and videos are no longer completely green. Thanks for the fix, Robert, and thanks for the Xorg push, Martin!

With that, not creating a new bug. If you nevertheless still would like the info you requested in Comment 38, Martin, do ping me and I'll create one.

Can confirm, now VAAPI acceleration is working properly on latest Nightly build.

Thanks for the fix, everyone.

This is happened to me when widget.wayland-dmabuf-vaapi.enabled is set to True

Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.