Bug 71591 - Second Life shaders fail to compile (extension declared in middle of shader)
Summary: Second Life shaders fail to compile (extension declared in middle of shader)
Status: RESOLVED NOTOURBUG
Alias: None
Product: Mesa
Classification: Unclassified
Component: Mesa core (show other bugs)
Version: 9.2
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: mesa-dev
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 75664
  Show dependency treegraph
 
Reported: 2013-11-14 01:49 UTC by MirceaKitsune
Modified: 2015-04-13 08:19 UTC (History)
5 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Log file containing the normal console output and GLSL dump (23.08 KB, text/plain)
2013-11-14 01:49 UTC, MirceaKitsune
Details

Description MirceaKitsune 2013-11-14 01:49:59 UTC
Created attachment 89171 [details]
Log file containing the normal console output and GLSL dump

Shaders fail to compile for the Second Life viewer, under Mesa with the free Radeon driver. The same version of SL worked with fglrx (proprietary ATI driver) but no longer does after switching to the free driver, although Mesa should be capable of running those shaders. I was recommended to post this bug here.

My Linux distribution is openSUSE 13.1, KDE 4.11.2. X11 1.6.2, MESA 9.2.2, Radeon 7.2.0 (xf86-video-ati) supporting OpenGL <= 3.1. My video card is a Radeon HD 6870. The viewer I use is Firestorm LGPL version 4.5.2.

Attached is a text file with the normal console output of the viewer, followed by a MESA_GLSL=dump output of the error.
Comment 1 Kenneth Graunke 2013-11-14 04:22:36 UTC
According to the GLSL specification (1.30, page 14), #extension directives must come before any non-preprocessor tokens.  These shaders are invalid and we correctly fail to compile them.

So, this is a bug in the Second Life viewer.  I would try filing a bug with them.
Comment 2 MirceaKitsune 2013-11-14 12:26:08 UTC
I understand. Still strange that proprietary drivers compile them while Mesa does not. Even if they're written poorly, I imagine Mesa could add compatibility support to avoid situations like this. Since Linden Labs fail to notice bug reports for years, I'm not sure if they'll care for this... but I'll take it to their bug tracker.

I know this is beyond the purpose of a bug report. But until they fix it, can anyone explain where and how I should move that statement to manually repair the shaders please?
Comment 3 Jose Fonseca 2013-11-14 17:14:33 UTC
We saw other apps doing the same (Photoshop).  And we ended up working around it with a small non-invasive change in Mesa.

This seems a common mistake.  I think we should consider applying it in upstream mesa too, for sake of compatability.
Comment 4 Eero Tamminen 2013-11-18 09:19:15 UTC
E.g. Unigine's Heaven benchmark (v4.0) does this also.

It's a shame as that would be a nice test case for recently added geometry shader and GL_ARB_sample_shading support in Mesa.  93 of the shaders in Heaven benchmark (= nearly half of them) have:
  #extension GL_ARB_sample_shading : enable

And in none of them it's at the start.  There are no other extension declarations.


(In reply to comment #3)
> This seems a common mistake.  I think we should consider applying it in
> upstream mesa too, for sake of compatability.

All the Heaven shaders have main().  I think they're concatenated together from different parts and in that case it makes sense to declare extensions required by given part of shader, in the beginning of that part.

Program needing to keep track of what extensions are require by different shader parts is inconvenient, especially if those concatenated parts can be edited separately from the program (they e.g. come from disk).


(In reply to comment #1)
> According to the GLSL specification (1.30, page 14), #extension directives
> must come before any non-preprocessor tokens.

The spec says:
"Each extension can define its allowed granularity of scope. If nothing is said, the granularity is a shader (that is, a single compilation unit), and the extension directives must occur before any non-preprocessor tokens."

What if extension defines its allowed scope?
Comment 5 MirceaKitsune 2013-11-18 11:30:01 UTC
Although I'm not a driver developer and have little technical expertise here, I've gotten a better understanding of the issue.

I can see why Mesa deems such shaders as invalid, given the statement is not placed correctly and it is bad writing. However, I believe the Mesa developers should consider that many applications have shaders written this way, with developers who might not care to fix them. Failing with an error here will condemn those with the free drivers to be unable to run what those with the proprietary drivers can. Yes, Mesa's approach is technically correct, the writing is bad and shouldn't be supported... but users of many programs with bad shaders have to suffer the consequences, which I don't think is right.

My personal suggestion is to not fail with an error, but print a big warning in the console to let people know the shaders are written poorly. I believe this is a fair approach, and a compromise Mesa can make for the sake of compatibility. Please consider it.
Comment 6 Eero Tamminen 2013-11-21 14:47:11 UTC
Giving an error when the extension changes behavior of existing functionality can be useful for developers, as such things may cause grief for them.  For anything else, it's the error itself that causes grief.

Options for fixing this issue are:
- changing all errors about this to warnings
- making the error extension specific: outputting it only for behavior changing extensions
- adding a config option for whether this gives an error or not.  Default config could then disable this error for binaries that are known to use extension declarations in wrong place.

How common this issue is, what other programs have this issue besides Second life client, photoshop, and Unigine Heaven benchmark?  And what extensions those declare in wrong place?
Comment 7 Eero Tamminen 2013-11-22 09:00:17 UTC
Got a comment from Ian & Kenneth on what's the Mesa policy for things that are against GL spec, but accepted by competing driver(s):
---------------------------
1. Follow the spec.

2. If nobody follows the spec, get the spec changed.  Goto 1.

With a rare case of:

Add driconf workaround for broken app, but only if:
- People still care about broken app
- App will never get fixed
----------------------------

According to comment 1), ATI driver doesn't follow the spec.
What about Nvidia driver?
Comment 8 Eero Tamminen 2013-12-05 11:31:37 UTC
Kevin has a patch to allow extensions anywhere in shader:
  http://patchwork.freedesktop.org/patch/16221/

Apparently that's not going in as-is, but you could try it to see whether it allows the programs currently suffering from this to work ok.
Comment 9 Eero Tamminen 2015-04-10 15:01:17 UTC
Mesa has nowadays "allow_glsl_extension_directive_midshader" option which can be used in drirc, and is set there for Unigine (see parent bug).

Could you check whether that option would fix also Second Life viewer and Photoshop?

If yes, drirc could be updated and this marked as fixed.
Comment 10 MirceaKitsune 2015-04-10 16:56:56 UTC
I'm sorry I forgot to update this earlier. I added "export allow_glsl_extension_directive_midshader=true" to my ~/.profile file, and since then Second Life shaders activate without this issue.

As long as this directive will always be there, I am okay with this. Although unless there isn't a serious reason not to, I still suggest enabling it by default, as not everyone might find this fix and other people will have to deal with this issue by default.
Comment 11 Eero Tamminen 2015-04-13 07:16:44 UTC
How Second Life viewer can be identified by Mesa, does it have distinctive binary name?

What about Photoshop?


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.