Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VAAPI decoding HDR content with artifacts #13533

Closed
VlaDexa opened this issue Feb 23, 2024 · 13 comments
Closed

VAAPI decoding HDR content with artifacts #13533

VlaDexa opened this issue Feb 23, 2024 · 13 comments

Comments

@VlaDexa
Copy link

VlaDexa commented Feb 23, 2024

Important Information

Provide following Information:

  • mpv version
    v0.37.0
  • Linux Distribution and Version
    Arch Linux
  • Source of the mpv binary
    both Arch Linux repository and ALHP
  • If known which version of mpv introduced the problem
  • Window Manager and version
    Plasma 6 Release Candidate 2
  • GPU model, driver and version
    AMD 7800XT, mesa 24.0.1
  • Possible screenshot or video of visual glitches
    mpv-shot0003

If you're not using git master or the latest release, update.
Releases are listed here: https://github.com/mpv-player/mpv/releases

Reproduction steps

mpv --no-config --hwdec=vaapi "https://youtu.be/ar_lpz_VQ58?si=MHAmFK-gM2_QBSRf"

The first frame will be okay, but then there will be artifacts

Expected behavior

The same picture as the software decoding

Actual behavior

See screenshot

Log file

hardware_artifact.txt

Sample files

sample.mp4
@llyyr
Copy link
Contributor

llyyr commented Feb 23, 2024

No artifacts for me on vulkan but it gave me non-HDR h264 video for some reason by default

I can reproduce it on vaapi even on players that aren't mpv. So not our bug most likely

@VlaDexa
Copy link
Author

VlaDexa commented Feb 23, 2024

Must be that github recompressed the sample video, sorry about that

@llyyr
Copy link
Contributor

llyyr commented Feb 23, 2024

Must be that github recompressed the sample video, sorry about that

no you're good, running mpv [youtube link] gave me h264 and I failed to notice.

Chaotic-Temeraire pushed a commit to chaotic-cx/mesa-mirror that referenced this issue Feb 27, 2024
…nabled

Workaround for ffmpeg setting segmentation_update_map to 1 with
segmentation_enabled == 0.

Fixes decoding sample from mpv-player/mpv#13533

Cc: mesa-stable

Reviewed-by: Leo Liu <leo.liu@amd.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27816>
Chaotic-Temeraire pushed a commit to chaotic-cx/mesa-mirror that referenced this issue Feb 27, 2024
…nabled

Workaround for ffmpeg setting segmentation_update_map to 1 with
segmentation_enabled == 0.

Fixes decoding sample from mpv-player/mpv#13533

Cc: mesa-stable

Reviewed-by: Leo Liu <leo.liu@amd.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27816>
(cherry picked from commit 82ff920)
@nowrep
Copy link

nowrep commented Feb 28, 2024

Fixed in mesa 24.0.2 https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27816

@kasper93
Copy link
Contributor

Workaround for ffmpeg setting segmentation_update_map to 1 with segmentation_enabled == 0.

Shouldn't it be fixed in ffmpeg in this case? The issue also happens on d3d11va

@Dudemanguy
Copy link
Member

Sorry now that I actually read the mesa MR I suppose the problem isn't really solved. Seems like an ffmpeg issue?

@llyyr
Copy link
Contributor

llyyr commented Feb 29, 2024

I asked about it in ffmpeg-devel and apparently the vaapi fix isn't a workaround at all and just how all drivers should treat values behind switches.

diff --git a/libavcodec/vp9.c b/libavcodec/vp9.c
index 855936cdc1c7..4a628625131e 100644
--- a/libavcodec/vp9.c
+++ b/libavcodec/vp9.c
@@ -717,6 +717,8 @@ static int decode_frame_header(AVCodecContext *avctx,
                 s->s.h.segmentation.feat[i].skip_enabled = get_bits1(&s->gb);
             }
         }
+    } else {
+        s->s.h.segmentation.update_map = 0;
     }
 
     // set qmul[] based on Y/UV, AC/DC and segmentation Q idx deltas

this fixes the issue from ffmpeg side because the bitstream header values aren't initialized at 0, but remain at the same value as they were for the previous frame.

@kasper93
Copy link
Contributor

kasper93 commented Feb 29, 2024

and just how all drivers should treat values behind switches.

Citation needed. If everyone is not handling it like ffmpeg expects, that means ffmpeg is wrong, not everyone. And it has to be workaround on ffmpeg side.

@nowrep
Copy link

nowrep commented Feb 29, 2024

