[vlc-devel] OpenGL stereo capabilities for libglwin32

[Rémi Denis-Courmont]

    Hello,

On Sun, 27 Feb 2011 11:39:02 +1100, Garry Keltie <garry.keltie at rmit.edu.au>
wrote:
>>> * If I'd like to submit changes (I'd assume complete files to diff
> I have attached a first cut at it. I couldn't work out how to 'create' a
> git diff as a product but could create a git patch which seems to do the
> same thing. Two files are changed although one of them, opengl.h will
> affect the glx build (which I haven't looked at yet).

> I tried to build from the repository head (1.2.0) but I'm currently
> building on msys as per the instructions on the website - I ran into
> build issues. So I am still on 1.1.7. Happy to move up as soon as I can
> but may be another thread.

You don't have a choice really... If we were to merge your code to 1.1.7,
it would be lost once 1.2.0 is released. Besides we most probably won't
merge your code to 1.1.7 as it makes invasive changes to an existing
plug-in, thus breaking our informal code freeze policy.

In the end, I can only recommend cross-compiling with MingW, preferably
with a Debian(-like) GNU/Linux installation. That's by far the most
reliable method.


Other than that, I have a few concerns with the patch:

* Use var_Inherit*() rather than var_Create*().

* Don't write/read stereoscopic settings when stereoscopic mode is disabled
(except for the mode setting, of course).

* Define a new function for _Display in stereoscopic mode. There is almost
no common code anyway with the existing normal OpenGL  _Display.

* Also, don't clutter the existing opengl structure with stereoscopic
settings; define a new dedicated structure instead.
I guess your code breaks all other OpenGL outputs, that fail to set the new
members in the existing structure.

* Results from var_*String*() need free().

* The default value of add_float_with_range() must be in the range.

* The minimum of add_float_with_range() must be strictly smaller than the
maximum. Otherwise, there is no point in having an option.

-- 
Rémi Denis-Courmont
http://www.remlab.net
http://fi.linkedin.com/in/remidenis