Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
56076:
media player main() entry point (Closed)
|
Created:
11 years, 8 months ago by fbarchard Modified:
7 years, 2 months ago CC:
chromium-reviews_googlegroups.com Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
Descriptionplayer_wtl main entry point separated from the rest of media player for code review.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=14829
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : '' #Patch Set 8 : '' #Patch Set 9 : '' #Patch Set 10 : '' #Patch Set 11 : '' #Patch Set 12 : '' #Patch Set 13 : '' #Patch Set 14 : '' #Patch Set 15 : '' #Patch Set 16 : '' #Patch Set 17 : '' #
Total comments: 50
Patch Set 18 : '' #
Total comments: 58
Patch Set 19 : '' #Patch Set 20 : '' #
Total comments: 39
Patch Set 21 : '' #
Total comments: 3
Patch Set 22 : '' #Patch Set 23 : '' #
Total comments: 6
Messages
Total messages: 18 (0 generated)
This should be easy to check in. Not. But it would sure help me, if you guys could use the player to find bugs, and maintain the bits affected by your changes. So the fun begins...
removed 32 bpp view option. added audio enable/disable, to allow working from fremont. some code cleanup: -added copyright -removed pragma once -replaced tabs with spaces -view.h follows google coding conventions. To test it, I'm building with the old sln based player.
GYP build file working. Added View->size menu items, but ghosted full screen. Disabled mmx for now (used to apply to ffmpeg decoder).... may want to use it for yuv in future.
On 2009/04/22 00:16:23, fbarchard wrote: > GYP build file working. > Added View->size menu items, but ghosted full screen. > Disabled mmx for now (used to apply to ffmpeg decoder).... may want to use it > for yuv in future. oh man...this is kinda long and actually windows code...it's also been oustanding for 3 weeks without any comments... Should we try to divy this up between the 4 of us (andrew, alpha, ralph, me) and get this checked in this week?
Added time shifted audio. Cleaned up most lint warnings.
Here's pass 1 comments for this file. I kinda did it like a readability review where I generally only highlighted the first instance of an issue, and if possible, linked to the style-guide reference. http://codereview.chromium.org/56076/diff/6266/7192 File media/player/view.h (right): http://codereview.chromium.org/56076/diff/6266/7192#newcode5 Line 5: // View.h : Interface of the CBitmapView class. This comment isn't quite descriptive enough. See link below for guidance. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#File_Comments http://codereview.chromium.org/56076/diff/6266/7192#newcode7 Line 7: #ifndef __VIEW_H__ like you mentioned, non-standard header. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#The__define_Guard http://codereview.chromium.org/56076/diff/6266/7192#newcode13 Line 13: #ifdef TESTING Can we control this via a commandline flag instead of a macro? I realize it adds conditionals, but recompiling kinda sucks. http://codereview.chromium.org/56076/diff/6266/7192#newcode24 Line 24: #include "media/base/buffers.h" How many of these can we remove from the .h if we separate out the implementation into a .cc? http://codereview.chromium.org/56076/diff/6266/7192#newcode36 Line 36: static inline double GetTime(void) { Since this is C++, I think people generally don't put the extra (void) in there on the arg list. http://codereview.chromium.org/56076/diff/6266/7192#newcode44 Line 44: extern bool enable_swscaler; globals are frowned upon. If we're going to use them, consider prefixing with g_? If they are only for this class, make them static? Alternately, hook into the CommandLine class and let this be command-line enabled. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Variable_Names http://codereview.chromium.org/56076/diff/6266/7192#newcode49 Line 49: class WtlVideoWindow : public CScrollWindowImpl<WtlVideoWindow> { Having the implementation inlined in the header is generally avoided. Consider spiltting into a view.cc? Also, instead of prefixing everything with Wtl, how about using a namespace? http://codereview.chromium.org/56076/diff/6266/7192#newcode59 Line 59: size_.cx = 0; What's the "c" mean in "cx"? Can it just be "x"? Hungarian is frowned upon. http://codereview.chromium.org/56076/diff/6266/7192#newcode61 Line 61: renderer_ = new WtlVideoRenderer(this); This call does "do work" does it? I don't know what WtlVideoRenderer does. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Doing_Work_in_... http://codereview.chromium.org/56076/diff/6266/7192#newcode65 Line 65: BOOL PreTranslateMessage(MSG* /*pMsg*/) { Prefer bool over BOOL. Also, don't comment out the unused argument, and drop the hungarian. http://codereview.chromium.org/56076/diff/6266/7192#newcode84 Line 84: void * pBits; void*, not void * http://codereview.chromium.org/56076/diff/6266/7192#newcode86 Line 86: // bmp_.CreateCompatibleBitmap(dc, size_.cx, size_.cy); avoid commented out dead code. http://codereview.chromium.org/56076/diff/6266/7192#newcode144 Line 144: bool lock_result = video_frame->Lock(&frame_in); This function is too long. Unless there's some driving reason that it all has to be one blob, generally the rule of thumb is to try to keep it around 40 lines max. That number, ofcourse, is just a guideline. However, this feels like we have logic sections that can be broken apart. Example: CalculateScaledSize(clipped_width, cliped_height, view_size, &scaled_width, &scaled_height); DumpYuvFile(frame_in); etc. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Write_Short_Fu... http://codereview.chromium.org/56076/diff/6266/7192#newcode151 Line 151: uint8 *movie_dib_bits = (uint8 *)bm.bmBits break line after the operator. http://codereview.chromium.org/56076/diff/6266/7192#newcode155 Line 155: if (dibwidth < clipped_width) clipped_width = dibwidth; I think our code seems to prefer putting the statement on the next line. Change here and below? http://codereview.chromium.org/56076/diff/6266/7192#newcode162 Line 162: case 0: comment explaining ordering of case statements since default isn't at the end. http://codereview.chromium.org/56076/diff/6266/7192#newcode347 Line 347: } newline after the end of a function. http://codereview.chromium.org/56076/diff/6266/7192#newcode350 Line 350: scoped_refptr<WtlVideoRenderer> renderer_; I'm not looking at WtlVideoRenderer's usage, but do we really need a ref-counted pointer? This is a standalone player no? Can't you just instantiate one of these in main and pass it to everyone w/o needing refcounts? http://codereview.chromium.org/56076/diff/6266/7192#newcode351 Line 351: private: newline before the private:
Reviewed movie.h and movie.cc Biggest recommendation is to refactor the globals into a singleton class (see media/filters/ffmpeg_glue.h for an example) http://codereview.chromium.org/56076/diff/7204/7213 File media/player/movie.cc (right): http://codereview.chromium.org/56076/diff/7204/7213#newcode5 Line 5: // movie.cc code that binds pipeline to wtl usually we put top-of-file explanations in the .h as opposed to the .cc http://codereview.chromium.org/56076/diff/7204/7213#newcode13 Line 13: #include "media/filters/file_data_source.h" alphabetize includes http://codereview.chromium.org/56076/diff/7204/7213#newcode17 Line 17: remove extra line here http://codereview.chromium.org/56076/diff/7204/7213#newcode21 Line 21: #include "base/string_util.h" alphabetize includes here http://codereview.chromium.org/56076/diff/7204/7213#newcode22 Line 22: using media::PipelineImpl; space before your first using line, and alphabetize these using statements http://codereview.chromium.org/56076/diff/7204/7213#newcode31 Line 31: HBITMAP movieDib = NULL; avoid camelcase style, so these could be HBITMAP movie_bitmap HWND movie_hwnd http://codereview.chromium.org/56076/diff/7204/7213#newcode33 Line 33: extern bool g_enableaudio; tiny nit, but really should be enable_audio http://codereview.chromium.org/56076/diff/7204/7213#newcode38 Line 38: float play_rate = 1.0f; all of these can part of the Movie singleton class http://codereview.chromium.org/56076/diff/7204/7213#newcode40 Line 40: bool MovieIsOpen(void) { can ditch the void http://codereview.chromium.org/56076/diff/7204/7213#newcode41 Line 41: return pipeline ? true : false; you can just check for NULL instead of a conditional http://codereview.chromium.org/56076/diff/7204/7213#newcode49 Line 49: bool OpenMovie(const TCHAR *url, WtlVideoWindow* video_window) { pointer goes with the type http://codereview.chromium.org/56076/diff/7204/7213#newcode50 Line 50: // close previous movie Capitalize "close" and add a period http://codereview.chromium.org/56076/diff/7204/7213#newcode51 Line 51: if (pipeline) { looks like you can just call CloseMovie() here instead http://codereview.chromium.org/56076/diff/7204/7213#newcode53 Line 53: ::Sleep(100); // Allow pipeline threads time to shutdown. do we need this? this looks like a race condition waiting to happen :) nit: use PlatformThread::Sleep() (it's in milliseconds as well, but will help with porting to platforms) http://codereview.chromium.org/56076/diff/7204/7213#newcode66 Line 66: if (g_enableaudio) tiny nit: instead of an if/else, you can actually just add the NullAudioRenderer BEFORE AudioRendererImpl if g_enableenable is false We walk down the list, so NullAudioRenderer would get created instead of AudioRendererImpl maybe add braces for clarity as well http://codereview.chromium.org/56076/diff/7204/7213#newcode70 Line 70: factories->AddFactory(new media::InstanceFilterFactory<WtlVideoRenderer> drop the whole "new ..." statement onto the next line indented by 4 http://codereview.chromium.org/56076/diff/7204/7213#newcode73 Line 73: pipeline = new(PipelineImpl); syntax is a little strange.. just use this instead: pipeline = new PipelineImpl(); http://codereview.chromium.org/56076/diff/7204/7213#newcode76 Line 76: ::WideToUTF8(url, 512, &url8); why/how 512? also this is where you can get burned using TCHAR since on non-unicode builds TCHAR is a char, not a wchar_t also we don't tend to use the scope operator for these functions http://codereview.chromium.org/56076/diff/7204/7213#newcode81 Line 81: ::Sleep(100); nit: use PlatformThread::Sleep() (it's in milliseconds as well, but will help with porting to platforms) http://codereview.chromium.org/56076/diff/7204/7213#newcode90 Line 90: remove extra line http://codereview.chromium.org/56076/diff/7204/7213#newcode107 Line 107: void CloseMovie(void) { remove void http://codereview.chromium.org/56076/diff/7204/7213#newcode110 Line 110: ::Sleep(100); // Allow pipeline threads time to shutdown (comment shamelessly copied from above) do we need this? this looks like a race condition waiting to happen :) nit: use PlatformThread::Sleep() (it's in milliseconds as well, but will help with porting to platforms) http://codereview.chromium.org/56076/diff/7204/7214 File media/player/movie.h (right): http://codereview.chromium.org/56076/diff/7204/7214#newcode1 Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. Copyright should be 2009 (as should all other new files) http://codereview.chromium.org/56076/diff/7204/7214#newcode6 Line 6: #define MEDIA_PLAYER_MOVIE_H_ should add a top-of-file comment briefly explaining the purpose of movie.h and movie.cc http://codereview.chromium.org/56076/diff/7204/7214#newcode10 Line 10: extern HBITMAP movieDib; you can expose these as part of the singleton http://codereview.chromium.org/56076/diff/7204/7214#newcode15 Line 15: bool OpenMovie(const TCHAR *url, WtlVideoWindow* video_window); I'd highly recommend turning this into a class and making it a singleton using base/singleton.h It's a cleaner way of implementing global-ness and you would access it via: Movie::get()->OpenMovie(...) Movie::get()->PlayMovie(...) Take a look at media/filters/ffmpeg_glue.h for an example http://codereview.chromium.org/56076/diff/7204/7214#newcode15 Line 15: bool OpenMovie(const TCHAR *url, WtlVideoWindow* video_window); pointers go with the typename also would it be possible to ditch TCHAR all together and use std::wstring or string16? http://codereview.chromium.org/56076/diff/7204/7214#newcode17 Line 17: // TODO(fbarchard): Readd SetFrameBuffer or remove Typo on "Readd" and no period Do we still need this or can it be removed? http://codereview.chromium.org/56076/diff/7204/7214#newcode21 Line 21: bool MovieIsOpen(void); can get rid of void http://codereview.chromium.org/56076/diff/7204/7214#newcode21 Line 21: bool MovieIsOpen(void); usually boolean functions are in the style of: IsSomeCondition() So here we'd have: IsMovieOpen()
Thanks Albert. We're making progress... about 30% checked in now... http://codereview.chromium.org/56076/diff/6266/7192 File media/player/view.h (right): http://codereview.chromium.org/56076/diff/6266/7192#newcode5 Line 5: // View.h : Interface of the CBitmapView class. On 2009/04/22 22:13:27, awong wrote: > This comment isn't quite descriptive enough. See link below for guidance. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#File_Comments Done. http://codereview.chromium.org/56076/diff/6266/7192#newcode7 Line 7: #ifndef __VIEW_H__ On 2009/04/22 22:13:27, awong wrote: > like you mentioned, non-standard header. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#The__define_Guard > Done. http://codereview.chromium.org/56076/diff/6266/7192#newcode7 Line 7: #ifndef __VIEW_H__ On 2009/04/22 22:13:27, awong wrote: > like you mentioned, non-standard header. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#The__define_Guard > Done. http://codereview.chromium.org/56076/diff/6266/7192#newcode13 Line 13: #ifdef TESTING adding command line parsing is a new feature request. It'll likely happen in the next week. For now the code is off by default. http://codereview.chromium.org/56076/diff/6266/7192#newcode24 Line 24: #include "media/base/buffers.h" WTL is template oriented... using cc files is not generally done. as far as I know, these headers are needed, except for the fact that this header is included by a cc which includes other headers which use these headers. http://codereview.chromium.org/56076/diff/6266/7192#newcode36 Line 36: static inline double GetTime(void) { it was a coding standard at my old work place. http://codereview.chromium.org/56076/diff/6266/7192#newcode36 Line 36: static inline double GetTime(void) { On 2009/04/22 22:13:27, awong wrote: > Since this is C++, I think people generally don't put the extra (void) in there > on the arg list. Done. http://codereview.chromium.org/56076/diff/6266/7192#newcode36 Line 36: static inline double GetTime(void) { On 2009/04/22 22:13:27, awong wrote: > Since this is C++, I think people generally don't put the extra (void) in there > on the arg list. Done. http://codereview.chromium.org/56076/diff/6266/7192#newcode44 Line 44: extern bool enable_swscaler; g_ ? I thought hungarian was frowned on. I'll go with that for now. command line is coming and will expose all of these... the idea being every menu item is a command line switch as well. But thats a bit of a new feature. The design of WTL and these particular options does not easily allow them to be class members. http://codereview.chromium.org/56076/diff/6266/7192#newcode49 Line 49: class WtlVideoWindow : public CScrollWindowImpl<WtlVideoWindow> { Re header WTL generally uses templates, so a header is the place to put templates. This particular file, may be able to change. Is it worth the effort? Its inconsistent with the original microsoft design with all windows, dialogs, menus etc in templated headers, so for orthogonality, its best kept a header. A name space would be okay, but I'd have to do it consistently, and being a tool, it wouldnt resolve any real conflicts. http://codereview.chromium.org/56076/diff/6266/7192#newcode59 Line 59: size_.cx = 0; sorry, I dont know. this came from the original sample and works as is. my guess would be that its an acronym for something like center x. when i add window sizing, i'll likely need to get to know this. http://codereview.chromium.org/56076/diff/6266/7192#newcode61 Line 61: renderer_ = new WtlVideoRenderer(this); I'm starting to think ralph should help out... since he and Andrew wrote the original renderer. It used to do work, when it was SDL. I think its lazy now, deferring the window creation til we get movie frames. Ralph did a rewrite of this recently. http://codereview.chromium.org/56076/diff/6266/7192#newcode65 Line 65: BOOL PreTranslateMessage(MSG* /*pMsg*/) { google coding standards say its best practice to use the native types in such situations. The example they give is DWORD. pMsg is also native style, and just a comment. But okay, I'll change that to msg. oh no... thats an acronym for monosodium glutamate! It should be uppercase :-) http://codereview.chromium.org/56076/diff/6266/7192#newcode84 Line 84: void * pBits; those idiots at microsoft! not only is the white space wrong, they used Hungarian notation. http://codereview.chromium.org/56076/diff/6266/7192#newcode86 Line 86: // bmp_.CreateCompatibleBitmap(dc, size_.cx, size_.cy); okay, but I think ralph did this, and we have never checked in the code, so there is some value in showing the old/alternative way to do it. http://codereview.chromium.org/56076/diff/6266/7192#newcode144 Line 144: bool lock_result = video_frame->Lock(&frame_in); There are 2 reasons this function is long 1. debug goop, which I plan to remove, not formalize 2. conversion, including debug versions. In the future, I plan to have a central high level conversion function. But for now, I experiment with several low levels. If you're seriously against the size, I would simply remove the debug goop for now, since its not worth formalizing. http://codereview.chromium.org/56076/diff/6266/7192#newcode151 Line 151: uint8 *movie_dib_bits = (uint8 *)bm.bmBits I trust youre right on this, because I dont recall seeing this in the coding standards http://codereview.chromium.org/56076/diff/6266/7192#newcode155 Line 155: if (dibwidth < clipped_width) clipped_width = dibwidth; coding standards say its preferred, but acceptable to do either way. But as you bring it up, I'll go ahead and do it the googly way with all the {} fixin's :) http://codereview.chromium.org/56076/diff/6266/7192#newcode162 Line 162: case 0: On 2009/04/22 22:13:27, awong wrote: > comment explaining ordering of case statements since default isn't at the end. Done. http://codereview.chromium.org/56076/diff/6266/7192#newcode347 Line 347: } doh! I've added a comment just to make this clear, that its the end of DoPaint http://codereview.chromium.org/56076/diff/6266/7192#newcode350 Line 350: scoped_refptr<WtlVideoRenderer> renderer_; sorry, I'm not qualified to answer that question. Ralph wrote this bit. My inclination is that you're right, that its overkill. The app can only have one window right now. It's likely just for future and/or chrome considerations http://codereview.chromium.org/56076/diff/6266/7192#newcode351 Line 351: private: On 2009/04/22 22:13:27, awong wrote: > newline before the private: Done. http://codereview.chromium.org/56076/diff/6266/7192#newcode359 Line 359: Thanks Albert!
So, I'd like one more person to look over this (andrew?) file before checkin just to make sure we're not missing any major general issues. At this point, it's looking good enough that I think it's basically checkinable as long as all the warts are TODOed. Why TODO it? Cause i'm a pessamist and generally believe that once it's checked in, it's not changing much in terms of style. :) Thus having the TODO is really more of a marker to people 2-3 years down the line saying "don't copy this style." http://codereview.chromium.org/56076/diff/6266/7192 File media/player/view.h (right): http://codereview.chromium.org/56076/diff/6266/7192#newcode24 Line 24: #include "media/base/buffers.h" On 2009/04/25 00:42:02, fbarchard wrote: > WTL is template oriented... using cc files is not generally done. > as far as I know, these headers are needed, except for the fact that this header > is included by a cc which includes other headers which use these headers. > > Hmm...I don't know how much we break MS coding style to fit Google coding style. I still feel like WtlVideoWindow doesn't really need to be implemented in the header. There aren't any template declarations in here, so they're separable, and I don't see any benefit of collocating the header. If you do split it, Is till think some of these headers can be pruned out of hte .h... http://codereview.chromium.org/56076/diff/6266/7192#newcode44 Line 44: extern bool enable_swscaler; On 2009/04/25 00:42:02, fbarchard wrote: > g_ ? I thought hungarian was frowned on. > I'll go with that for now. > command line is coming and will expose all of these... the idea being every menu > item is a command line switch as well. But thats a bit of a new feature. > The design of WTL and these particular options does not easily allow them to be > class members. Yeah, hungarian is unliked...except in this one case. :) Style guide puts the single exception in there for globals. If this is the case, I'm tempted to suggest going with Andrew's comment about moving to a singleton for config parameters. http://codereview.chromium.org/56076/diff/6266/7192#newcode49 Line 49: class WtlVideoWindow : public CScrollWindowImpl<WtlVideoWindow> { On 2009/04/25 00:42:02, fbarchard wrote: > Re header > WTL generally uses templates, so a header is the place to put templates. This > particular file, may be able to change. Is it worth the effort? Its > inconsistent with the original microsoft design with all windows, dialogs, menus > etc in templated headers, so for orthogonality, its best kept a header. > A name space would be okay, but I'd have to do it consistently, and being a > tool, it wouldnt resolve any real conflicts. I'm willing to budget on the not splittingup the implemenation (though that doesn't seem too hard). However, the namespace thing, that's something I think we should stick to stronger. I'd rather not see a medium sized piece of code without a namespace sitting checked in. We can argue about this you want, but let's do that in person... http://codereview.chromium.org/56076/diff/6266/7192#newcode61 Line 61: renderer_ = new WtlVideoRenderer(this); On 2009/04/25 00:42:02, fbarchard wrote: > I'm starting to think ralph should help out... since he and Andrew wrote the > original renderer. > > It used to do work, when it was SDL. I think its lazy now, deferring the window > creation til we get movie frames. Ralph did a rewrite of this recently. Ralph? Andrew? Comments? http://codereview.chromium.org/56076/diff/6266/7192#newcode144 Line 144: bool lock_result = video_frame->Lock(&frame_in); On 2009/04/25 00:42:02, fbarchard wrote: > There are 2 reasons this function is long > 1. debug goop, which I plan to remove, not formalize > 2. conversion, including debug versions. In the future, I plan to have a > central high level conversion function. But for now, I experiment with several > low levels. > If you're seriously against the size, I would simply remove the debug goop for > now, since its not worth formalizing. I'm fine with TODOing it if we can make sure to chop it down somewhere soonish (where soonish = next couple of weeks). http://codereview.chromium.org/56076/diff/6266/7192#newcode155 Line 155: if (dibwidth < clipped_width) clipped_width = dibwidth; On 2009/04/25 00:42:02, fbarchard wrote: > coding standards say its preferred, but acceptable to do either way. > But as you bring it up, I'll go ahead and do it the googly way with all the {} > fixin's :) Heh...actually google codes does it all 3 ways. The rule on this one is stay consistent within a file, and then within a project. http://codereview.chromium.org/56076/diff/6266/7192#newcode347 Line 347: } On 2009/04/25 00:42:02, fbarchard wrote: > doh! I've added a comment just to make this clear, that its the end of DoPaint errr...that's actually worse I think. But it's up to you. http://codereview.chromium.org/56076/diff/6266/7192#newcode350 Line 350: scoped_refptr<WtlVideoRenderer> renderer_; On 2009/04/25 00:42:02, fbarchard wrote: > sorry, I'm not qualified to answer that question. Ralph wrote this bit. > > My inclination is that you're right, that its overkill. The app can only have > one window right now. It's likely just for future and/or chrome considerations Ralph?
header issues addressed, but not movie.cc http://codereview.chromium.org/56076/diff/7204/7214 File media/player/movie.h (right): http://codereview.chromium.org/56076/diff/7204/7214#newcode1 Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved. On 2009/04/23 21:51:23, scherkus wrote: > Copyright should be 2009 (as should all other new files) Done. http://codereview.chromium.org/56076/diff/7204/7214#newcode6 Line 6: #define MEDIA_PLAYER_MOVIE_H_ On 2009/04/23 21:51:23, scherkus wrote: > should add a top-of-file comment briefly explaining the purpose of movie.h and > movie.cc Done. http://codereview.chromium.org/56076/diff/7204/7214#newcode10 Line 10: extern HBITMAP movieDib; On 2009/04/23 21:51:23, scherkus wrote: > you can expose these as part of the singleton singleton idea limits all variables to be instantiated in one place. right now each variable is instantiated where it is used, and externed elsewhere. http://codereview.chromium.org/56076/diff/7204/7214#newcode15 Line 15: bool OpenMovie(const TCHAR *url, WtlVideoWindow* video_window); okay, I'll look into this, but it'll take a bit longer... http://codereview.chromium.org/56076/diff/7204/7214#newcode17 Line 17: // TODO(fbarchard): Readd SetFrameBuffer or remove On 2009/04/23 21:51:23, scherkus wrote: > Typo on "Readd" and no period > > Do we still need this or can it be removed? Done. I'd like the image functionality back. Ralph hacked it out to get things working quickly. I'll take a look to see if its easy or not. http://codereview.chromium.org/56076/diff/7204/7214#newcode21 Line 21: bool MovieIsOpen(void); On 2009/04/23 21:51:23, scherkus wrote: > can get rid of void Done. http://codereview.chromium.org/56076/diff/7204/7214#newcode21 Line 21: bool MovieIsOpen(void); I had a class or namespace in mind, so it would be Movie.IsOpen(). Is that good?
This CL reduced to just the main entry point for player_wtl.cc Cleaned up lint warnings, except header order, which is not easily fixed. http://codereview.chromium.org/56076/diff/7204/7213 File media/player/movie.cc (right): http://codereview.chromium.org/56076/diff/7204/7213#newcode5 Line 5: // movie.cc code that binds pipeline to wtl On 2009/04/23 21:51:23, scherkus wrote: > usually we put top-of-file explanations in the .h as opposed to the .cc Done. http://codereview.chromium.org/56076/diff/7204/7213#newcode13 Line 13: #include "media/filters/file_data_source.h" On 2009/04/23 21:51:23, scherkus wrote: > alphabetize includes Done. http://codereview.chromium.org/56076/diff/7204/7213#newcode17 Line 17: On 2009/04/23 21:51:23, scherkus wrote: > remove extra line here Done. http://codereview.chromium.org/56076/diff/7204/7213#newcode21 Line 21: #include "base/string_util.h" On 2009/04/23 21:51:23, scherkus wrote: > alphabetize includes here Done. http://codereview.chromium.org/56076/diff/7204/7213#newcode22 Line 22: using media::PipelineImpl; On 2009/04/23 21:51:23, scherkus wrote: > space before your first using line, and alphabetize these using statements Done. http://codereview.chromium.org/56076/diff/7204/7213#newcode31 Line 31: HBITMAP movieDib = NULL; On 2009/04/23 21:51:23, scherkus wrote: > avoid camelcase style, so these could be > HBITMAP movie_bitmap > HWND movie_hwnd Done. http://codereview.chromium.org/56076/diff/7204/7213#newcode33 Line 33: extern bool g_enableaudio; On 2009/04/23 21:51:23, scherkus wrote: > tiny nit, but really should be enable_audio Done. http://codereview.chromium.org/56076/diff/7204/7213#newcode38 Line 38: float play_rate = 1.0f; in this pass I'll keep the functions global... will review for the class version http://codereview.chromium.org/56076/diff/7204/7213#newcode40 Line 40: bool MovieIsOpen(void) { On 2009/04/23 21:51:23, scherkus wrote: > can ditch the void Done. http://codereview.chromium.org/56076/diff/7204/7213#newcode49 Line 49: bool OpenMovie(const TCHAR *url, WtlVideoWindow* video_window) { On 2009/04/23 21:51:23, scherkus wrote: > pointer goes with the type Done. http://codereview.chromium.org/56076/diff/7204/7213#newcode50 Line 50: // close previous movie On 2009/04/23 21:51:23, scherkus wrote: > Capitalize "close" and add a period Done. http://codereview.chromium.org/56076/diff/7204/7213#newcode51 Line 51: if (pipeline) { On 2009/04/23 21:51:23, scherkus wrote: > looks like you can just call CloseMovie() here instead Done. http://codereview.chromium.org/56076/diff/7204/7213#newcode53 Line 53: ::Sleep(100); // Allow pipeline threads time to shutdown. This code is not working reliably, so sleep() was a shoot in the dark. I'll remove for now. http://codereview.chromium.org/56076/diff/7204/7213#newcode66 Line 66: if (g_enableaudio) if you mean unconditionally, that seems implementation specific behavior. I've added {} and switched the order, if thats what you meant. http://codereview.chromium.org/56076/diff/7204/7213#newcode70 Line 70: factories->AddFactory(new media::InstanceFilterFactory<WtlVideoRenderer> it doesn't fit 2 lines, but I'll make it 3 lines and see what you think factories->AddFactory( new media::InstanceFilterFactory<WtlVideoRenderer>(video_window->renderer_) ); http://codereview.chromium.org/56076/diff/7204/7213#newcode73 Line 73: pipeline = new(PipelineImpl); On 2009/04/23 21:51:23, scherkus wrote: > syntax is a little strange.. just use this instead: > pipeline = new PipelineImpl(); > Done. http://codereview.chromium.org/56076/diff/7204/7213#newcode76 Line 76: ::WideToUTF8(url, 512, &url8); 512 is ANSI recommended string length. WTL gives me TCHAR, which is wchar_t. Whats the best way to convert to string? I've added a TODO for now http://codereview.chromium.org/56076/diff/7204/7213#newcode81 Line 81: ::Sleep(100); On 2009/04/23 21:51:23, scherkus wrote: > nit: use PlatformThread::Sleep() (it's in milliseconds as well, but will help > with porting to platforms) Done. http://codereview.chromium.org/56076/diff/7204/7213#newcode90 Line 90: On 2009/04/23 21:51:23, scherkus wrote: > remove extra line Done. http://codereview.chromium.org/56076/diff/7204/7213#newcode107 Line 107: void CloseMovie(void) { On 2009/04/23 21:51:23, scherkus wrote: > remove void Done. http://codereview.chromium.org/56076/diff/7204/7213#newcode110 Line 110: ::Sleep(100); // Allow pipeline threads time to shutdown On 2009/04/23 21:51:23, scherkus wrote: > (comment shamelessly copied from above) > > do we need this? this looks like a race condition waiting to happen :) > > nit: use PlatformThread::Sleep() (it's in milliseconds as well, but will help > with porting to platforms) Done.
http://codereview.chromium.org/56076/diff/11004/11005 File media/player/player_wtl.cc (right): http://codereview.chromium.org/56076/diff/11004/11005#newcode1 Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved. year should be 2009 http://codereview.chromium.org/56076/diff/11004/11005#newcode15 Line 15: // Note Headers are order sensitive. nit: Headers -> headers http://codereview.chromium.org/56076/diff/11004/11005#newcode30 Line 30: nit: remove blank line http://codereview.chromium.org/56076/diff/11004/11005#newcode31 Line 31: #include "media/player/movie.h" these headers shouldn't be order sensitive... is it a side effect of having header files with inlined implementations? if so include the "order sensitive" comment here http://codereview.chromium.org/56076/diff/11004/11005#newcode38 Line 38: nit: remove blank line http://codereview.chromium.org/56076/diff/11004/11005#newcode45 Line 45: CAppModule _Module; _Module -> g_module add a comment saying this defines an application instance required by WTL is this accessed outside of this file? otherwise an anonymous namespace would be good http://codereview.chromium.org/56076/diff/11004/11005#newcode47 Line 47: int Run(LPTSTR lpstrCmdLine = NULL, int nCmdShow = SW_SHOWDEFAULT) { LPTSTR -> TCHAR* lpstrCmdLine -> cmd_line nCmdShow -> cmd_show get rid of the default parameters too http://codereview.chromium.org/56076/diff/11004/11005#newcode48 Line 48: CMessageLoop theLoop; theLoop -> message_loop http://codereview.chromium.org/56076/diff/11004/11005#newcode51 Line 51: CMainFrame wndMain; wndMain -> main_frame (or similar) http://codereview.chromium.org/56076/diff/11004/11005#newcode53 Line 53: ATLTRACE(_T("Main window creation failed!\n")); better to include base/logging.h and use: DLOG(FATAL) << "Main window creation failed!"; or you can assert with: DCHECK(false) << "Main window creation failed!"; http://codereview.chromium.org/56076/diff/11004/11005#newcode61 Line 61: TCHAR *url = NULL; pointer with type http://codereview.chromium.org/56076/diff/11004/11005#newcode68 Line 68: } can simplify all of this with: if (lpstrCmdLine && *lpstrCmdLine) { wndMain.MovieOpenFile(*lpstrCmdLine) } http://codereview.chromium.org/56076/diff/11004/11005#newcode70 Line 70: int nRet = theLoop.Run(); nRet -> result http://codereview.chromium.org/56076/diff/11004/11005#newcode79 Line 79: LPTSTR lpstrCmdLine, int nCmdShow) { hInstance -> instance hPrevInstance -> prev_instance lpstrCmdLine -> cmd_line nCmdShow -> cmd_show LPTSTR -> TCHAR* http://codereview.chromium.org/56076/diff/11004/11005#newcode84 Line 84: bRet; remove void statement http://codereview.chromium.org/56076/diff/11004/11005#newcode85 Line 85: ATLASSERT(bRet); I would simply replace the above little bit of code with: if (!::InitCommonControlsEx(&iccx)) { DCHECK(false) << "Failed to initialize common controls": return 1; } http://codereview.chromium.org/56076/diff/11004/11005#newcode88 Line 88: hRes; remove void statement http://codereview.chromium.org/56076/diff/11004/11005#newcode89 Line 89: ATLASSERT(SUCCEEDED(hRes)); similarly, could replace the above with: if (FAILED(_Module.Init(NULL, hInstance)) { DCHECK(false) << "Failed to initialize application module"; return 1; } http://codereview.chromium.org/56076/diff/11004/11005#newcode91 Line 91: int nRet = Run(lpstrCmdLine, nCmdShow); nRet -> result
all done. Alpha? http://codereview.chromium.org/56076/diff/11004/11005 File media/player/player_wtl.cc (right): http://codereview.chromium.org/56076/diff/11004/11005#newcode1 Line 1: // Copyright (c) 2006-2009 The Chromium Authors. All rights reserved. On 2009/04/28 18:07:55, scherkus wrote: > year should be 2009 Done. http://codereview.chromium.org/56076/diff/11004/11005#newcode15 Line 15: // Note Headers are order sensitive. On 2009/04/28 18:07:55, scherkus wrote: > nit: Headers -> headers Done. http://codereview.chromium.org/56076/diff/11004/11005#newcode30 Line 30: On 2009/04/28 18:07:55, scherkus wrote: > nit: remove blank line Done. http://codereview.chromium.org/56076/diff/11004/11005#newcode31 Line 31: #include "media/player/movie.h" On 2009/04/28 18:07:55, scherkus wrote: > these headers shouldn't be order sensitive... is it a side effect of having > header files with inlined implementations? > > if so include the "order sensitive" comment here Done. http://codereview.chromium.org/56076/diff/11004/11005#newcode38 Line 38: added comment instead http://codereview.chromium.org/56076/diff/11004/11005#newcode45 Line 45: CAppModule _Module; its used by mainfrm. http://codereview.chromium.org/56076/diff/11004/11005#newcode45 Line 45: CAppModule _Module; On 2009/04/28 18:07:55, scherkus wrote: > _Module -> g_module > > add a comment saying this defines an application instance required by WTL > > is this accessed outside of this file? otherwise an anonymous namespace would > be good Done. http://codereview.chromium.org/56076/diff/11004/11005#newcode47 Line 47: int Run(LPTSTR lpstrCmdLine = NULL, int nCmdShow = SW_SHOWDEFAULT) { On 2009/04/28 18:07:55, scherkus wrote: > LPTSTR -> TCHAR* > lpstrCmdLine -> cmd_line > nCmdShow -> cmd_show > > get rid of the default parameters too Done. http://codereview.chromium.org/56076/diff/11004/11005#newcode48 Line 48: CMessageLoop theLoop; On 2009/04/28 18:07:55, scherkus wrote: > theLoop -> message_loop Done. http://codereview.chromium.org/56076/diff/11004/11005#newcode51 Line 51: CMainFrame wndMain; On 2009/04/28 18:07:55, scherkus wrote: > wndMain -> main_frame (or similar) Done. http://codereview.chromium.org/56076/diff/11004/11005#newcode53 Line 53: ATLTRACE(_T("Main window creation failed!\n")); On 2009/04/28 18:07:55, scherkus wrote: > better to include base/logging.h and use: > DLOG(FATAL) << "Main window creation failed!"; > > or you can assert with: > DCHECK(false) << "Main window creation failed!"; Done. http://codereview.chromium.org/56076/diff/11004/11005#newcode61 Line 61: TCHAR *url = NULL; On 2009/04/28 18:07:55, scherkus wrote: > pointer with type Done. http://codereview.chromium.org/56076/diff/11004/11005#newcode68 Line 68: } unsafe with some compilers. do it anyway? http://codereview.chromium.org/56076/diff/11004/11005#newcode70 Line 70: int nRet = theLoop.Run(); On 2009/04/28 18:07:55, scherkus wrote: > nRet -> result Done. http://codereview.chromium.org/56076/diff/11004/11005#newcode79 Line 79: LPTSTR lpstrCmdLine, int nCmdShow) { On 2009/04/28 18:07:55, scherkus wrote: > hInstance -> instance > hPrevInstance -> prev_instance > lpstrCmdLine -> cmd_line > nCmdShow -> cmd_show > LPTSTR -> TCHAR* Done. http://codereview.chromium.org/56076/diff/11004/11005#newcode84 Line 84: bRet; its to avoid warning on release build. but ok http://codereview.chromium.org/56076/diff/11004/11005#newcode85 Line 85: ATLASSERT(bRet); On 2009/04/28 18:07:55, scherkus wrote: > I would simply replace the above little bit of code with: > if (!::InitCommonControlsEx(&iccx)) { > DCHECK(false) << "Failed to initialize common controls": > return 1; > } > Done. http://codereview.chromium.org/56076/diff/11004/11005#newcode88 Line 88: hRes; On 2009/04/28 18:07:55, scherkus wrote: > remove void statement Done. http://codereview.chromium.org/56076/diff/11004/11005#newcode89 Line 89: ATLASSERT(SUCCEEDED(hRes)); On 2009/04/28 18:07:55, scherkus wrote: > similarly, could replace the above with: > if (FAILED(_Module.Init(NULL, hInstance)) { > DCHECK(false) << "Failed to initialize application module"; > return 1; > } > Done. http://codereview.chromium.org/56076/diff/11004/11005#newcode91 Line 91: int nRet = Run(lpstrCmdLine, nCmdShow); On 2009/04/28 18:07:55, scherkus wrote: > nRet -> result Done.
http://codereview.chromium.org/56076/diff/10007/11006 File media/player/player_wtl.cc (right): http://codereview.chromium.org/56076/diff/10007/11006#newcode65 Line 65: if (url) { the previous if statement also has one statement under the scope but it doesn't have braces, maybe make them consistent.
done. Look good yet? http://codereview.chromium.org/56076/diff/10007/11006 File media/player/player_wtl.cc (right): http://codereview.chromium.org/56076/diff/10007/11006#newcode45 Line 45: CAppModule _Module; changed to g_module http://codereview.chromium.org/56076/diff/10007/11006#newcode65 Line 65: if (url) { On 2009/04/29 02:34:27, Alpha wrote: > the previous if statement also has one statement under the scope but it doesn't > have braces, maybe make them consistent. Done.
look good yet?
LGTM.
more comments http://codereview.chromium.org/56076/diff/11009/10011 File media/player/player_wtl.cc (right): http://codereview.chromium.org/56076/diff/11009/10011#newcode16 Line 16: #include "media/player/stdafx.h" usually stdafx.h is your precompiled header, so it may make sense to move the <atl*.h> headers into there to avoid these tricky header dependencies http://codereview.chromium.org/56076/diff/11009/10011#newcode45 Line 45: CAppModule g_module; given that this is used in other files, it'd be good to add a comment describing its use http://codereview.chromium.org/56076/diff/11009/10011#newcode48 Line 48: CMessageLoop the_loop; nit: the_loop is a bit of a funny name :) most of chrome uses message_loop when referencing message loop objects http://codereview.chromium.org/56076/diff/11009/10011#newcode59 Line 59: base::AtExitManager exit_manager; just realized.. this declaration should actually go as one of the first lines in your main function _tWinMain http://codereview.chromium.org/56076/diff/11009/10011#newcode67 Line 67: wnd_main.MovieOpenFile(url); you never use url again, so you may want to rewrite it as: if (cmd_line && *cmd_line) { wnd_main.MovieOpenFile(*cmd_line); } http://codereview.chromium.org/56076/diff/11009/10011#newcode79 Line 79: wchar_t* cmd_line, int cmd_show) { just a bit of a nit regarding TCHAR vs. wchar_t... TCHAR is either char* or wchar_t* depending on whether the program is compiled with _UNICODE Since we _should_ be a unicode app (using wchar_t) you'll want to replace _tWinMain with wWinMain Take a look at /chrome/app/chrome_exe_main.cc for an example (they also do the AtExitManager stuff) |
