| Bug ID | 97988 |
|---|---|
| Summary | [radeonsi] playing back videos with VDPAU exhibits deinterlac= ing/anti-aliasing issues not visible with VA-API |
| Product | Mesa |
| Version | git |
| Hardware | x86-64 (AMD64) |
| OS | Linux (All) |
| Status | NEW |
| Severity | normal |
| Priority | medium |
| Component | Drivers/Gallium/radeonsi |
| Assignee | dri-devel@lists.freedesktop.org |
| Reporter | kai@dev.carbon-project.org |
| QA Contact | dri-devel@lists.freedesktop.org |
Created att=
achment 126902 [details]
mpv --hwdec=3Dvdpau --vo=3Dopengl-hq
I noticed, that when I play videos with mpv using VDPAU I'm getting an image
that looks like deinterlacing/anti-aliasing is not working, making fine
structures/patterns or text look ugly/unreadable. Playing the same video wi=
th
the same stack but using VA-API doesn't exhibit this issue.
Attached you'll find a screenshot taken with VDPAU when playing a video usi=
ng
mpv --hwdec=3Dvdpau --vo=3Dopengl-hq
The only difference for the VA-API playback is using "--hwdec=3Dvaapi&=
quot;. I would
say that this is a regression, but I can't remember when it started. :-(
The stack showing this issue is (Debian testing as a base):
GPU: Hawaii PRO [Radeon R9 290] (ChipID =3D 0x67b1)
Mesa: Git:master/e4b585f009
libdrm: 2.4.70-1
LLVM: SVN:trunk/r282761 (4.0 devel)
X.Org: 2:1.18.4-1
Linux: 4.7.5
Firmware: firmware-amd-graphics/20160824-1
libclc: Git:master/88b82a6f70
DDX (amdgpu): 1.1.2-1
Let me know, if you need anything else.
Created attachment 126903 [details]
mpv --hwdec=3Dvaapi --vo=3Dopengl-hq (good rendering for reference)
Does that happen as well with "mpv -hwdec vdpau -vo vdpau= " ? Could as well be a problem in mpv.
(In reply to Christian K=C3=B6nig from comment #2) > Does that happen as well with "mpv -hwdec v= dpau -vo vdpau" ? No, it doesn't! > Could as well be a problem in mpv. So this is a MPV issue then?
Yeah, this is an issue with the GL interop. I can reproduce it= here, so maybe I should take a look at it.
Could as well be a problem within Mesa. Grigori if you could take a look that would be great. Thanks in advance, Christian.
That is strange. This sounds very much like a problem I and an= other user encountered with an older version of Mesa: https://github.co= m/mpv-player/mpv/issues/3317 https://bugs.freedesktop.org/show_bug.c= gi?id=3D96860 Upgrading to Mesa 12.0.0 fixed it, however. I cannot really provide further clues as I am currently not seeing the issue.
Still not quite right with vaapi or before this, but that's a = different hard to see issue without a res test of the correct height. This one seems to start with - commit 16be87c904293c2e53d50cc3519789a604a6a33b Author: Marek Ol=C5=A1=C3=A1k <marek.olsak@amd.com> Date: Tue Sep 13 14:25:44 2016 +0200 radeonsi: get rid of img/buf/sampler descriptor preloading (v2) 26011 shaders in 14651 tests Totals: SGPRS: 1251920 -> 1152636 (-7.93 %) VGPRS: 728421 -> 728198 (-0.03 %) Spilled SGPRs: 16644 -> 3776 (-77.31 %) Spilled VGPRs: 369 -> 369 (0.00 %) Scratch VGPRs: 1344 -> 1344 (0.00 %) dwords per thread Code Size: 36001064 -> 35835152 (-0.46 %) bytes LDS: 767 -> 767 (0.00 %) blocks Max Waves: 222221 -> 222372 (0.07 %) Wait states: 0 -> 0 (0.00 %) v2: merge codepaths where possible
Here's a bit of background: Earlier, mpv used interop with OutputSurfaces. That is, it rendered the vid= eo into an RGB surface and then used that for its own procesing (scaling, rendering OSD on top, etc.). Now recently, mpv has switched to interop with VideoSurfaces. That is, it m= aps four fields of luma/chroma planes. These are split up into top and bottom field, just as the spec for the interop extension says: http= s://www.opengl.org/registry/specs/NV/vdpau_interop.txt mpv then weaves the fields and does color space conversion, scaling, etc. Things get wrong with the weaving, and I found some very suspicious code in Mesa: https://cgit.freedesktop.org/mesa/mesa/tree/src/mesa/sta= te_tracker/st_vdpau.c#n82 Here a sampler is returned based on texture name index, and the same plane sampler is used for top and bottom field. That can't be right at all. Is the sampler possibly mangled somewhere else to make it point to the right field= s? It does not look like it, but maybe I'm just missing something. This really makes me wonder whether VideoSurface interop ever worked correc= tly.
Sorry, it's something else. The field is set up later by point= ing to the right layer. So there's something else going wrong. Andy, I can confirm that reverting to that commit fixes it. I'll keep looking.
Created attachment 126946=
[details]
Weave shader dumps
It looks like LLVM miscompiles the weave fragment shader due to the changes=
to
descriptor loading. See the attached file. With these changes, LLVM generat=
es
some really odd looking and inefficient code in this case, and the two samp=
le
instructions are somehow folded together, which cannot possibly work.
Someone proficient in LLVM can probably figure out what is going wrong.
Thanks Grigori.
GLSL:
color =3D fract(gl_FragCoord.y / 2) < 0.5 ?
texture(texture0, texcoord0) :
texture(texture1, texcoord0);
texture0 and texture1 are SMEM loads.
LLVM (SimplifyCFG) transforms it to:
color =3D texture(fract(gl_FragCoord.y / 2) < 0.5 ? texture0 : texture=
1,
texcoord0);
That's a nice transformation. You don't have to use 2 SMEM loads, you can j=
ust
use one SMEM load depending on the result of the condition.
The problem is gl_FragCoord.y is a VGPR and texture0/1 are SGPRs, therefore
flat VMEM loads are used to load the descriptors. However, image_sample
requires descriptors in SGPRs, so v_readfirstlane is used. That effectively
uses the result of the condition from the first active lane, discarding the
results from all other lanes. The result would be exactly the same if the
compiler did: v_readfirstlane s0, gl_FragCoord.y;
The test case seems pretty trivial I wonder how many other apps are affecte=
d.
(In reply to Marek Ol=C5=A1=C3=A1k from comment #11) > Thanks Grigori. >=20 > GLSL: > color =3D fract(gl_FragCoord.y / 2) < 0.5 ? > texture(texture0, texcoord0) : > texture(texture1, texcoord0); >=20 > texture0 and texture1 are SMEM loads. >=20 > LLVM (SimplifyCFG) transforms it to: > color =3D texture(fract(gl_FragCoord.y / 2) < 0.5 ? texture0 : te= xture1, > texcoord0); >=20 > That's a nice transformation. You don't have to use 2 SMEM loads, you = can > just use one SMEM load depending on the result of the condition. >=20 > The problem is gl_FragCoord.y is a VGPR and texture0/1 are SGPRs, ther= efore > flat VMEM loads are used to load the descriptors. However, image_sample > requires descriptors in SGPRs, so v_readfirstlane is used. That effect= ively > uses the result of the condition from the first active lane, discardin= g the > results from all other lanes. The result would be exactly the same if = the > compiler did: v_readfirstlane s0, gl_FragCoord.y; >=20 > The test case seems pretty trivial I wonder how many other apps are af= fected. I think the best solution here would be to teach the backend how to do a sc= alar select based on value of vccz.
(In reply to Tom Stellard from comment #12) >=20 > I think the best solution here would be to teach the backend how to do= a > scalar select based on value of vccz. You are missing the point. The condition code (VCC) isn't the same across a= ll threads. The problem is that the conditional assignment is transformed into= a form that makes descriptor load addresses non-uniform (dependent on a VGPR). There is nothing the AMDGPU backend can do about it. It's not a problem in = the backend.
(In reply to Marek Ol=C5=A1=C3=A1k from comment #13) > (In reply to Tom Stellard from comment #12) > >=20 > > I think the best solution here would be to teach the backend how = to do a > > scalar select based on value of vccz. >=20 > You are missing the point. The condition code (VCC) isn't the same acr= oss > all threads. The problem is that the conditional assignment is transfo= rmed > into a form that makes descriptor load addresses non-uniform (dependen= t on a > VGPR). There is nothing the AMDGPU backend can do about it. It's not a > problem in the backend. Well the backend could iterate over all the variants to handle that, but I = have strong doubts that this would result in optimal performance. So I think the best solution for now would be to mark the texture intrinsic= in a way which disallows such optimizations.
Created attachment 126996 [d= etails] [review] piglit test The attached test reproduces the issue. CFGSimplificationPass is the culpri= t. Note that CFGSimplificationPass also doesn't preserve the !amdgpu.uniform metadata when transforming the loads, which is a change in behavior.
Looks like we'd need the opposite of the convergent attribute.= And that attribute should only apply to one argument of the texture sampling intrins= ic (i.e. not to the texture coordinate). I don't know if something like that already exists in LLVM.
(In reply to Tom Stellard from comment #12) > (In reply to Marek Ol=C5=A1=C3=A1k from comment #11) > > The test case seems pretty trivial I wonder= how many other apps are affected. I don't know the answer to that, but I've just noticed that a debug build of mesa will catch the issue with mpv - mpv: state_tracker/st_sampler_view.c:481: st_get_texture_sampler_view_from_stobj: Assertion `stObj->base.MinLayer = =3D=3D view->u.tex.first_layer' failed.
(In reply to Andy Furniss from comment #17) > mpv: state_tracker/st_sampler_view.c:481: > st_get_texture_sampler_view_from_stobj: Assertion `stObj->base.MinL= ayer =3D=3D > view->u.tex.first_layer' failed. That's unrelated.
(In reply to Marek Ol=C5=A1=C3=A1k from comment #18) > (In reply to Andy Furniss from comment #17) > > mpv: state_tracker/st_sampler_view.c:481: > > st_get_texture_sampler_view_from_stobj: Assertion `stObj->base= .MinLayer =3D=3D > > view->u.tex.first_layer' failed. >=20 > That's unrelated. Oops, yea, should have checked better - just it needed the same version/com= mand line with mpv as this bug to trigger, but it does start later =3D commit e5cc84dd43be066c1dd418e32f5ad258e31a150a Author: Brian Paul <brianp@= vmware.com> Date: Fri Sep 30 16:41:46 2016 -0600 st/mesa: optimize pipe_sampler_view validation
Created attachment 127812 <= a href=3D"attachment.cgi?id=3D127812&action=3Dedit" title=3D"Mesa part = of fix">[details] [review] Mesa part of fix LLVM patch https://reviews.llvm= .org/D26348 together with the attached patch for Mesa fix this bug.
(In reply to Nicolai H=C3=A4hnle from comment #20) > Created attachmen= t 127812 [details] [review] [review] > Mesa part of fix >=20 > LLVM patch https://reviews= .llvm.org/D26348 together with the attached patch > for Mesa fix this bug. Nice, will test later, but is there a actually a nice way to get a patch fr= om Phabricator that will apply from top level with git apply? I usually fail with "download raw diff" and have to search/sort o= ut the paths by hand.