Benchmark tool for GPU-accelerated video rendering

Benchmark tool for GPU-accelerated video rendering

This is a benchmarking program that times how fast a shader
can color-convert and render video frames to the screen. The
program takes a file with raw YUV frames, the number of 
frames in the file, and the dimensions of the video file as
parameters and outputs the max frames per second obtained
when rendering the video using various painting/shading
techniques.

BUG=48633
TEST=compiles

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

[vrk (LEFT CHROMIUM), 2010-11-12 05:42:37]

Not finished yet, but wanted to upload what I have so far before the changelist
got too too massive! Sorry that it's already pretty big...

[scherkus (not reviewing), 2010-11-12 07:45:53]

looking great so far!

let me know if you want style reviews + tips etc...

also I'll give my general advice that tools like this can often be time sinks
since you can always add new features :)

http://codereview.chromium.org/4873002/diff/2001/media/tools/shader_bench/sha...
File media/tools/shader_bench/shader_bench.cc (right):

http://codereview.chromium.org/4873002/diff/2001/media/tools/shader_bench/sha...
media/tools/shader_bench/shader_bench.cc:113: GetFrames(file_name, frames,
width, height, num_frames);
just to make sure.. we load everything into memory first?  no disk access during
testing?

[fbarchard, 2010-11-12 17:49:16]

http://codereview.chromium.org/4873002/diff/2001/media/tools/shader_bench/gl_...
File media/tools/shader_bench/gl_painter.cc (right):

http://codereview.chromium.org/4873002/diff/2001/media/tools/shader_bench/gl_...
media/tools/shader_bench/gl_painter.cc:111: glBindTexture(GL_TEXTURE_2D,
textures_[i]);
glBindTexture is slow.  It will cause the glDrawArray to stall.  So you're
measuring latency, not throughput.

http://codereview.chromium.org/4873002/diff/2001/media/tools/shader_bench/gl_...
media/tools/shader_bench/gl_painter.cc:118: context_->SwapBuffers();
SwapBuffers is slow.  Are you sure you want to benchmark this?

[Alpha Left Google, 2010-11-15 20:43:50]

Good job getting things onto the screen!

However there should be a message loop somewhere to perform repaint instead in
the main thread. Also be careful about the use of int. Rule of thumb is that
unless necessary int is good.
http://www.corp.google.com/eng/doc/cppguide.xml#Integer_Types

http://codereview.chromium.org/4873002/diff/2001/media/tools/shader_bench/gl_...
File media/tools/shader_bench/gl_painter.cc (right):

http://codereview.chromium.org/4873002/diff/2001/media/tools/shader_bench/gl_...
media/tools/shader_bench/gl_painter.cc:83: glTexImage2D(GL_TEXTURE_2D, 0,
GL_LUMINANCE, texture_width, texture_height, 0,
nit: 80 chars.

http://codereview.chromium.org/4873002/diff/2001/media/tools/shader_bench/gl_...
media/tools/shader_bench/gl_painter.cc:114: GL_LUMINANCE, GL_UNSIGNED_BYTE,
video_frame->data(i));
nit: indentation.

http://codereview.chromium.org/4873002/diff/2001/media/tools/shader_bench/gl_...
File media/tools/shader_bench/gl_painter.h (right):

