KhronosGroup / OpenGL-API

OpenGL and OpenGL ES API Issue Tracker
34 stars 5 forks source link

Request for modification of OpenGL Core Specification #91

Open MischaBaars opened 7 months ago

MischaBaars commented 7 months ago

Hello Khronos OpenGL Working Group,

I started out looking for blend modes and blend operators that give equal on-screen results for GtkGLArea widgets and CSS styled widgets. To that end I wrote a C program that pre-computes blender output colors for certain input colors, sorts them and places them into blender groups with equal output colors.

In doing so, I took the glBlendFunc() and glBlendEquation() functions for two separate blending stages, and implemented the glBlendEquation() part, with the source and destination blend factors as arguments to the min and max functions, by accident. That resulted in a glitch on the screen, when sliding over the different blend modes, so I started looking for the problem.

After inspection of the MesaGL source codes and all the different AMD Radeon Register Reference Guides, found out that arguments to glBlendFunc() and glBlendFuncSeparate() are being replaced by other arguments, when the blend operator is set to GL_MIN or GL_MAX. So I removed that couple of lines of code, and gone was the glitch.

After submitting a merge request, the owners / maintainers made me aware of the 'Notes' section of the glBlendEquation reference page. Then it became clear to me, that OpenGL had implemented blending in such a way, that though the underlying hardware supports them, the vast majority of all possible blender input combinations does not even reach the graphics adapter. NVidia (NV_blend_minmax_factor) and AMD (AMD_blend_minmax_factor), seem to be aware of this, but their proposals to extend functionality have never been implemented. From the NVidia extension it can be seen, that they agree with AMD on at least the naming convention of the additional tokens:

FACTOR_MIN_AMD                              0x901C
FACTOR_MAX_AMD                              0x901D 

This means that the chance that there even ever were adapters around that did do the last stage of blending without using the source and destination blend factors for blend operators GL_MIN and GL_MAX is virtually zero, and that the OpenGL interpretation of how this stage should be implemented has most probably been flawed from the start.

I would like to propose that the OpenGL Core Specification is modified (OpenGL 6.1 (Core) Specification, Table 17.1, p. 509), such that the source and destination blend factors are by default arguments to the second stage of blending, i.e. such that glBlendEquation() purely specifies the operator that is to be applied to the first stage of blending.

Kind regards, Ir. Michael J. Baars, Long 4 Core, the Netherlands.

NogginBops commented 7 months ago

Is there a reason the core specification needs to change? The two extensions are compatible (except the for precision limitation documented in the NV extension). Couldn't mesa just implement AMD_blend_minmax_factor (and possibly NV_blend_minmax_factor depending on hardware) and have this work as you want it to?

MischaBaars commented 7 months ago

Hi,

Yes, there is reason and a need:

1) The developers are a much too lazy to implement the extensions. The AMD extensions dates back to 2011. The two additional tokens have been inserted, but the rest of the implementation is simply not there. 2) The proposed merge request involves removing only a couple of lines, and is all ready to be applied. 3) By applying the patch you get 168% brand new additional blenders for the mentioned input set.

NogginBops commented 7 months ago

From what it sounds like to me you could modify your code to implement the proper extension, I get that it's not as simple as the solution you have currently but that way you are much more likely to get the features you actually want. The OpenGL 4.6 spec is likely the last version of OpenGL (one can hope we get a new version, but I wouldn't hold my breath), so getting these changes into the OpenGL spec itself is very unlikely.

The issue with your patch to MESA is that it would change behavior of existing programs, which is generally not accepted.

MischaBaars commented 7 months ago

The OpenGL 4.6 spec is likely the last version of OpenGL

Why? That would be awful! I'm just getting warmed up!

it would change behavior of existing programs

It would change behavior of existing programs that make use of GL_MIN or GL_MAX as the blend operator, AND make use of blend modes other than GL_ONE. That shouldn't be too many, if you expect the result to be that of blend mode GL_ONE.

getting these changes into the OpenGL spec itself is very unlikely

