Implement GLES video renderer in player_x11

Implement GLES video renderer in player_x11

This patch does the following:
1. Split the implementation of X11 and GL into two separate files
2. Add implementation of GLES render

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=38867

[piman, 2010-02-11 23:59:31]

http://codereview.chromium.org/596055/diff/1001/2003
File media/tools/player_x11/gl_video_renderer.cc (right):

http://codereview.chromium.org/596055/diff/1001/2003#newcode162
media/tools/player_x11/gl_video_renderer.cc:162: glLoadIdentity();
These last 2 lines are actually not needed, so you can remove them here too.

http://codereview.chromium.org/596055/diff/1001/2005
File media/tools/player_x11/gles_video_renderer.cc (right):

http://codereview.chromium.org/596055/diff/1001/2005#newcode66
media/tools/player_x11/gles_video_renderer.cc:66: static const char
kVertexShader[] =
On GLES you'll need to add this small header to get the shaders to compile:
on the vertex shader:
"precision highp float; precision highp int;"

on the fragment shader:
"precision mediump float; precision mediump int;"

http://codereview.chromium.org/596055/diff/1001/2005#newcode191
media/tools/player_x11/gles_video_renderer.cc:191: // glLoadIdentity();
You can simply remove those, we're not using the model view matrix anyway
(that's for the fixed function path, which doesn't exist in gles2).

http://codereview.chromium.org/596055/diff/1001/2005#newcode332
media/tools/player_x11/gles_video_renderer.cc:332: //
glPixelStorei(GL_UNPACK_ROW_LENGTH, frame_in.strides[i]);
This is likely the biggest issue, but we can keep a fast path if stride is just
the packed row size aligned on 1, 2, 4 or 8

[scherkus (not reviewing), 2010-02-12 00:00:28]

drive by!

http://codereview.chromium.org/596055/diff/1001/2004
File media/tools/player_x11/gl_video_renderer.h (right):

http://codereview.chromium.org/596055/diff/1001/2004#newcode18
media/tools/player_x11/gl_video_renderer.h:18: public Renderer {
alignment

http://codereview.chromium.org/596055/diff/1001/2007
File media/tools/player_x11/player_x11.cc (right):

http://codereview.chromium.org/596055/diff/1001/2007#newcode24
media/tools/player_x11/player_x11.cc:24: #ifdef RENDERER_GL
#ifdef ... -> #if defined(...)

http://codereview.chromium.org/596055/diff/1001/2007#newcode80
media/tools/player_x11/player_x11.cc:80: #ifdef RENDERER_GL
same here


hmm... instead of Renderer interface + implementation you might be able to swing
something like GraphicsContext where you typedef Gl/Gles/X11VideoRenderer to a
common class then rely on the function signatures matching

[Alpha Left Google, 2010-02-12 00:18:42]

fixed comments.

http://codereview.chromium.org/596055/diff/1001/2005
File media/tools/player_x11/gles_video_renderer.cc (right):

http://codereview.chromium.org/596055/diff/1001/2005#newcode66
media/tools/player_x11/gles_video_renderer.cc:66: static const char
kVertexShader[] =
On 2010/02/11 23:59:31, piman wrote:
> On GLES you'll need to add this small header to get the shaders to compile:
> on the vertex shader:
> "precision highp float; precision highp int;"
> 
> on the fragment shader:
> "precision mediump float; precision mediump int;"

Done.

http://codereview.chromium.org/596055/diff/1001/2005#newcode191
media/tools/player_x11/gles_video_renderer.cc:191: // glLoadIdentity();
On 2010/02/11 23:59:31, piman wrote:
> You can simply remove those, we're not using the model view matrix anyway
> (that's for the fixed function path, which doesn't exist in gles2).

OK. Done.

http://codereview.chromium.org/596055/diff/1001/2005#newcode332
media/tools/player_x11/gles_video_renderer.cc:332: //
glPixelStorei(GL_UNPACK_ROW_LENGTH, frame_in.strides[i]);
On 2010/02/11 23:59:31, piman wrote:
> This is likely the biggest issue, but we can keep a fast path if stride is
just
> the packed row size aligned on 1, 2, 4 or 8

I'll keep the commented code so you can fix it.

http://codereview.chromium.org/596055/diff/1001/2007
File media/tools/player_x11/player_x11.cc (right):

http://codereview.chromium.org/596055/diff/1001/2007#newcode24
media/tools/player_x11/player_x11.cc:24: #ifdef RENDERER_GL
On 2010/02/12 00:00:28, scherkus wrote:
> #ifdef ... -> #if defined(...)

Done.

http://codereview.chromium.org/596055/diff/1001/2007#newcode80
media/tools/player_x11/player_x11.cc:80: #ifdef RENDERER_GL
On 2010/02/12 00:00:28, scherkus wrote:
> same here
> 
> 
> hmm... instead of Renderer interface + implementation you might be able to
swing
> something like GraphicsContext where you typedef Gl/Gles/X11VideoRenderer to a
> common class then rely on the function signatures matching

sounds like a good idea, will do.

[scherkus (not reviewing), 2010-02-12 01:04:20]

nits but LGTM!

http://codereview.chromium.org/596055/diff/4004/4007
File media/tools/player_x11/gl_video_renderer.h (right):

http://codereview.chromium.org/596055/diff/4004/4007#newcode28
media/tools/player_x11/gl_video_renderer.h:28: virtual void Paint();
this doesn't need to be virtual

http://codereview.chromium.org/596055/diff/4004/4009
File media/tools/player_x11/gles_video_renderer.h (right):

http://codereview.chromium.org/596055/diff/4004/4009#newcode29
media/tools/player_x11/gles_video_renderer.h:29: virtual void Paint();
this doesn't need to be virtual

http://codereview.chromium.org/596055/diff/4004/4010
File media/tools/player_x11/player_x11.cc (right):

http://codereview.chromium.org/596055/diff/4004/4010#newcode27
media/tools/player_x11/player_x11.cc:27: #define Renderer GlVideoRenderer
typedef doesn't work here?

http://codereview.chromium.org/596055/diff/4004/4012
File media/tools/player_x11/x11_video_renderer.h (right):

http://codereview.chromium.org/596055/diff/4004/4012#newcode27
media/tools/player_x11/x11_video_renderer.h:27: virtual void Paint();
this no longers need to be virtual

[piman, 2010-02-12 01:06:21]

LGTM. I'll fix the texture issue when I get a chance, but if the stride matches
the packed row length then it should "just work"