http://codereview.chromium.org/4873002/diff/2001/media/tools/shader_bench/gl_...
media/tools/shader_bench/gl_painter.h:13: class GLPainter : public Painter {
Looking into Painter I found it's all related to GL, is it possible that you
merge two classes?

http://codereview.chromium.org/4873002/diff/2001/media/tools/shader_bench/gl_...
media/tools/shader_bench/gl_painter.h:19: virtual void
SetGLContext(gfx::GLContext* context);
Painter class also has a lot of GL stuff in it, maybe assign GLContext in
Initialize() and remove this method?

http://codereview.chromium.org/4873002/diff/2001/media/tools/shader_bench/pai...
File media/tools/shader_bench/painter.h (right):

http://codereview.chromium.org/4873002/diff/2001/media/tools/shader_bench/pai...
media/tools/shader_bench/painter.h:20: virtual void
Paint(scoped_refptr<media::VideoFrame> video_frame) = 0;
This should be protected. But I suggest you merge two classes and make this
private. Using inheritance to share data is generally not a good idea.

http://codereview.chromium.org/4873002/diff/2001/media/tools/shader_bench/pai...
media/tools/shader_bench/painter.h:22: virtual void
LoadFrames(std::deque<scoped_refptr<media::VideoFrame> >* frames);
use const & instead of *

http://codereview.chromium.org/4873002/diff/2001/media/tools/shader_bench/pai...
media/tools/shader_bench/painter.h:23: static GLuint LoadShader(gfx::GLContext*
context, unsigned type,
why are these methods in the Painter interface? Shouldn't they in GLPainter?

http://codereview.chromium.org/4873002/diff/2001/media/tools/shader_bench/sha...
File media/tools/shader_bench/shader_bench.cc (right):

http://codereview.chromium.org/4873002/diff/2001/media/tools/shader_bench/sha...
media/tools/shader_bench/shader_bench.cc:31: unsigned long
CalculateYUVFrameSize(FILE* file_handle, unsigned int num_frames) {
nit: 80 chars.

http://codereview.chromium.org/4873002/diff/2001/media/tools/shader_bench/sha...
media/tools/shader_bench/shader_bench.cc:31: unsigned long
CalculateYUVFrameSize(FILE* file_handle, unsigned int num_frames) {
in general don't need unsigned unless necessary, just use int here

http://codereview.chromium.org/4873002/diff/2001/media/tools/shader_bench/sha...
media/tools/shader_bench/shader_bench.cc:31: unsigned long
CalculateYUVFrameSize(FILE* file_handle, unsigned int num_frames) {
use uint64 for file size and for frame size just int

http://codereview.chromium.org/4873002/diff/2001/media/tools/shader_bench/sha...
media/tools/shader_bench/shader_bench.cc:39:
std::deque<scoped_refptr<media::VideoFrame> >& out_frames,
output object use * instead of reference

also output parameter should be at the end

http://codereview.chromium.org/4873002/diff/2001/media/tools/shader_bench/sha...
media/tools/shader_bench/shader_bench.cc:57: unsigned long bytes_read =
use int

http://codereview.chromium.org/4873002/diff/2001/media/tools/shader_bench/sha...
media/tools/shader_bench/shader_bench.cc:75: window->Paint();
This block of code won't actually measure how fast it paint but how fast we can
see 500 repaint messages to the main thread unless sending the expose event or
WM_PAINT is synchronous.

The code should look like this instead:

Window::OnPaint() {
  if (!started_)
    return;

  // Submit an expose or WM_PAINT message to cause OnPaint to be called again.
  PostRepaint(); 
  gl_painter_->paint();

  if (++count_ == limit) {
    done_task_->Run();
    delete done_task_;
  }
}

Window::Start(int count, Task* done_task) {
  started_ = true;
  count_ = 0;
  limit_ = count;
  done_task_ = done_task;

  // Post the first repaint.
  PostRepaint();
}

Window::MainLoop() {
  // Platform specific message loop.
  ...
  while (true) {
    Message msg = PeekMessage();
    if (msg == Repaint) OnPaint();
    else if (msg == Quit) return;
  }
}

void ShutdownWindow(Window* win) {
  win->Shutdown();
}

void RunTest(int count) {
  // Tell the window to start counting for repaint. When repaint count
  // reaches |count| then call |done_task|.
  window->Start(count, NewRunnableFunction(&ShutdownWindow, window));

  // The message loop of the window system.
  window->MainLoop();
}

int main() {
  ...
  MessageLoop loop;

  Timer.Start();
  RunTest(500);
  Timer.Stop();
  ...
}

The main point is to let the repaint loop run up to |count| and then time how
much time it needs to finish.

http://codereview.chromium.org/4873002/diff/2001/media/tools/shader_bench/win...
File media/tools/shader_bench/window_win.cc (right):

http://codereview.chromium.org/4873002/diff/2001/media/tools/shader_bench/win...
media/tools/shader_bench/window_win.cc:40: }  // namespace.
just // namespace

[vrk (LEFT CHROMIUM), 2010-11-19 01:15:30]

OK, I just uploaded a new patch that reflects the latest version of the
benchmarking tool. Let me know what you think, and let me know if you want me to
give an in-person overview of the design of the tool!

[Alpha Left Google, 2010-11-19 23:33:50]

Please merge with ToT, I can see some merge problems in this patch.

It looks great otherwise! very solid and good job!

I only have a few style comments on this.

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/cp...
File media/tools/shader_bench/cpu_color_painter.h (right):

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/cp...
media/tools/shader_bench/cpu_color_painter.h:22: };
disallow_copy_and_assign

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_color_painter.h (right):

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_color_painter.h:23: GLuint textures_[3];
disallow_copy_and_assign

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_color_painter_exp.cc (right):

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_color_painter_exp.cc:5: #include
"media/tools/shader_bench/gpu_color_painter_exp.h"
why is this file called gpu_color_painter_exp.cc? Doesn't match the class name.

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_color_painter_exp.h (right):

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_color_painter_exp.h:25: };
disallow_copy_and_assign

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_painter.h (right):

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_painter.h:14: class GPUPainter : public Painter {
also mention how this is shared between different GL painters. something like
sharing util methods and context_.

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_painter.h:24: };
disallow_copy_and_assign

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window.cc (right):

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.cc:18: }
//  namespace media

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window.h (right):

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.h:33: }
//  namespace media

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window_linux.cc (right):

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/wi...
media/tools/shader_bench/window_linux.cc:66: if (!running_)
very cool!

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/wi...
media/tools/shader_bench/window_linux.cc:87: } // media
//  namespace media

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window_win.cc (right):

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/wi...
media/tools/shader_bench/window_win.cc:135: }
//  namespace media