Why not issue a warning when people try to access outdated behavior, tell them how to modify the code, and let them flip a global variable when ready? We keep the old code in, write new code (that will overwrite GL_MIN and GL_MAX) next to it, and remove the old code in the next release of the specification? That way, there will be no additional tokens AND no unexpected changes in the specification.

That's a lot less code than writing the extensions.

NogginBops commented 7 months ago

Why? That would be awful! I'm just getting warmed up!

Khronos group has changed their focus to work on more modern APIs like Vulkan. There isn't a lot of focus on developing OpenGL further the focus is mostly on maintaining the standard and allowing for extensions and clarifications of the spec. Someone from Khronos can probably expand on this and explain their intentions better.

It would change behavior of existing programs that make use of GL_MIN or GL_MAX as the blend operator, AND make use of blend modes other than GL_ONE. That shouldn't be too many, if you expect the result to be that of blend mode GL_ONE.

Any application that uses GL_MIN or GL_MAX is likely to be affected as the applications probably do not set the factors to GL_ONE as the documentation says they will be ignored. Without numbers on how many applications it would effect I don't expect this breaking change to get accepted. My guess is that there are quite a few applications that actually use the min and max blend operations, just that they are not in the "public eye" so to say.

Why not issue a warning when people try to access outdated behavior, tell them how to modify the code, and let them flip a global variable when ready? We keep the old code in, write new code (that will overwrite GL_MIN and GL_MAX) next to it, and remove the old code in the next release of the specification? That way, there will be no additional tokens AND no unexpected changes in the specification.

This is what extensions are for. They allow vendors to modify and extend OpenGL without having to go through the official standard. I know this isn't what you want to hear, but I think implementing the extension in MESA is likely much easier than getting the OpenGL spec changed in this way. Though it would be nice to get a comment from someone from Khronos about this.

MischaBaars commented 7 months ago

This is what extensions are for. They allow vendors to modify and extend OpenGL without having to go through the official standard. I know this isn't what you want to hear, but I think implementing the extension in MESA is likely much easier than getting the OpenGL spec changed in this way. Though it would be nice to get a comment from someone from Khronos about this.

I'd like to add. I noticed that GL_MIN and GL_MAX are the only two operators specified without FUNC, so GL_FUNC_MIN and GL_FUNC_MAX are missing. I'm now going to write this extension, and I will rename FACTOR_MIN_AMD and FACTOR_MAX_AMD to GL_FUNC_MIN and GL_FUNC_MAX.

Any comment from Khronos is appreciated.

oddhack commented 7 months ago

@NogginBops is correct. The only way there would ever be a new OpenGL core revision is a combination of strong developer demand and a set of compelling, major new features to incorporate in it. This is exceedingly unlikely as OpenGL has been in maintenance mode for more than half a decade. We barely issue a minor bugfix update to the spec once/year, if that, and a handful of new extensions mostly to expose corresponding new SPIR-V capabilities.

If you do write a Mesa extension, please respect the namespace rules. You cannot use the token GL_FUNC_MIN as a new extension token as it is part of the core API namespace, but GL_FUNC_MIN_MESA would be OK (if Mesa wants to adopt and implement your extension).

I suggest you close this issue. If you succeed in getting a Mesa extension specified and implemented, you can bring the extension spec back to include in the extension registry at that point, but none of that will happen in this repository.

MischaBaars commented 7 months ago

in maintenance mode for more than half a decade

This means that you people have been doing a very poor job. The oldest of the two extensions dates back to 2011.

If I just get a bunch of unhappy people in return, just say so, I won't have to begin writing. I don't understand how 168% more blenders wouldn't make you happy.

https://github.com/search?q=org%3AKhronosGroup%20GL_MIN&type=code https://github.com/search?q=org%3AKhronosGroup%20GL_FUNC_MIN&type=code

oddhack commented 7 months ago

Being confrontational is not going to help. But it will discourage people from engaging with you.

The amount of work involved in providing a new extension is a significant cost to everyone involved in specifying, implementing, testing, and maintaining it. All the implementers must balance that against the resources they can justify devoting to OpenGL drivers, and the other priorities they have for those resources.

If thousands of customers were asking for this then there would be considerable motivation. Since very few people have asked for it, there's correspondingly little motivation. It's not about anyone's happiness, it's about how much time they can spend on a one-off feature request.