In my opinion if the field isn't in bitstream it shouldn't be passed to driver as bogus value, but technically it's not wrong because from spec it shouldn't be used when segmentation is not enabled and also intel driver can handle that, so there's that. The issue isn't present in gstreamer fwiw.

It's fixed now in mesa common code, which also fixes it for the d3d vaapi driver (not sure if that one was affected).

@llyyr
Copy link
Contributor

llyyr commented Feb 29, 2024

ffmpeg does it for vp8, so it probably should do it for vp9 as well. I sent a patch https://ffmpeg.org//pipermail/ffmpeg-devel/2024-February/322379.html

@nowrep
Copy link

nowrep commented Feb 29, 2024

That should probably also zero out segmentation.temporal.

@VlaDexa
Copy link
Author

VlaDexa commented Apr 22, 2024

Hey, the fix landed for me, and now the video is being correctly displayed. Since your conversation seems unfinished, do you consider this still being an issue? If not, I'll close it.

@llyyr
Copy link
Contributor

llyyr commented Apr 22, 2024

We don't track ffmpeg issues here so seems fine to close

@VlaDexa VlaDexa closed this as completed Apr 22, 2024
BtbN pushed a commit to FFmpeg/FFmpeg that referenced this issue May 27, 2024
Fields under the segmentation switch are never reset on a new frame, and
retain the value from the previous frame. This bugs out a bunch of
hwaccel drivers when segmentation is disabled but update_map isn't
reset because they don't ignore values behind switches. This commit also
resets the temporal field, though it may not be required.

We also do this for vp8 [1] so this commit is just mirroring the vp8
logic.

This fixes an issue with certain samples [2] that causes blocky
artifacts with vaapi, d3d11va and cuda (and possibly others).
Mesa worked around [3] this by ignoring these fields if
segmentation.enabled is 0, but d3d11va still displays blocky artifacts.

[1] https://git.ffmpeg.org/gitweb/ffmpeg.git/blob/2e877090f958131accb8c7e5ac10e5b9865d1735:/libavcodec/vp8.c#l797
[2] mpv-player/mpv#13533
[3] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27816

Signed-off-by: llyyr <llyyr.public@gmail.com>
BtbN pushed a commit to FFmpeg/FFmpeg that referenced this issue May 27, 2024
Fields under the segmentation switch are never reset on a new frame, and
retain the value from the previous frame. This bugs out a bunch of
hwaccel drivers when segmentation is disabled but update_map isn't
reset because they don't ignore values behind switches. This commit also
resets the temporal field, though it may not be required.

We also do this for vp8 [1] so this commit is just mirroring the vp8
logic.

This fixes an issue with certain samples [2] that causes blocky
artifacts with vaapi, d3d11va and cuda (and possibly others).
Mesa worked around [3] this by ignoring these fields if
segmentation.enabled is 0, but d3d11va still displays blocky artifacts.

[1] https://git.ffmpeg.org/gitweb/ffmpeg.git/blob/2e877090f958131accb8c7e5ac10e5b9865d1735:/libavcodec/vp8.c#l797
[2] mpv-player/mpv#13533
[3] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27816

Signed-off-by: llyyr <llyyr.public@gmail.com>
BtbN pushed a commit to FFmpeg/FFmpeg that referenced this issue May 27, 2024
Fields under the segmentation switch are never reset on a new frame, and
retain the value from the previous frame. This bugs out a bunch of
hwaccel drivers when segmentation is disabled but update_map isn't
reset because they don't ignore values behind switches. This commit also
resets the temporal field, though it may not be required.

We also do this for vp8 [1] so this commit is just mirroring the vp8
logic.

This fixes an issue with certain samples [2] that causes blocky
artifacts with vaapi, d3d11va and cuda (and possibly others).
Mesa worked around [3] this by ignoring these fields if
segmentation.enabled is 0, but d3d11va still displays blocky artifacts.

[1] https://git.ffmpeg.org/gitweb/ffmpeg.git/blob/2e877090f958131accb8c7e5ac10e5b9865d1735:/libavcodec/vp8.c#l797
[2] mpv-player/mpv#13533
[3] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27816

Signed-off-by: llyyr <llyyr.public@gmail.com>
BtbN pushed a commit to FFmpeg/FFmpeg that referenced this issue May 27, 2024
Fields under the segmentation switch are never reset on a new frame, and
retain the value from the previous frame. This bugs out a bunch of
hwaccel drivers when segmentation is disabled but update_map isn't
reset because they don't ignore values behind switches. This commit also
resets the temporal field, though it may not be required.

We also do this for vp8 [1] so this commit is just mirroring the vp8
logic.