[vrk (LEFT CHROMIUM), 2010-11-24 22:01:30]

Thanks for the review! Let me know if the changes are okay. Also, I tried to
rebase and merge with ToT and I didn't have any conflicts? gitk looks right,
too.

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/cp...
File media/tools/shader_bench/cpu_color_painter.h (right):

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/cp...
media/tools/shader_bench/cpu_color_painter.h:22: };
On 2010/11/19 23:33:51, Alpha wrote:
> disallow_copy_and_assign

Done.

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_color_painter.h (right):

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_color_painter.h:23: GLuint textures_[3];
On 2010/11/19 23:33:51, Alpha wrote:
> disallow_copy_and_assign

Done.

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_color_painter_exp.cc (right):

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_color_painter_exp.cc:5: #include
"media/tools/shader_bench/gpu_color_painter_exp.h"
On 2010/11/19 23:33:51, Alpha wrote:
> why is this file called gpu_color_painter_exp.cc? Doesn't match the class
name.

The class names are ridiculously long and specific because I wanted the class to
describe what was implemented... but I didn't think it was worth making the file
names that ridiculously long and specific. Do you think it's worth changing the
file name or class name?

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_color_painter_exp.h (right):

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_color_painter_exp.h:25: };
On 2010/11/19 23:33:51, Alpha wrote:
> disallow_copy_and_assign

Done.

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_painter.h (right):

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_painter.h:14: class GPUPainter : public Painter {
On 2010/11/19 23:33:51, Alpha wrote:
> also mention how this is shared between different GL painters. something like
> sharing util methods and context_.

I'm not quite sure if I understand... The static methods are of course shared,
but the context_ is not necessarily shared (is an instance field, not static).
When I use the painters in shader_bench, I set the contexts all to be the same,
but I could have set them to different contexts.

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_painter.h:24: };
On 2010/11/19 23:33:51, Alpha wrote:
> disallow_copy_and_assign

Done.

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window.cc (right):

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.cc:18: }
On 2010/11/19 23:33:51, Alpha wrote:
> //  namespace media

Done.

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window.h (right):

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.h:33: }
On 2010/11/19 23:33:51, Alpha wrote:
> //  namespace media

Done.

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window_linux.cc (right):

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/wi...
media/tools/shader_bench/window_linux.cc:87: } // media
On 2010/11/19 23:33:51, Alpha wrote:
> //  namespace media

Done.

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window_win.cc (right):

http://codereview.chromium.org/4873002/diff/14001/media/tools/shader_bench/wi...
media/tools/shader_bench/window_win.cc:135: }
On 2010/11/19 23:33:51, Alpha wrote:
> //  namespace media

Done.

[tfarina, 2010-11-26 15:38:16]

Some minor comments below.

http://codereview.chromium.org/4873002/diff/27001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window.h (right):

http://codereview.chromium.org/4873002/diff/27001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.h:15: class Window {
This implementation is very similar to gpu/demos/framework/window.h

Should should they be shared somehow?

http://codereview.chromium.org/4873002/diff/27001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.h:18: virtual void Start(int limit, Task*
done_task, Painter* painter);
Make this 3 virtual methods pure if they are always required by the
implementations?

http://codereview.chromium.org/4873002/diff/27001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.h:31: };
DISALLOW_COPY_AND_ASSIGN?

http://codereview.chromium.org/4873002/diff/27001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window_linux.cc (right):

http://codereview.chromium.org/4873002/diff/27001/media/tools/shader_bench/wi...
media/tools/shader_bench/window_linux.cc:10: #include
"base/utf_string_conversions.h"
Do you need this include?

http://codereview.chromium.org/4873002/diff/27001/media/tools/shader_bench/wi...
media/tools/shader_bench/window_linux.cc:51:
gtk_signal_connect(GTK_OBJECT(GTK_WINDOW(window_handle_)),
no need of GTK_OBJECT. Same thing below.

http://codereview.chromium.org/4873002/diff/27001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window_win.cc (right):

http://codereview.chromium.org/4873002/diff/27001/media/tools/shader_bench/wi...
media/tools/shader_bench/window_win.cc:7: #include
"base/utf_string_conversions.h"
Do you need this include?

[vrk (LEFT CHROMIUM), 2010-11-29 18:14:41]

http://codereview.chromium.org/4873002/diff/27001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window.h (right):

http://codereview.chromium.org/4873002/diff/27001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.h:15: class Window {
On 2010/11/26 15:38:16, tfarina wrote:
> This implementation is very similar to gpu/demos/framework/window.h
> 
> Should should they be shared somehow?

In a word, yes :) But because they are used for pretty different purposes (one
being demos, the other being benchmarks), they are just different enough that
sharing the two classes requires a bit of refactoring. Both the demos and
shader_bench are stand-alone apps not used by chrome, so I would rather defer
that task to another cl if possible!