MischaBaars commented 7 months ago

none of that will happen in this repository

I believe you were the one being confrontational.

Also, I really have no idea what you're talking about. This patch is already ready to be applied, I'd only have to add support for GL_FUNC_MIN, GL_FUNC_MAX and add an extra warning for people that try to use deprecated code. That's all.

Never mind, perhaps you should do it yourself.

NogginBops commented 7 months ago

This patch is already ready to be applied, I'd only have to add support for GL_FUNC_MIN, GL_FUNC_MAX and add an extra warning for people that try to use deprecated code. That's all.

The patch that is ready is for MESA only, it wouldn't make other OpenGL implementations have this by default. The patch also doesn't include any tests which is probably something the MESA people want. It would make a lot more sense for you to implement the AMD or NV extension in MESA as that is something that might actually work. You could also create a separate MESA extension for this.

@oddhack how much work is making an ARB extension in this day and age? Is that even something that happens? I'm feeling like it should be quite easy to merge the AMD and NV extensions into a form that is easy for different types of hardware to support.

MischaBaars commented 7 months ago

Hi Julius,

Because you decided to reopen this issue, I will write you to the best of my abilities, a program that shows what exactly is going wrong in my opinion, and I will fine-tune the patch.

The previous merge update failed on some scripts. This is where the MesaGL developers should pick up, because I am neither an expert in these scripts, nor in MesaGL itself. You people will have to do that yourself. Deal?

Best regards, Mischa.

MischaBaars commented 7 months ago

A screenshot is better:

Screenshot from 2024-01-22 11-40-38

Not only is the blend control data of the Radeon graphics adapter modified along the way, the programmer is made to believe that the blend control data was applied unmodified.

Ok, so what now? Because a have a brand new AMD Radeon RX 6500 XT and I'm very eager to experience blending as intended by the manufacturer instead of by the OpenGL specifications.

zmike commented 7 months ago

@oddhack how much work is making an ARB extension in this day and age? Is that even something that happens? I'm feeling like it should be quite easy to merge the AMD and NV extensions into a form that is easy for different types of hardware to support.

There hasn't been a new ARB extension released in more than 5 years. That's not to say that such a thing is impossible, but nowadays extension releases are typically vendor-driven for specific purposes which have quantifiable demand. Presently there are existing vendor extensions which can fulfill the functionality requested in this issue. Given that there is no significant demand for an ARB version of this vendor-based functionality, I see no reason why a specification-based solution is required.

In short, you have three options:

MischaBaars commented 7 months ago

I see no reason why a specification-based solution is required

Well, I do see reason to do so. The specification is flawed and dis-FUNC-tional, it's implementation misleading. The proposals you see as extensions are in my opinion more like attempts to correct your specification in the first place, and I am not planning to waste any valuable time implementing any extension that should have been implemented by you people some 13 years ago.

oddhack commented 7 months ago

I see no reason why a specification-based solution is required

Well, I do see reason to do so. The specification is flawed and dis-FUNC-tional, it's implementation misleading. The proposals you see as extensions are in my opinion more like attempts to correct your specification in the first place, and I am not planning to waste any valuable time implementing any extension that should have been implemented by you people some 13 years ago.

Mischa, I'll ask you one more time to dial it back. You are going out of your way to insult people who are providing the very API you demand we do something with. This is in violation of the Khronos Code of Conduct that governs this repository.

Please confine yourself to technical issues in activity on our repositories.

MischaBaars commented 7 months ago

My insult:

Screenshot from 2024-01-22 11-40-38

My demand: Bet you can do some pretty neat graphics with these blenders once made available.

I did my best to offer a solution that apparently pleases exactly no one. What else do you want me to say?

MischaBaars commented 5 months ago

2011111100 - Radeon Southern Islands 3D - Compute Register Reference Guide (ALPHA_COMB_FCN in CB:CB_BLEND 0-7 _CONTROL) 2011111100 - Radeon Southern Islands 3D - Compute Register Reference Guide (COLOR_COMB_FCN in CB:CB_BLEND 0-7 _CONTROL)