media player view window for painting frames to the display with YUV conversi...

media player view window for painting frames to the display with YUV conversion and scaling.


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

[fbarchard, 2009-04-27 22:51:04]

I've applied the changes you suggested.
Split view.h into a separate CL so it can be submitted on its own.

[awong, 2009-04-27 23:12:01]

http://codereview.chromium.org/100075/diff/1002/1003
File media/player/view.h (right):

http://codereview.chromium.org/100075/diff/1002/1003#newcode5
Line 5: // View.h : Paints the current movie frame (with scaling) to the display
Period at end of a sentence.

http://codereview.chromium.org/100075/diff/1002/1003#newcode94
Line 94: // condition.
"set it last to avoid race condition" sounds scary to me. :)

Do we need a mutex or some other sort of memory barrier?

http://codereview.chromium.org/100075/diff/1002/1003#newcode109
Line 109: LRESULT OnEraseBackground(UINT /*uMsg*/, WPARAM wParam, LPARAM
/*lParam*/,
uncomment parameters?

http://codereview.chromium.org/100075/diff/1002/1003#newcode111
Line 111: CDCHandle dc = (HDC)wParam;
static cast?

http://codereview.chromium.org/100075/diff/1002/1003#newcode115
Line 115: int x = 0;
it still looks too long...will talk to you in person...

http://codereview.chromium.org/100075/diff/1002/1003#newcode308
Line 308: void DoPaint(CDCHandle dc) {
newline after the end of a function.

http://codereview.chromium.org/100075/diff/1002/1003#newcode352
Line 352: CBitmap bmp_;
Why are these public?

[fbarchard, 2009-04-28 00:19:53]

done

http://codereview.chromium.org/100075/diff/1002/1003
File media/player/view.h (right):

http://codereview.chromium.org/100075/diff/1002/1003#newcode5
Line 5: // View.h : Paints the current movie frame (with scaling) to the display
On 2009/04/27 23:12:01, awong wrote:
> Period at end of a sentence.

Done.

http://codereview.chromium.org/100075/diff/1002/1003#newcode94
Line 94: // condition.
Ralph wrote this and would be better equiped to comment on it.  Should we
include him in the review?

http://codereview.chromium.org/100075/diff/1002/1003#newcode109
Line 109: LRESULT OnEraseBackground(UINT /*uMsg*/, WPARAM wParam, LPARAM
/*lParam*/,
This is best practice for unused parameters in C++.
You need to avoid unused parameters, but you want to document what they should
have been.

http://codereview.chromium.org/100075/diff/1002/1003#newcode111
Line 111: CDCHandle dc = (HDC)wParam;
On 2009/04/27 23:12:01, awong wrote:
> static cast?

Done.

http://codereview.chromium.org/100075/diff/1002/1003#newcode115
Line 115: int x = 0;
hoisted DrawYUV and DumpYUV

http://codereview.chromium.org/100075/diff/1002/1003#newcode308
Line 308: void DoPaint(CDCHandle dc) {
On 2009/04/27 23:12:01, awong wrote:
> newline after the end of a function.

Done.

http://codereview.chromium.org/100075/diff/1002/1003#newcode352
Line 352: CBitmap bmp_;
written by Ralph... I'll see if I can make them private.
But the tendency of WTL is to use globals because everything is in headers.

[awong, 2009-04-28 01:04:29]

lgtm.

Can you add a TODO for those 2 comments that you want ralph to address?  Then
you can check it in and make him sync it down + fix it. :)