http://codereview.chromium.org/4873002/diff/27001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.h:18: virtual void Start(int limit, Task*
done_task, Painter* painter);
On 2010/11/26 15:38:16, tfarina wrote:
> Make this 3 virtual methods pure if they are always required by the
> implementations?

The platform-dependent Window implementations are not actually subclasses of
Window, so pure virtual methods don't make sense for this. Either
window_linux.cc or window_win.cc is included depending on what platform this is
being built on, so either the linux or windows definitions will be linked with
the Window header file.

http://codereview.chromium.org/4873002/diff/27001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.h:31: };
On 2010/11/26 15:38:16, tfarina wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

http://codereview.chromium.org/4873002/diff/27001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window_linux.cc (right):

http://codereview.chromium.org/4873002/diff/27001/media/tools/shader_bench/wi...
media/tools/shader_bench/window_linux.cc:10: #include
"base/utf_string_conversions.h"
On 2010/11/26 15:38:16, tfarina wrote:
> Do you need this include?

Nope! Thanks for the catch! Removed.

http://codereview.chromium.org/4873002/diff/27001/media/tools/shader_bench/wi...
media/tools/shader_bench/window_linux.cc:51:
gtk_signal_connect(GTK_OBJECT(GTK_WINDOW(window_handle_)),
On 2010/11/26 15:38:16, tfarina wrote:
> no need of GTK_OBJECT. Same thing below.

When I try to delete GTK_OBJECT, I get a cast error (cannot convert GtkWindow*
to GtkObject*). But I can delete GTK_WINDOW. Thanks!

http://codereview.chromium.org/4873002/diff/27001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window_win.cc (right):

http://codereview.chromium.org/4873002/diff/27001/media/tools/shader_bench/wi...
media/tools/shader_bench/window_win.cc:7: #include
"base/utf_string_conversions.h"
On 2010/11/26 15:38:16, tfarina wrote:
> Do you need this include?

Didn't need this guy either. Thanks!

[tfarina, 2010-11-29 18:28:36]

http://codereview.chromium.org/4873002/diff/27001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window.h (right):

http://codereview.chromium.org/4873002/diff/27001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.h:15: class Window {
On 2010/11/29 18:14:41, Victoria Kirst wrote:
> On 2010/11/26 15:38:16, tfarina wrote:
> > This implementation is very similar to gpu/demos/framework/window.h
> > 
> > Should should they be shared somehow?
> 
> In a word, yes :) But because they are used for pretty different purposes (one
> being demos, the other being benchmarks), they are just different enough that
> sharing the two classes requires a bit of refactoring. Both the demos and
> shader_bench are stand-alone apps not used by chrome, so I would rather defer
> that task to another cl if possible!

Sure :)

That is what I was suggesting, it was more like a note for further reference. 
Maybe file a bug for this?