This fixes an issue with certain samples [2] that causes blocky
artifacts with vaapi, d3d11va and cuda (and possibly others).
Mesa worked around [3] this by ignoring these fields if
segmentation.enabled is 0, but d3d11va still displays blocky artifacts.

[1] https://git.ffmpeg.org/gitweb/ffmpeg.git/blob/2e877090f958131accb8c7e5ac10e5b9865d1735:/libavcodec/vp8.c#l797
[2] mpv-player/mpv#13533
[3] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27816

Signed-off-by: llyyr <llyyr.public@gmail.com>
BtbN pushed a commit to FFmpeg/FFmpeg that referenced this issue May 27, 2024
Fields under the segmentation switch are never reset on a new frame, and
retain the value from the previous frame. This bugs out a bunch of
hwaccel drivers when segmentation is disabled but update_map isn't
reset because they don't ignore values behind switches. This commit also
resets the temporal field, though it may not be required.

We also do this for vp8 [1] so this commit is just mirroring the vp8
logic.

This fixes an issue with certain samples [2] that causes blocky
artifacts with vaapi, d3d11va and cuda (and possibly others).
Mesa worked around [3] this by ignoring these fields if
segmentation.enabled is 0, but d3d11va still displays blocky artifacts.

[1] https://git.ffmpeg.org/gitweb/ffmpeg.git/blob/2e877090f958131accb8c7e5ac10e5b9865d1735:/libavcodec/vp8.c#l797
[2] mpv-player/mpv#13533
[3] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27816

Signed-off-by: llyyr <llyyr.public@gmail.com>
BtbN pushed a commit to FFmpeg/FFmpeg that referenced this issue May 27, 2024
Fields under the segmentation switch are never reset on a new frame, and
retain the value from the previous frame. This bugs out a bunch of
hwaccel drivers when segmentation is disabled but update_map isn't
reset because they don't ignore values behind switches. This commit also
resets the temporal field, though it may not be required.

We also do this for vp8 [1] so this commit is just mirroring the vp8
logic.

This fixes an issue with certain samples [2] that causes blocky
artifacts with vaapi, d3d11va and cuda (and possibly others).
Mesa worked around [3] this by ignoring these fields if
segmentation.enabled is 0, but d3d11va still displays blocky artifacts.

[1] https://git.ffmpeg.org/gitweb/ffmpeg.git/blob/2e877090f958131accb8c7e5ac10e5b9865d1735:/libavcodec/vp8.c#l797
[2] mpv-player/mpv#13533
[3] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27816

Signed-off-by: llyyr <llyyr.public@gmail.com>
BtbN pushed a commit to FFmpeg/FFmpeg that referenced this issue May 27, 2024
Fields under the segmentation switch are never reset on a new frame, and
retain the value from the previous frame. This bugs out a bunch of
hwaccel drivers when segmentation is disabled but update_map isn't
reset because they don't ignore values behind switches. This commit also
resets the temporal field, though it may not be required.

We also do this for vp8 [1] so this commit is just mirroring the vp8
logic.

This fixes an issue with certain samples [2] that causes blocky
artifacts with vaapi, d3d11va and cuda (and possibly others).
Mesa worked around [3] this by ignoring these fields if
segmentation.enabled is 0, but d3d11va still displays blocky artifacts.

[1] https://git.ffmpeg.org/gitweb/ffmpeg.git/blob/2e877090f958131accb8c7e5ac10e5b9865d1735:/libavcodec/vp8.c#l797
[2] mpv-player/mpv#13533
[3] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27816

Signed-off-by: llyyr <llyyr.public@gmail.com>
richardpl pushed a commit to librempeg/librempeg that referenced this issue May 28, 2024
Fields under the segmentation switch are never reset on a new frame, and
retain the value from the previous frame. This bugs out a bunch of
hwaccel drivers when segmentation is disabled but update_map isn't
reset because they don't ignore values behind switches. This commit also
resets the temporal field, though it may not be required.

We also do this for vp8 [1] so this commit is just mirroring the vp8
logic.

This fixes an issue with certain samples [2] that causes blocky
artifacts with vaapi, d3d11va and cuda (and possibly others).
Mesa worked around [3] this by ignoring these fields if
segmentation.enabled is 0, but d3d11va still displays blocky artifacts.

[1] https://git.ffmpeg.org/gitweb/ffmpeg.git/blob/2e877090f958131accb8c7e5ac10e5b9865d1735:/libavcodec/vp8.c#l797
[2] mpv-player/mpv#13533
[3] https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27816

Signed-off-by: llyyr <llyyr.public@gmail.com>
Signed-off-by: Paul B Mahol <onemda@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants