[Mesa-dev] [RFC] Merging feature work to Mesa master

[Ian Romanick]

On 02/16/2012 03:45 PM, Marek Olšák wrote:
> On Thu, Feb 16, 2012 at 12:28 AM, Ian Romanick<idr at freedesktop.org>  wrote:
>> Over the last six months a lot of feature work has happened in Mesa, and the
>> load has been carried by a lot of different people / organization. In the
>> process, we discovered a number of development process issues that made
>> things more difficult than they needed to be.
>>
>> The biggest problem the we encountered was the number of features marked
>> "50% done" in GL3.txt.  To complete these features, a significant amount of
>> time had to be spent figuring out what was done, and what was left to do.
>>   Since the changes had landed on master a very long time ago, this was a
>> frustrating and error prone process.  Transform feedback was the worst case
>> of this.  One developer spent several weeks trying to assess the state of
>> development.  In the process, a couple items were missed and not detected
>> until early January.
>>
>> PROPOSAL: No feature will be merged to master until a vertical slice[1] of
>> functionality is complete.
>>
>> To be specific, this means that some useful workload should be able to
>> correctly execute.  This doesn't mean that everything is done or even that
>> any particular real application needs to work.  At the very least there
>> should be a demo or piglit test that uses the functionality in its intended
>> manner that works.
>>
>> This means that some incomplete features may sit on branches of a long time.
>>   That's okay!  It's really easy to see what has been done on a branch
>> because you can 'git log origin/master..EXT_some_feature'.  This trivializes
>> the assessment of the state of development.
>>
>> We encountered similar problems with pieces of functionality that were
>> ostensibly done.  Many elements of OpenGL functionality are like Koch
>> snowflakes:  everything is a corner case.  Integer textures and
>> floating-point textures are prime cases of this.  Even though the
>> implementation was done and enabled in several drivers, we had no way to
>> assess the quality.  The same problem holds in cases of features that are
>> known to be incomplete, even if a vertical slice is functional.
>>
>> PROPOSAL: No feature will be merged to master until a robust set of tests
>> are implemented or at least documented.
>>
>> We don't necessarily need 10,000 tests for some feature, but there should be
>> some.  There should also be a list of "test this corner case, test that
>> corner case, check this error condition, etc."  As an example, we've come up
>> with a list of missing test cases for EXT_framebuffer_multisample:
>>
>> - Test turning multisample on and off on a MSAA buffer.
>> - Test multisample points smooth
>> - Test multisample points non-smooth
>> - Test multisample lines smooth
>> - Test multisample lines non-smooth
>> - Test multisample line stipple
>> - Test multisample polygon smooth
>> - Test multisample polygon stipple
>> - Test multisample glBitmap
>> - Test sample alpha to one
>> - Test sample coverage
>> - Test sample coverage invert
>> - Test sample alpha-to-coverage, with alpha-to-one
>> - Test sample alpha-to-coverage, without alpha-to-one
>> - Test blit multisample-to-multisample (matching sample count)
>> - Test blit multisample-to-multisample (non-matching sample count)
>> - Test blit multisample-to-multisample (non-matching format)
>> - Test blit singlesample-to-multisample
>> - Test blit multisample-to-multisample (scaling blit)
>> - Iterate all the above for all the sample counts
>> - Iterate all internal formats for some basic MSAA test
>> - "All of the above" for depth and stencil
>>     - Try really hard to hit HiZ resolves too
>>
>> This list represents about 30 or 40 test cases.  By giving some indication
>> of areas that need testing, we communicate the parts of the feature that are
>> known to be incomplete or just plain broken (Keith's 1st Law: Any piece of
>> code that isn't tested is broken.).
>>
>> The test list also need not necessarily come from the developer.  It is
>> completely reasonable and desirable for reviewers to suggest or implement
>> tests.  All of that information just needs to be captured, and consensus
>> must be achieved.
>>
>> The onus of enforcing these proposals will fall on the reviewers.  This
>> still leaves room for sensible things to happen while preventing the
>> problems that we encountered.  For example, it should be fine to merge Marek
>> and nobled's ARB_debug work with few tests implemented or specified.  The
>> same could not be said for the proposed implementation of
>> ARB_uniform_buffer_object.
>
> I don't think your first proposal is anything new. Some of us have
> been doing that already (me included). Also, I don't see how the first
> proposal is related to your complains. Yes, we don't usually write
> down what has been done when a feature is incomplete, because there is
> no _official_ place where we could just dump our personal notes, todo
> lists, ideas, and whatnot.

It's certainly contrary to how some things have been done in Mesa. 
There are already dangling bits for GL_ARB_uniform_buffer_object, 
GL_ARB_texture_buffer_object, GL_EXT_gpu_shader4, and probably others.

> I think you are unhappy with the current processes, because you and
> your colleagues had a punishing deadline in a people-starved project
> and it didn't go exactly as planned. I myself prefer new features to

That is probably what prompted me to act instead of just whine. 
Regardless, we spent several person-weeks trying to figure out what had 
been done on transform feedback.  Had some random contributor in the 
community wanted to implement that feature, they would have likely given 
up and gone to a different project.  This was completely wasted time. 
Given the amount that was done, we would had been better off if nothing 
had been done.  (Note that it didn't help that the person doing the 
investigation was very new to Mesa.)

Not only that, but the code that was done was a) not tested and b) not 
testable (because not enough was implemented to do anything).  As a 
result, a fair amount of the code that did exist was broken.

I guess my point is that half done dangling bits of partial 
functionality do more harm than good.  If that work had still been in a 
branch, it would have at least been easier to isolate and analyze.  git 
log origin/master..branch tells every commit that has been done for that 
feature.

> live in git (e.g. in the master branch) for a while before making a
> release and not trying to push lots of new work in at the last minute.
> Please don't read this that I am ungrateful - I am very grateful about
> what all the devs have done and I wouldn't have done it differently.
>
> I think the problem is not in the process itself, but in the fact that
> this project lacks manpower (at least outside of the Intel camp) and I
> am afraid it will not get better without another 5-10 people working
> full-time on the project. Independent contributors like myself will
> never have enough time to implement tests for every possible corner

I agree.  Even a team of six people don't always have that time. :) 
However, that list of framebuffer multisample tests took one person a 
couple hours to write.  I think that's reasonable, and it has additional 
follow-on benefits.  We can point new contributors at a big list of 
tests when they ask how they can help.

> case which may occur. The question is: should one individual stop
> doing huge and hard work all by himself when it clearly would be
> better done by 3 well-organized people who meet face-to-face in an
> office every day? With this manpower, I don't think so.
>
> I mostly agree with more strict patch reviews and keeping new features
> in branches for a longer period of time.
>
> Note that there is code in core Mesa which Gallium doesn't hit (the
> meta ops are one example, others are swrast, tnl, and all the other
> utility code unused by st/mesa). No matter how well a vertical slice
> is implemented and tested on gallium, additional non-gallium work
> might be needed to support classic drivers as well (and no, it's not
> part of that vertical slice).

I understand that.  There's always work involved in enabling a feature 
on a particular driver.  That will never change.  The same goes for 
features that only exist in classic drivers.  However, if the feature 
can be enabled and do something on one driver, that speaks volumes. 
Just seeing how the feature is implemented into the depths of a gallium 
(or classic) driver gives a lot of indication of how that feature could 
be implemented in the other.  I think Paul's gl_ClipVertex work is an 
example of this.