http://codereview.chromium.org/4873002/diff/27001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.h:18: virtual void Start(int limit, Task*
done_task, Painter* painter);
On 2010/11/29 18:14:41, Victoria Kirst wrote:
> On 2010/11/26 15:38:16, tfarina wrote:
> > Make this 3 virtual methods pure if they are always required by the
> > implementations?
> 
> The platform-dependent Window implementations are not actually subclasses of
> Window, so pure virtual methods don't make sense for this.
Ah, yeah. We are noting doing inheritance here :( 

Like:

class WindowWin : public Window {
...
};

class WindowGtk: public Window {
...
};

I find this more intuitive though. Hence my mistake.

> Either
> window_linux.cc or window_win.cc is included depending on what platform this
is
> being built on, so either the linux or windows definitions will be linked with
> the Window header file.

Yeah, that is an option, but not often used by chrome.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.h:8: #include "base/scoped_ptr.h"
It seems you are not using this include here.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.h:9: #include "base/task.h"
If I'm not wrong, it can be possible to forward declare Task and Painter.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.h:17: Window(int width, int height);
For readability, could you add a blank line between these methods? I also would
move these virtual functions below PluginWindow (no need to address this
though).

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.h:23: 
Please, left the blank line above protected, same thing with private.

[Alpha Left Google, 2010-11-29 20:06:13]

Another pass for style fix.

http://codereview.chromium.org/4873002/diff/33001/media/media.gyp
File media/media.gyp (left):

http://codereview.chromium.org/4873002/diff/33001/media/media.gyp#oldcode83
media/media.gyp:83: 'base/media_filter_collection.cc',
This file is still not merged correctly. You may have to fix this diff manually.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/cp...
File media/tools/shader_bench/cpu_color_painter.h (right):

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/cp...
media/tools/shader_bench/cpu_color_painter.h:19: private:
empty line before this line.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/cp...
media/tools/shader_bench/cpu_color_painter.h:22: };
disallow_copy_and_assign

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_color_painter.h (right):

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_color_painter.h:21: private:
Empty line before private.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_color_painter_exp.h (right):

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_color_painter_exp.h:22: private:
Empty line before this private.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_painter.cc (right):

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_painter.cc:26: GPUPainter::GPUPainter() :
':' goes to next line.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_painter.cc:27: context_(0) {
context_(NULL) instead of 0.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_painter.h (right):

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_painter.h:18: protected:
Empty line before protected.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/pa...
File media/tools/shader_bench/painter.cc (right):

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/pa...
media/tools/shader_bench/painter.cc:8: : frames_(0) {
frames_(NULL), only WebKit uses 0 instead of NULL.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/pa...
File media/tools/shader_bench/painter.h (right):

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/pa...
media/tools/shader_bench/painter.h:33: };
You can add a private section and then disallow_copy_and_assing().

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/sh...
File media/tools/shader_bench/shader_bench.cc (right):

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/sh...
media/tools/shader_bench/shader_bench.cc:32: base::TimeTicks start_;
global variables should be called g_***_.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window.cc (right):

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.cc:10: : done_task_(0),
done_task_(NULL),

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.cc:11: painter_(0),
painter(NULL),

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window.h (right):

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.h:30: private:
Empty line above this. Each section should have an empty line from the previous
one.

[vrk (LEFT CHROMIUM), 2010-11-30 01:21:34]

Thanks for all the feedback! Dah, I keep mixing up WebKit and Chromium style!
Hopefully I corrected everything you guys pointed out, let me know if I missed
something.

In addition to fixing the style according to feedback, I also added comments to
some header files and I print a simple usage message when no arguments are
given.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/cp...
File media/tools/shader_bench/cpu_color_painter.h (right):

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/cp...
media/tools/shader_bench/cpu_color_painter.h:19: private:
On 2010/11/29 20:06:13, Alpha wrote:
> empty line before this line.

Done.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/cp...
media/tools/shader_bench/cpu_color_painter.h:22: };
On 2010/11/29 20:06:13, Alpha wrote:
> disallow_copy_and_assign

Done.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_color_painter.h (right):

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_color_painter.h:21: private:
On 2010/11/29 20:06:13, Alpha wrote:
> Empty line before private.

Done.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_color_painter_exp.h (right):

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_color_painter_exp.h:22: private:
On 2010/11/29 20:06:13, Alpha wrote:
> Empty line before this private.

Done.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_painter.cc (right):

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_painter.cc:26: GPUPainter::GPUPainter() :
On 2010/11/29 20:06:13, Alpha wrote:
> ':' goes to next line.

Done.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_painter.cc:27: context_(0) {
On 2010/11/29 20:06:13, Alpha wrote:
> context_(NULL) instead of 0.

Done.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/pa...
File media/tools/shader_bench/painter.cc (right):

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/pa...
media/tools/shader_bench/painter.cc:8: : frames_(0) {
On 2010/11/29 20:06:13, Alpha wrote:
> frames_(NULL), only WebKit uses 0 instead of NULL.

Done.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/pa...
File media/tools/shader_bench/painter.h (right):

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/pa...
media/tools/shader_bench/painter.h:33: };
On 2010/11/29 20:06:13, Alpha wrote:
> You can add a private section and then disallow_copy_and_assing().

Done.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/sh...
File media/tools/shader_bench/shader_bench.cc (right):

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/sh...
media/tools/shader_bench/shader_bench.cc:32: base::TimeTicks start_;
On 2010/11/29 20:06:13, Alpha wrote:
> global variables should be called g_***_.

Done.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window.cc (right):

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.cc:10: : done_task_(0),
On 2010/11/29 20:06:13, Alpha wrote:
> done_task_(NULL),

Done.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.cc:11: painter_(0),
On 2010/11/29 20:06:13, Alpha wrote:
> painter(NULL),

Done.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window.h (right):

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.h:8: #include "base/scoped_ptr.h"
On 2010/11/29 18:28:36, tfarina wrote:
> It seems you are not using this include here. 

Done.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.h:9: #include "base/task.h"
On 2010/11/29 18:28:36, tfarina wrote:
> If I'm not wrong, it can be possible to forward declare Task and Painter.

Done.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.h:17: Window(int width, int height);
On 2010/11/29 18:28:36, tfarina wrote:
> For readability, could you add a blank line between these methods? I also
would
> move these virtual functions below PluginWindow (no need to address this
> though).

Done.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.h:23: protected:
On 2010/11/29 18:28:36, tfarina wrote:
> Please, left the blank line above protected, same thing with private.

Done.

http://codereview.chromium.org/4873002/diff/33001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.h:30: private:
On 2010/11/29 20:06:13, Alpha wrote:
> Empty line above this. Each section should have an empty line from the
previous
> one.

Done.

[vrk (LEFT CHROMIUM), 2010-11-30 01:22:59]

On 2010/11/29 18:28:36, tfarina wrote:
> That is what I was suggesting, it was more like a note for further reference. 
> Maybe file a bug for this?
> 
Sounds good to me. Will do once this is checked in!

[Alpha Left Google, 2010-11-30 01:47:27]

LGTM.

[tfarina, 2010-11-30 12:51:11]

My nits LGTM.

[scherkus (not reviewing), 2010-11-30 23:36:09]

bunch of nits

also I think you uploaded another patch halfway through the review so apologies
in advance for duplicate comments :(

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_color_painter.cc (right):

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_color_painter.cc:56: : program_id_(-1) {
indent by extra 2 spaces

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_color_painter_exp.cc (right):

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_color_painter_exp.cc:71: : program_id_(-1) {
indent by extra 2 spaces

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_painter.cc (right):

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_painter.cc:27: : context_(NULL) {
indent by extra 2 spaces

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_painter.cc:46: printf("Shader did not compile:
%s\n", log);
LOG(ERROR)?

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_painter.cc:75: printf("Program did not link: %s\n",
log);
LOG(ERROR)?

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_painter.h (right):

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_painter.h:5: #ifndef
MEDIA_TOOLS_BENCHMARK_GPU_PAINTER_H_
should be MEDIA_TOOLS_SHADER_BENCH_GPU_PAINTER_H_

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_painter.h:21: protected:
try to avoid protected

I'd simply make the methods public and/or provide public context() getter

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_painter.h:25: static GLuint
LoadShader(gfx::GLContext* context, unsigned type,
might be wrong but looks like this is only used by CreateShaderProgram(), in
which case this doesn't need to be in .h file

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_painter.h:30: static GLuint
CreateShaderProgram(gfx::GLContext* context,
does it makes sense to have this be a member function instead of static seeing
as it has access to context_?

just curious

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/pa...
File media/tools/shader_bench/painter.cc (right):

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/pa...
media/tools/shader_bench/painter.cc:8: : frames_(NULL) {
indent by extra 2 spaces

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/pa...
File media/tools/shader_bench/painter.h (right):

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/pa...
media/tools/shader_bench/painter.h:5: #ifndef MEDIA_TOOLS_BENCHMARK_PAINTER_H_
header guard should be MEDIA_TOOLS_SHADER_BENCH_PAINTER_H_

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/pa...
media/tools/shader_bench/painter.h:17: 
where's the destructor?

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/sh...
File media/tools/shader_bench/shader_bench.cc (right):

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/sh...
media/tools/shader_bench/shader_bench.cc:32: base::TimeTicks g_start_;
I'd make these static as well

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window.cc (right):

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.cc:12: : done_task_(NULL),
indent by extra two spaces

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window_linux.cc (right):

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/wi...
media/tools/shader_bench/window_linux.cc:22: if (window) window->OnPaint();
nit: try to avoid one-liner-if-statements

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/wi...
media/tools/shader_bench/window_linux.cc:88: } // namespace media
nit: two spaces between } and //

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window_win.cc (right):

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/wi...
media/tools/shader_bench/window_win.cc:77: if (hwnd == NULL) return NULL;
ditto on one-liners

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/cp...
File media/tools/shader_bench/cpu_color_painter.cc (right):

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/cp...
media/tools/shader_bench/cpu_color_painter.cc:36: : program_id_(-1) {
nit: extra 2 spaces

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_color_painter.cc (right):

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_color_painter.cc:56: : program_id_(-1) {
nit: extra 2 spaces

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_color_painter_exp.cc (right):

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_color_painter_exp.cc:71: : program_id_(-1) {
nit: extra 2 spaces

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/pa...
File media/tools/shader_bench/painter.cc (right):

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/pa...
media/tools/shader_bench/painter.cc:8: : frames_(NULL) {
nit: extra 2 spaces

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window.cc (right):

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.cc:12: : done_task_(NULL),
nit: indent by extra 2 spaces

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window.h (right):

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.h:18: 
dtor?

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.h:35: protected:
bit confused... I don't think anyone inherits from Window, right?  can we make
these private?

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window_linux.cc (right):

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/wi...
media/tools/shader_bench/window_linux.cc:22: if (window) window->OnPaint();
nit: one-liners

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window_win.cc (right):

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/wi...
media/tools/shader_bench/window_win.cc:31: if (window != NULL)
window->OnPaint();
nit: one-liners

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/wi...
media/tools/shader_bench/window_win.cc:55: if (!RegisterClass(&wnd_class))
return NULL;
nit: one-liners

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/wi...
media/tools/shader_bench/window_win.cc:77: if (hwnd == NULL) return NULL;
nit: one-liners

[vrk (LEFT CHROMIUM), 2010-12-01 02:46:11]

Thanks Andrew! Fixed everything and reuploaded. 

Also, forgot that I didn't write an implementation for mac haha, so changed the
gyp file to not try to compile on mac.

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_color_painter.cc (right):

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_color_painter.cc:56: : program_id_(-1) {
On 2010/11/30 23:36:10, scherkus wrote:
> indent by extra 2 spaces

Done.

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_color_painter_exp.cc (right):

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_color_painter_exp.cc:71: : program_id_(-1) {
On 2010/11/30 23:36:10, scherkus wrote:
> indent by extra 2 spaces

Done.

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_painter.cc (right):

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_painter.cc:27: : context_(NULL) {
On 2010/11/30 23:36:10, scherkus wrote:
> indent by extra 2 spaces

Done.

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_painter.cc:46: printf("Shader did not compile:
%s\n", log);
On 2010/11/30 23:36:10, scherkus wrote:
> LOG(ERROR)?

Done.

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_painter.cc:75: printf("Program did not link: %s\n",
log);
On 2010/11/30 23:36:10, scherkus wrote:
> LOG(ERROR)?

Done.

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_painter.h (right):

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_painter.h:5: #ifndef
MEDIA_TOOLS_BENCHMARK_GPU_PAINTER_H_
On 2010/11/30 23:36:10, scherkus wrote:
> should be MEDIA_TOOLS_SHADER_BENCH_GPU_PAINTER_H_

Good catch, thanks! Done.

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_painter.h:21: protected:
On 2010/11/30 23:36:10, scherkus wrote:
> try to avoid protected
> 
> I'd simply make the methods public and/or provide public context() getter

Done.

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_painter.h:25: static GLuint
LoadShader(gfx::GLContext* context, unsigned type,
On 2010/11/30 23:36:10, scherkus wrote:
> might be wrong but looks like this is only used by CreateShaderProgram(), in
> which case this doesn't need to be in .h file

Made into private member function instead.

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_painter.h:30: static GLuint
CreateShaderProgram(gfx::GLContext* context,
On 2010/11/30 23:36:10, scherkus wrote:
> does it makes sense to have this be a member function instead of static seeing
> as it has access to context_?
> 
> just curious

Yeah, makes sense! I was just unnecessarily generalizing. Modified the class to
make things simpler.

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/pa...
File media/tools/shader_bench/painter.cc (right):

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/pa...
media/tools/shader_bench/painter.cc:8: : frames_(NULL) {
On 2010/11/30 23:36:10, scherkus wrote:
> indent by extra 2 spaces

Done.

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/pa...
File media/tools/shader_bench/painter.h (right):

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/pa...
media/tools/shader_bench/painter.h:5: #ifndef MEDIA_TOOLS_BENCHMARK_PAINTER_H_
On 2010/11/30 23:36:10, scherkus wrote:
> header guard should be MEDIA_TOOLS_SHADER_BENCH_PAINTER_H_

Done.

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/pa...
media/tools/shader_bench/painter.h:17: 
On 2010/11/30 23:36:10, scherkus wrote:
> where's the destructor?

Oops! Added.

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/sh...
File media/tools/shader_bench/shader_bench.cc (right):

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/sh...
media/tools/shader_bench/shader_bench.cc:32: base::TimeTicks g_start_;
On 2010/11/30 23:36:10, scherkus wrote:
> I'd make these static as well

Done.

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window.cc (right):

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.cc:12: : done_task_(NULL),
On 2010/11/30 23:36:10, scherkus wrote:
> indent by extra two spaces

Done.

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window_linux.cc (right):

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/wi...
media/tools/shader_bench/window_linux.cc:22: if (window) window->OnPaint();
On 2010/11/30 23:36:10, scherkus wrote:
> nit: try to avoid one-liner-if-statements

Done.

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/wi...
media/tools/shader_bench/window_linux.cc:88: } // namespace media
On 2010/11/30 23:36:10, scherkus wrote:
> nit: two spaces between } and //

Done.

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window_win.cc (right):

http://codereview.chromium.org/4873002/diff/50001/media/tools/shader_bench/wi...
media/tools/shader_bench/window_win.cc:77: if (hwnd == NULL) return NULL;
On 2010/11/30 23:36:10, scherkus wrote:
> ditto on one-liners

Done.

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/cp...
File media/tools/shader_bench/cpu_color_painter.cc (right):

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/cp...
media/tools/shader_bench/cpu_color_painter.cc:36: : program_id_(-1) {
On 2010/11/30 23:36:10, scherkus wrote:
> nit: extra 2 spaces

Done.

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_color_painter.cc (right):

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_color_painter.cc:56: : program_id_(-1) {
On 2010/11/30 23:36:10, scherkus wrote:
> nit: extra 2 spaces

Done.

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_color_painter_exp.cc (right):

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_color_painter_exp.cc:71: : program_id_(-1) {
On 2010/11/30 23:36:10, scherkus wrote:
> nit: extra 2 spaces

Done.

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/pa...
File media/tools/shader_bench/painter.cc (right):

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/pa...
media/tools/shader_bench/painter.cc:8: : frames_(NULL) {
On 2010/11/30 23:36:10, scherkus wrote:
> nit: extra 2 spaces

Done.

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window.cc (right):

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.cc:12: : done_task_(NULL),
On 2010/11/30 23:36:10, scherkus wrote:
> nit: indent by extra 2 spaces

Done.

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window.h (right):

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.h:18: 
On 2010/11/30 23:36:10, scherkus wrote:
> dtor?

Talked about it, don't think it's needed in this case.

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/wi...
media/tools/shader_bench/window.h:35: protected:
On 2010/11/30 23:36:10, scherkus wrote:
> bit confused... I don't think anyone inherits from Window, right?  can we make
> these private?

Yup, sorry! When I first wrote the class I was going to have the OS-specific
Windows inherit from Window, but then I saw the gpu/demos/ example and changed
my mind, then blindly kept things protected. Also weirdly kept the above methods
virutal haha, no wonder people were confused. Changed both!

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window_linux.cc (right):

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/wi...
media/tools/shader_bench/window_linux.cc:22: if (window) window->OnPaint();
On 2010/11/30 23:36:10, scherkus wrote:
> nit: one-liners

Done.

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/wi...
File media/tools/shader_bench/window_win.cc (right):

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/wi...
media/tools/shader_bench/window_win.cc:31: if (window != NULL)
window->OnPaint();
On 2010/11/30 23:36:10, scherkus wrote:
> nit: one-liners

Done.

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/wi...
media/tools/shader_bench/window_win.cc:55: if (!RegisterClass(&wnd_class))
return NULL;
On 2010/11/30 23:36:10, scherkus wrote:
> nit: one-liners

Done.

http://codereview.chromium.org/4873002/diff/56001/media/tools/shader_bench/wi...
media/tools/shader_bench/window_win.cc:77: if (hwnd == NULL) return NULL;
On 2010/11/30 23:36:10, scherkus wrote:
> nit: one-liners

Done.

[scherkus (not reviewing), 2010-12-01 05:28:47]

LGTM w/ nits

awesome work!!!!!!!

http://codereview.chromium.org/4873002/diff/72001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_painter.h (right):

http://codereview.chromium.org/4873002/diff/72001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_painter.h:15: 
nit: no blank line

http://codereview.chromium.org/4873002/diff/72001/media/tools/shader_bench/pa...
File media/tools/shader_bench/painter.h (right):

http://codereview.chromium.org/4873002/diff/72001/media/tools/shader_bench/pa...
media/tools/shader_bench/painter.h:34:
std::deque<scoped_refptr<media::VideoFrame> >* frames_;
looks like this doesn't need to be protected either (none of the subclasses
appear to access it)

[vrk (LEFT CHROMIUM), 2010-12-01 18:05:53]

Thanks everyone! Will try to land the patch today!

http://codereview.chromium.org/4873002/diff/72001/media/tools/shader_bench/gp...
File media/tools/shader_bench/gpu_painter.h (right):

http://codereview.chromium.org/4873002/diff/72001/media/tools/shader_bench/gp...
media/tools/shader_bench/gpu_painter.h:15: 
On 2010/12/01 05:28:47, scherkus wrote:
> nit: no blank line

Done.

http://codereview.chromium.org/4873002/diff/72001/media/tools/shader_bench/pa...
File media/tools/shader_bench/painter.h (right):

http://codereview.chromium.org/4873002/diff/72001/media/tools/shader_bench/pa...
media/tools/shader_bench/painter.h:34:
std::deque<scoped_refptr<media::VideoFrame> >* frames_;
On 2010/12/01 05:28:47, scherkus wrote:
> looks like this doesn't need to be protected either (none of the subclasses
> appear to access it)

Done.