Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1219923011:
Added image pattern comparison logic for test interface and fixture. (Closed)
DescriptionAdded image pattern comparison logic including unit tests.
The Image pattern comparison logic is: calculate the average color of the expected rect region on the current frame and compare it to the expected color to see if it matches.
BUG=
Committed: https://crrev.com/55bcb47addbc50f753487a73fb13ed5e370ae169
Cr-Commit-Position: refs/heads/master@{#338588}
Patch Set 1 : "Added image pattern comparison logic" #
Total comments: 19
Patch Set 2 : "Addressed feedback from Joe and Updated connection helper with base::ResetAndReturn()" #
Total comments: 56
Patch Set 3 : "Addressed feedback from Sergey" #
Total comments: 2
Patch Set 4 : "Minor changes on comments" #
Total comments: 11
Patch Set 5 : "Per discussion with Sergey, removed dependency on the dirty rects, which is not stable. #Patch Set 6 : "Added unit tests for image comparison logic" #
Total comments: 46
Patch Set 7 : "Addressed feedback from Joe and fixed bug on using an empty packet to check pattern is matched" #
Total comments: 16
Patch Set 8 : "Minor update on naming and comments, also add a 10min timer to prevent bugs from hanging the syste… #
Total comments: 17
Patch Set 9 : "Minor update on naming and comments: addressed feedback from Joe and Sergey" #
Total comments: 2
Patch Set 10 : "Minor update on comments" #
Messages
Total messages: 26 (6 generated)
Patchset #1 (id:1) has been deleted
liaoyuke@chromium.org changed reviewers: + anandc@chromium.org, joedow@chromium.org, sergeyu@chromium.org
Hey Joe, Sergey, Anand, Please review a changelist from liaoyuke. Thank you very much! https://codereview.chromium.org/1219923011/diff/20001/remoting/test/test_vide... File remoting/test/test_video_renderer.cc (right): https://codereview.chromium.org/1219923011/diff/20001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:8: #include <cmath> Usage: algorithm: std::min, std::max, cmath: sqrt.
https://codereview.chromium.org/1219923011/diff/20001/remoting/test/test_vide... File remoting/test/test_video_renderer.cc (right): https://codereview.chromium.org/1219923011/diff/20001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:57: RgbaColor ColorEncode(const int red, const int green, const int blue) const; Why are the int values const here? ints are passed by value so I don't think this is very beneficial. https://codereview.chromium.org/1219923011/diff/20001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:117: const double fuzzy_threshold_ = 0.05; Does this need to be a class member or can it just be a const like kBytesPerPixel https://codereview.chromium.org/1219923011/diff/20001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:217: if (!image_pattern_matched_callback_.is_null()) { This be simpler if you return here if the callback is null instead of enclosing this work in the if block if the callback is not null. https://codereview.chromium.org/1219923011/diff/20001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:232: image_pattern_matched_callback_.Reset(); base::ResetAndReturn() can replace lines 231 and 232 (and is a good practice) https://codereview.chromium.org/1219923011/diff/20001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:261: const RgbaColor& rgba_color) const { If you are going to return an array, you want scoped_array<int>, otherwise the wrong delete is going to be called when scoped_ptr goes out of scope. https://codereview.chromium.org/1219923011/diff/20001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:271: bool TestVideoRenderer::Core::ContainsRect( rect1 and rect2 aren't quite descriptive enough, would something like rect1 => candidate_rect, rect2 => template_rect work? https://codereview.chromium.org/1219923011/diff/20001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:275: DCHECK(!image_pattern_matched_callback_.is_null()); Does it matter if the callback is not null here? https://codereview.chromium.org/1219923011/diff/20001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:285: if (accumulating_rect_.width() == 0 && accumulating_rect_.height() == 0) { if (!accumulating_rect_.width() && !accumulating_rect_.height()) no need to compare against 0. Also, is it possible for one of those dimensions to be 0? That seems like it would be odd and possibly be avoided. https://codereview.chromium.org/1219923011/diff/20001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:304: y * kBytesPerPixel * screen_size_.width() + x * kBytesPerPixel; some parens would help with the readability here
Hey Joe, Sergey, Anand, Please take a look at Patch set 2. Thank you very much! https://codereview.chromium.org/1219923011/diff/20001/remoting/test/test_vide... File remoting/test/test_video_renderer.cc (right): https://codereview.chromium.org/1219923011/diff/20001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:57: RgbaColor ColorEncode(const int red, const int green, const int blue) const; On 2015/07/09 03:03:43, joedow wrote: > Why are the int values const here? ints are passed by value so I don't think > this is very beneficial. I think you are right, they shouldn't be const https://codereview.chromium.org/1219923011/diff/20001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:117: const double fuzzy_threshold_ = 0.05; On 2015/07/09 03:03:43, joedow wrote: > Does this need to be a class member or can it just be a const like > kBytesPerPixel Done. https://codereview.chromium.org/1219923011/diff/20001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:217: if (!image_pattern_matched_callback_.is_null()) { On 2015/07/09 03:03:43, joedow wrote: > This be simpler if you return here if the callback is null instead of enclosing > this work in the if block if the callback is not null. Done. https://codereview.chromium.org/1219923011/diff/20001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:232: image_pattern_matched_callback_.Reset(); On 2015/07/09 03:03:43, joedow wrote: > base::ResetAndReturn() can replace lines 231 and 232 (and is a good practice) Done. https://codereview.chromium.org/1219923011/diff/20001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:271: bool TestVideoRenderer::Core::ContainsRect( On 2015/07/09 03:03:43, joedow wrote: > rect1 and rect2 aren't quite descriptive enough, would something like rect1 => > candidate_rect, rect2 => template_rect work? Done. https://codereview.chromium.org/1219923011/diff/20001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:275: DCHECK(!image_pattern_matched_callback_.is_null()); On 2015/07/09 03:03:43, joedow wrote: > Does it matter if the callback is not null here? At first, I thought this function will only be called inside the ProcessVideoPacket, but it seems it's not the case, it may also be used in the debug mode. https://codereview.chromium.org/1219923011/diff/20001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:285: if (accumulating_rect_.width() == 0 && accumulating_rect_.height() == 0) { On 2015/07/09 03:03:43, joedow wrote: > if (!accumulating_rect_.width() && !accumulating_rect_.height()) > > no need to compare against 0. > > Also, is it possible for one of those dimensions to be 0? That seems like it > would be odd and possibly be avoided. I think it's not possible, that's why I use it as an initial state for accumulating_rect, because DesktopRect doesn't have something like is_null(). https://codereview.chromium.org/1219923011/diff/20001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:304: y * kBytesPerPixel * screen_size_.width() + x * kBytesPerPixel; On 2015/07/09 03:03:43, joedow wrote: > some parens would help with the readability here Done. https://codereview.chromium.org/1219923011/diff/20001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:306: green_sum += buffer_->data()[offset + 1]; Here, I should mention that the decoded frame is in BGRA format.
https://codereview.chromium.org/1219923011/diff/40001/remoting/test/app_remot... File remoting/test/app_remoting_latency_test_fixture.h (right): https://codereview.chromium.org/1219923011/diff/40001/remoting/test/app_remot... remoting/test/app_remoting_latency_test_fixture.h:33: typedef uint32 RGBA32; Please don't duplicate type definition. I might be better to just use uint32_t everywhere, unless there are many more places where we need to pass this value. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... File remoting/test/test_video_renderer.cc (right): https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:23: const int kBytesPerPixel = 4; use webrtc::DesktopFrame::kBytesPerPixel. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:26: const double fuzzy_threshold_ = 0.05; Not a proper name for a const. What units is it measured in? It looks like you use this in two places: to compare position of the rect and to compare color. It might be better to have two separate values for these two cases. You don't expect lossy encoding to shift anything around. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:97: scoped_ptr<webrtc::DesktopFrame> buffer_; should this be called frame_? https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:108: RGBColor accumulating_color_; I'm not sure why it's called "accumulating" given that it's average of "accumulating_rect_" https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:221: if (ContainsRect(expected_rect_, dirty_rect)) { I don't get this part. What if dirty_rect is partially within expected_rect? Do we want to ignore it in this case? Or what if dirty_rect covers expected_rect_? It looks to me that what you want to do here is to intersect the dirty_region with a rect that contains expected_rect with margins added on each side https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:226: UpdateAccumulatingColor(); would it be better to return "accumulated_color" from UpdateAccumulatingColor() and pass it to ExpectedImagePatternIsMatched()? accumulated_color_ doesn't need to be class memeber. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:227: if (ExpectedImagePatternIsMatched()) { I don't get the logic here. UpdateAccumulatedColor() calculates average color over accumulated_rect_ and then ExpectedImagePatternIsMatched() verifies that that color matches expected value. But I don't see anything to verify that this color fills the whole expected_rect_. I.e. the code seems to consider patter matched even if we received update for some parts of the target rect. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:229: accumulating_rect_ = webrtc::DesktopRect::MakeLTRB(0, 0, 0, 0); Default constructor creates an empty rect, i.e. this can be webrtc::DesktopRect(); https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:244: bool TestVideoRenderer::Core::ContainsRect( This can be static function, doesn't need to be a method. Also name doesn't make it clear that this function allows some error margins on the edges. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:249: double fuzzy_limit_width = fuzzy_threshold_ * template_rect.width(); Why does this need to be relative to the rect size? Can it be just 1 or 2 pixel in each direction no matter what the size of the rect? https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:252: return candidate_rect.left() >= template_rect.left() - fuzzy_limit_width && I think you can use DesktopRect::ContainsRect(), just need to add the margins to the candidate_rect https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:262: if (!accumulating_rect_.width() && !accumulating_rect_.height()) { accumulating_rect_.is_empty() https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:266: accumulating_rect_ = webrtc::DesktopRect::MakeLTRB( would it be better to store region instead of rect? https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:285: (y * kBytesPerPixel * screen_size_.width()) + (x * kBytesPerPixel); nit: extra parentheses don't add much here https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:285: (y * kBytesPerPixel * screen_size_.width()) + (x * kBytesPerPixel); use buffer_->stride() instead of screen_size_.width(). Lines may not be alligned in the buffer (and sometimes they are in reverse order, i.e. stride() can be negative). https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:285: (y * kBytesPerPixel * screen_size_.width()) + (x * kBytesPerPixel); You can make this code slightly faster by increment offset by 4 every time instead of calculating it for each pixel. Or even better store pointer to current position and increment it instead of indexing into buffer->data() every time. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:300: double red_error = static_cast<double>(expected_color_.GetRed() - I don't think you need explicit cast here https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:300: double red_error = static_cast<double>(expected_color_.GetRed() - Also I don't think that comparing averages is the right thing to do. E.g. if you expect gray (128, 128, 128) and you get a frame that has half of the pixels black (0, 0, 0) and half of them white (255, 255, 255) then you will get average of (128, 128, 128) while the frame clearly doesn't match. You want to calculate total error over the whole rect for each pixel. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:314: // accumulating_rect_ must be valid in order to be matched. nit: |accumulating_rect_| https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... File remoting/test/test_video_renderer.h (right): https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.h:29: typedef uint32 RGBA32; uint32_t . uint32 is a non-standard type used in older code. All new code should use uint32_t. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.h:33: struct RGBColor { this is a class, not struct. https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Structs_vs._... https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.h:36: explicit RGBColor(RGBA32 color) : color_(color) {} Not sure why you need this. Default copy constructor should do the right thing. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.h:40: int GetRed() const; These can be inline, red() green() and blue() See https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.h:45: void SetRGB(int red, int green, int blue); Call it Set() https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.h:49: RGBA32 color_; would it be better to store red green and blue channels separately (e.g. using uint8_t), that would simplify code.
Hey Joe, Sergey, Anand, Please take a look at Patch set 3. Thank you very much! https://codereview.chromium.org/1219923011/diff/40001/remoting/test/app_remot... File remoting/test/app_remoting_latency_test_fixture.h (right): https://codereview.chromium.org/1219923011/diff/40001/remoting/test/app_remot... remoting/test/app_remoting_latency_test_fixture.h:33: typedef uint32 RGBA32; On 2015/07/09 19:43:13, Sergey Ulanov wrote: > Please don't duplicate type definition. I might be better to just use uint32_t > everywhere, unless there are many more places where we need to pass this value. Done. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... File remoting/test/test_video_renderer.cc (right): https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:23: const int kBytesPerPixel = 4; On 2015/07/09 19:43:14, Sergey Ulanov wrote: > use webrtc::DesktopFrame::kBytesPerPixel. Done. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:26: const double fuzzy_threshold_ = 0.05; On 2015/07/09 19:43:14, Sergey Ulanov wrote: > Not a proper name for a const. > > What units is it measured in? > > It looks like you use this in two places: to compare position of the rect and to > compare color. It might be better to have two separate values for these two > cases. You don't expect lossy encoding to shift anything around. I think so, have two separate values looks cleaner. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:59: // Returns true if |rect2| falls within |rect1|, allowing for certain amount Update comments in next patch. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:108: RGBColor accumulating_color_; On 2015/07/09 19:43:14, Sergey Ulanov wrote: > I'm not sure why it's called "accumulating" given that it's average of > "accumulating_rect_" Done. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:221: if (ContainsRect(expected_rect_, dirty_rect)) { On 2015/07/09 19:43:14, Sergey Ulanov wrote: > I don't get this part. What if dirty_rect is partially within expected_rect? Do > we want to ignore it in this case? Or what if dirty_rect covers expected_rect_? > It looks to me that what you want to do here is to intersect the dirty_region > with a rect that contains expected_rect with margins added on each side Yes, it will be ignored in both case, and we only care about those dirty rects that fall within the expected_rect_. The reason is that generated expected_rects will always be the merge result of some dirty rects, which means that if a dirty rect is one that we are interested in, it is ensured to be contained by the expected_rect. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:226: UpdateAccumulatingColor(); On 2015/07/09 19:43:14, Sergey Ulanov wrote: > would it be better to return "accumulated_color" from UpdateAccumulatingColor() > and pass it to ExpectedImagePatternIsMatched()? accumulated_color_ doesn't need > to be class memeber. Yes, I think that's a good idea! https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:227: if (ExpectedImagePatternIsMatched()) { On 2015/07/09 19:43:14, Sergey Ulanov wrote: > I don't get the logic here. UpdateAccumulatedColor() calculates average color > over accumulated_rect_ and then ExpectedImagePatternIsMatched() verifies that > that color matches expected value. But I don't see anything to verify that this > color fills the whole expected_rect_. I.e. the code seems to consider patter > matched even if we received update for some parts of the target rect. Actually, ExpectedImagePatternIsMatched() not only verifies color is matched, but also expected_rect is matched i.e. the sum of the update on rect has covered target rect. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:229: accumulating_rect_ = webrtc::DesktopRect::MakeLTRB(0, 0, 0, 0); On 2015/07/09 19:43:14, Sergey Ulanov wrote: > Default constructor creates an empty rect, i.e. this can be > webrtc::DesktopRect(); Done. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:244: bool TestVideoRenderer::Core::ContainsRect( On 2015/07/09 19:43:14, Sergey Ulanov wrote: > This can be static function, doesn't need to be a method. > > Also name doesn't make it clear that this function allows some error margins on > the edges. Done. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:249: double fuzzy_limit_width = fuzzy_threshold_ * template_rect.width(); On 2015/07/09 19:43:14, Sergey Ulanov wrote: > Why does this need to be relative to the rect size? Can it be just 1 or 2 pixel > in each direction no matter what the size of the rect? I think you are right, since the lossy encoding will not shift anything around, 2 pixels in each direction is fair enough. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:252: return candidate_rect.left() >= template_rect.left() - fuzzy_limit_width && On 2015/07/09 19:43:14, Sergey Ulanov wrote: > I think you can use DesktopRect::ContainsRect(), just need to add the margins to > the candidate_rect Good point! https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:262: if (!accumulating_rect_.width() && !accumulating_rect_.height()) { On 2015/07/09 19:43:15, Sergey Ulanov wrote: > accumulating_rect_.is_empty() Done. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:266: accumulating_rect_ = webrtc::DesktopRect::MakeLTRB( On 2015/07/09 19:43:14, Sergey Ulanov wrote: > would it be better to store region instead of rect? Yes, I think so, using a single rectangle would lose some information, but for now, I haven't come up with a clear solution on how to do comparison on region and how to generate the expected region. rect is easy to deal with, but it might be an important future work to move to region in the future. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:285: (y * kBytesPerPixel * screen_size_.width()) + (x * kBytesPerPixel); On 2015/07/09 19:43:15, Sergey Ulanov wrote: > nit: extra parentheses don't add much here Done. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:285: (y * kBytesPerPixel * screen_size_.width()) + (x * kBytesPerPixel); On 2015/07/09 19:43:14, Sergey Ulanov wrote: > use buffer_->stride() instead of screen_size_.width(). Lines may not be alligned > in the buffer (and sometimes they are in reverse order, i.e. stride() can be > negative). Done. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:285: (y * kBytesPerPixel * screen_size_.width()) + (x * kBytesPerPixel); On 2015/07/09 19:43:14, Sergey Ulanov wrote: > You can make this code slightly faster by increment offset by 4 every time > instead of calculating it for each pixel. Or even better store pointer to > current position and increment it instead of indexing into buffer->data() every > time. Done. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:300: double red_error = static_cast<double>(expected_color_.GetRed() - On 2015/07/09 19:43:14, Sergey Ulanov wrote: > I don't think you need explicit cast here Done. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:300: double red_error = static_cast<double>(expected_color_.GetRed() - On 2015/07/09 19:43:14, Sergey Ulanov wrote: > Also I don't think that comparing averages is the right thing to do. E.g. if you > expect gray (128, 128, 128) and you get a frame that has half of the pixels > black (0, 0, 0) and half of them white (255, 255, 255) then you will get average > of (128, 128, 128) while the frame clearly doesn't match. You want to calculate > total error over the whole rect for each pixel. Yes, you are right, there are some potential problems with the average color. But if we are going to calculate total error, then to create a test case, the input of this test case would be a rectangle with detailed pixel information, which would be painful for the user. I believe there are better solutions, but for now, I believe this simple metric should work for most cases, and in the future, more features can be added to improve the precision and robustness. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:314: // accumulating_rect_ must be valid in order to be matched. On 2015/07/09 19:43:14, Sergey Ulanov wrote: > nit: |accumulating_rect_| Done. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... File remoting/test/test_video_renderer.h (right): https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.h:29: typedef uint32 RGBA32; On 2015/07/09 19:43:15, Sergey Ulanov wrote: > uint32_t . uint32 is a non-standard type used in older code. All new code should > use uint32_t. Done. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.h:33: struct RGBColor { On 2015/07/09 19:43:15, Sergey Ulanov wrote: > this is a class, not struct. > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Structs_vs._... Done. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.h:36: explicit RGBColor(RGBA32 color) : color_(color) {} On 2015/07/09 19:43:15, Sergey Ulanov wrote: > Not sure why you need this. Default copy constructor should do the right thing. I have this default constructor only because I don't want to initialize it, but you are right, I should delete this. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.h:40: int GetRed() const; On 2015/07/09 19:43:15, Sergey Ulanov wrote: > These can be inline, red() green() and blue() > See > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names Done. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.h:45: void SetRGB(int red, int green, int blue); On 2015/07/09 19:43:15, Sergey Ulanov wrote: > Call it Set() Done. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.h:49: RGBA32 color_; On 2015/07/09 19:43:15, Sergey Ulanov wrote: > would it be better to store red green and blue channels separately (e.g. using > uint8_t), that would simplify code. Yeah, I think it would simplify the code and be more efficient since GetRed, GetGreen, GetBlue functions are frequently called.
https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... File remoting/test/test_video_renderer.cc (right): https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:221: if (ContainsRect(expected_rect_, dirty_rect)) { On 2015/07/09 23:21:01, liaoyuke wrote: > On 2015/07/09 19:43:14, Sergey Ulanov wrote: > > I don't get this part. What if dirty_rect is partially within expected_rect? > Do > > we want to ignore it in this case? Or what if dirty_rect covers > expected_rect_? > > It looks to me that what you want to do here is to intersect the dirty_region > > with a rect that contains expected_rect with margins added on each side > > Yes, it will be ignored in both case, and we only care about those dirty rects > that fall within the expected_rect_. The reason is that generated expected_rects > will always be the merge result of some dirty rects, which means that if a dirty > rect is one that we are interested in, it is ensured to be contained by the > expected_rect. I still don't see how this can work correctly. Let's say you have expected_rect=(x=100, y=100, w=100, h=100) and you receive a rectangle that covers the whole screen, e.g. (x=0, y=0, w=1024, h=768). This code will ignore that updated, but it shouldn't. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:227: if (ExpectedImagePatternIsMatched()) { On 2015/07/09 23:21:01, liaoyuke wrote: > On 2015/07/09 19:43:14, Sergey Ulanov wrote: > > I don't get the logic here. UpdateAccumulatedColor() calculates average color > > over accumulated_rect_ and then ExpectedImagePatternIsMatched() verifies that > > that color matches expected value. But I don't see anything to verify that > this > > color fills the whole expected_rect_. I.e. the code seems to consider patter > > matched even if we received update for some parts of the target rect. > > Actually, ExpectedImagePatternIsMatched() not only verifies color is matched, > but also expected_rect is matched i.e. the sum of the update on rect has covered > target rect. Missed that sorry. But I still think it isn't correct. Lets say you have expected_rect=(x=100, y=100, w=100, h=100) and you receive two updates one for (x=100, y=100, w=50, h=50) and another one for (x=150, y=150, w=50, h=50). You'll get accumulated_rect_ to contain the whole expected_rect, while you've received data only for a half of it. Here is an approach that I think would work better: 1. Dump whole dirty_region() to accumulated_region_. 2. Verify that accumulated_region_ contains the whole expected_rect_, maybe excluding 1 or 2 pixel from the edges. This is easy implement by contracting expected_rect_by the margin on each side and intersecting with accumulated_region_. i.e. something like: ContractRect(expected_rect, margin); DesktopRegion intersection; intersection.Intersect(accumulated_region_, DesktopRegion(expected_rect)); bool rect_covered = DesktopRegion(expected_rect).Equals(intersection); 3. If the whole rect is covered by accumulated_region_ compare colors in each pixel. https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:266: accumulating_rect_ = webrtc::DesktopRect::MakeLTRB( On 2015/07/09 23:21:01, liaoyuke wrote: > On 2015/07/09 19:43:14, Sergey Ulanov wrote: > > would it be better to store region instead of rect? > > Yes, I think so, using a single rectangle would lose some information, but for > now, I haven't come up with a clear solution on how to do comparison on region > and how to generate the expected region. rect is easy to deal with, but it might > be an important future work to move to region in the future. See my comment above for my suggestions https://codereview.chromium.org/1219923011/diff/40001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:300: double red_error = static_cast<double>(expected_color_.GetRed() - On 2015/07/09 23:21:01, liaoyuke wrote: > On 2015/07/09 19:43:14, Sergey Ulanov wrote: > > Also I don't think that comparing averages is the right thing to do. E.g. if > you > > expect gray (128, 128, 128) and you get a frame that has half of the pixels > > black (0, 0, 0) and half of them white (255, 255, 255) then you will get > average > > of (128, 128, 128) while the frame clearly doesn't match. You want to > calculate > > total error over the whole rect for each pixel. > > Yes, you are right, there are some potential problems with the average color. > But if we are going to calculate total error, then to create a test case, the > input of this test case would be a rectangle with detailed pixel information, > which would be painful for the user. I don't think you would need to change the interface for that. This code would still get a rectangle and color, but instead of calculating the average and comparing it to the expected color it should compare color of each pixel to the expected values. > I believe there are better solutions, but > for now, I believe this simple metric should work for most cases, and in the > future, more features can be added to improve the precision and robustness.
https://codereview.chromium.org/1219923011/diff/80001/remoting/test/test_vide... File remoting/test/test_video_renderer.cc (right): https://codereview.chromium.org/1219923011/diff/80001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:26: int red_; don't need _ suffix for struct fields. It's only for class fields https://codereview.chromium.org/1219923011/diff/80001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:33: const int kRectFuzzyThreshold = 2; Maybe call it something like kMaxPositionError? Document this is in pixels https://codereview.chromium.org/1219923011/diff/80001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:34: const double kColorFuzzyThreshold = 0.05; kMaxColorError? Also document units. https://codereview.chromium.org/1219923011/diff/80001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:313: uint8_t* ptr = |ptr| is not the best name. https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Nami... . Something like |position| or |buffer_pos| would be better https://codereview.chromium.org/1219923011/diff/80001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:319: red_sum += *(ptr + 2); These can be ptr[2], ptr[1] and ptr[0]
Hey Joe, Sergey, Anand, I have get rid of the dependency on dirty rects, which is not stable. And now the logic is simplified to: Calculate the average color of the expected rect region on the current frame and compare it to the expected color to see if it matches. Please take a look at patch set 5. Thank you very much! https://codereview.chromium.org/1219923011/diff/60001/remoting/test/test_vide... File remoting/test/test_video_renderer.cc (left): https://codereview.chromium.org/1219923011/diff/60001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:46: Add comments in next patch. https://codereview.chromium.org/1219923011/diff/60001/remoting/test/test_vide... File remoting/test/test_video_renderer.cc (right): https://codereview.chromium.org/1219923011/diff/60001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:142: accumulating_rect_ = webrtc::DesktopRect::MakeLTRB(0, 0, 0, 0); Change to DesktopRect() in next patch. https://codereview.chromium.org/1219923011/diff/80001/remoting/test/test_vide... File remoting/test/test_video_renderer.cc (left): https://codereview.chromium.org/1219923011/diff/80001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:20: Per discussion with Sergey, the dirty rects are not that stable and the method should not rely on dirty rects. To get rid of the dependency on dirty rects, I'll change the comparison logic to every time when a frame is received, calculate the average color of pixels fall within the expected rect and see if it matches. This is more computationally intensive, but it's more stable. https://codereview.chromium.org/1219923011/diff/80001/remoting/test/test_vide... File remoting/test/test_video_renderer.cc (right): https://codereview.chromium.org/1219923011/diff/80001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:26: int red_; On 2015/07/10 01:21:56, Sergey Ulanov wrote: > don't need _ suffix for struct fields. It's only for class fields Done. https://codereview.chromium.org/1219923011/diff/80001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:33: const int kRectFuzzyThreshold = 2; On 2015/07/10 01:21:56, Sergey Ulanov wrote: > Maybe call it something like kMaxPositionError? Document this is in pixels Done. https://codereview.chromium.org/1219923011/diff/80001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:34: const double kColorFuzzyThreshold = 0.05; On 2015/07/10 01:21:55, Sergey Ulanov wrote: > kMaxColorError? > Also document units. Done. https://codereview.chromium.org/1219923011/diff/80001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:313: uint8_t* ptr = On 2015/07/10 01:21:56, Sergey Ulanov wrote: > |ptr| is not the best name. > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Nami... > . Something like |position| or |buffer_pos| would be better Done. https://codereview.chromium.org/1219923011/diff/80001/remoting/test/test_vide... remoting/test/test_video_renderer.cc:319: red_sum += *(ptr + 2); On 2015/07/10 01:21:55, Sergey Ulanov wrote: > These can be ptr[2], ptr[1] and ptr[0] Done.
Hey Joe, Sergey, Anand, I have added unit tests for the image pattern comparison logc. They mainly cover two cases: true positive: correct pattern should be matched. true negative: incorrect pattern should not be matched. Please take a look at patch set 6 when you have time. Thank you very much!
https://codereview.chromium.org/1219923011/diff/120001/remoting/test/app_remo... File remoting/test/app_remoting_latency_test_fixture.cc (right): https://codereview.chromium.org/1219923011/diff/120001/remoting/test/app_remo... remoting/test/app_remoting_latency_test_fixture.cc:55: DCHECK(thread_checker_.CalledOnValidThread()); remove const https://codereview.chromium.org/1219923011/diff/120001/remoting/test/app_remo... File remoting/test/app_remoting_latency_test_fixture.h (right): https://codereview.chromium.org/1219923011/diff/120001/remoting/test/app_remo... remoting/test/app_remoting_latency_test_fixture.h:47: const uint32_t expected_avg_color); why is the uint32 marked const? It is passed by value... https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... File remoting/test/test_video_renderer.cc (right): https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:21: // RGB Color triplets, and it can be converted to and from RGBA32. This comment is a little confusing, I don't think you need the struct name in the comment and also, how does the conversion occur (casting?) https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:22: struct RGBTriplets { Would RgbValue be a better name here? If we care about alpha in the future, I'm assuming we wouldn't have an RGBAQuartet (vs. RgbaValue) https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:27: }; This struct should be in the anonymous namespace below https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:31: // in percent. nit: Can you condense this comment so it fits on one line? https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:34: no need for an end "// namespace" comment when the namespace block is so short (pretty sure you shouldn't get a lint error) https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:61: const RGBA32& expected_avg_color, Does this need to be a const ref since it is just a 32bit uint? https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:68: static RGBTriplets ConvertRGBA32ToTriplets(RGBA32 color); Do these need to be static or can they live in the anonymous namespace above (with the RGBTriplets struct) https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:216: LOG(INFO) << accumulating_avg_triplets.red << " " LOG(INFO) is pretty chatty, can this be a VLOG(2)? https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:219: if (ExpectedColorIsMatched(accumulating_avg_triplets, kMaxColorError)) { Does kMaxColorError need to be passed? If you always use a constant, can you just use that constant in the function and remove the second param? https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:227: const RGBA32& expected_avg_color, Remove const? https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:379: const RGBA32& expected_avg_color, remove const? https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... File remoting/test/test_video_renderer.h (right): https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer.h:53: scoped_ptr<webrtc::DesktopFrame> GetFrameForTest() const; nit: GetCurrentFrameForTest? https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... File remoting/test/test_video_renderer_unittest.cc (right): https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:26: void VideoPacketDoneHandler(const base::Closure& done_closure, 'VideoPacketDoneHandler' isn't very descriptive, would something like 'EmptyVideoPacketDoneHandler' or 'NeverMatchVideoPacketDoneHandler' be clearer? https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:34: const int kDefaultScreenHeight = 768; nit: You can remove the comment if you change the name of the variables to kDefaultScreenWidthPx and kDefaultScreenHeightPx https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:68: // image can be matched and corresponding callback can be fired. These comments are better suited for the definition of the method (i.e. where you want context of how since you are reading through the code). The documenting comment should be simpler and be something like "Handles setting a pattern and sending a frame which are expected to be matched by the TVR." https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:112: RGBA32 CalculateAverageColorTripletsForFrame( CalculateAverageRgbColorForFrame https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:178: EXPECT_TRUE(frame); I don't think you need EXPECT_TRUE since it is a helper method the test owns. If this failed (frame were null) it would be a test bug that you'd find very quickly. https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:189: expected_color, run_loop_->QuitClosure())); Why are you posting a task here instead of just calling the function on the test_video_renderer_ pointer that have? Internally the TVR is just going to post the task to its decode thread so I'm not sure what posting here as well is doing for you. https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:200: // Wait for image pattern matched reply to be fired. If there is a product bug, then the MessageLoop could run forever. I think you also want a timer here with a long max timeout (since you saw enourmously long decode times on certain machines) so that you can ensure this test doesn't block a run if a bug is introduced. https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:249: // the callback has been fired or not. Posting an empty packet will just cause your done callback to be called on the current thread right? It won't get posted to the decode thread's messageloop https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:460: RGBA32 expected_color = 0xFF000000; nit: I'd change the var name to reflect the color you've chosen since you know exactly what it is.
Hey Joe, Sergey, Anand, I have addressed feedback from Joe and fixed bugs on using an empty packet to check if the expected image pattern is matched. Please take a look at patch set 7. Thank you very much! https://codereview.chromium.org/1219923011/diff/120001/remoting/test/app_remo... File remoting/test/app_remoting_latency_test_fixture.cc (right): https://codereview.chromium.org/1219923011/diff/120001/remoting/test/app_remo... remoting/test/app_remoting_latency_test_fixture.cc:55: DCHECK(thread_checker_.CalledOnValidThread()); On 2015/07/10 23:39:15, joedow wrote: > remove const Done. https://codereview.chromium.org/1219923011/diff/120001/remoting/test/app_remo... File remoting/test/app_remoting_latency_test_fixture.h (right): https://codereview.chromium.org/1219923011/diff/120001/remoting/test/app_remo... remoting/test/app_remoting_latency_test_fixture.h:47: const uint32_t expected_avg_color); On 2015/07/10 23:39:15, joedow wrote: > why is the uint32 marked const? It is passed by value... Done. https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... File remoting/test/test_video_renderer.cc (right): https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:21: // RGB Color triplets, and it can be converted to and from RGBA32. On 2015/07/10 23:39:16, joedow wrote: > This comment is a little confusing, I don't think you need the struct name in > the comment and also, how does the conversion occur (casting?) Acknowledged. https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:22: struct RGBTriplets { On 2015/07/10 23:39:15, joedow wrote: > Would RgbValue be a better name here? If we care about alpha in the future, I'm > assuming we wouldn't have an RGBAQuartet (vs. RgbaValue) Yes, and maybe I should move the convertion functions to here. https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:27: }; On 2015/07/10 23:39:16, joedow wrote: > This struct should be in the anonymous namespace below Done. https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:31: // in percent. On 2015/07/10 23:39:16, joedow wrote: > nit: Can you condense this comment so it fits on one line? Done. https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:34: On 2015/07/10 23:39:15, joedow wrote: > no need for an end "// namespace" comment when the namespace block is so short > (pretty sure you shouldn't get a lint error) I actually do get a lint error: "Anonymous namespace should be terminated with "// namespace" [readability/namespace] [5]" https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:61: const RGBA32& expected_avg_color, On 2015/07/10 23:39:16, joedow wrote: > Does this need to be a const ref since it is just a 32bit uint? Done. https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:68: static RGBTriplets ConvertRGBA32ToTriplets(RGBA32 color); On 2015/07/10 23:39:15, joedow wrote: > Do these need to be static or can they live in the anonymous namespace above > (with the RGBTriplets struct) Yes, I think so, since they are only used with RGBValue struct. https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:216: LOG(INFO) << accumulating_avg_triplets.red << " " On 2015/07/10 23:39:16, joedow wrote: > LOG(INFO) is pretty chatty, can this be a VLOG(2)? Done. https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:219: if (ExpectedColorIsMatched(accumulating_avg_triplets, kMaxColorError)) { On 2015/07/10 23:39:15, joedow wrote: > Does kMaxColorError need to be passed? If you always use a constant, can you > just use that constant in the function and remove the second param? Done. https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:227: const RGBA32& expected_avg_color, On 2015/07/10 23:39:16, joedow wrote: > Remove const? Done. https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:379: const RGBA32& expected_avg_color, On 2015/07/10 23:39:16, joedow wrote: > remove const? Done. https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... File remoting/test/test_video_renderer.h (right): https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer.h:53: scoped_ptr<webrtc::DesktopFrame> GetFrameForTest() const; On 2015/07/10 23:39:16, joedow wrote: > nit: GetCurrentFrameForTest? Done. https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... File remoting/test/test_video_renderer_unittest.cc (right): https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:26: void VideoPacketDoneHandler(const base::Closure& done_closure, On 2015/07/10 23:39:16, joedow wrote: > 'VideoPacketDoneHandler' isn't very descriptive, would something like > 'EmptyVideoPacketDoneHandler' or 'NeverMatchVideoPacketDoneHandler' be clearer? I have changed the mechanism: in both Match and NotMatch test case, we first send the test packet and then a copy of the test packet with a done handler is sent to the TVR to see if the run_loop_->QuitClosure() has been fired, which indicates if the expected pattern has been matched. So, I changed the name to 'PatternMatchedVerifyPacketDoneHandler'. https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:34: const int kDefaultScreenHeight = 768; On 2015/07/10 23:39:16, joedow wrote: > nit: You can remove the comment if you change the name of the variables to > kDefaultScreenWidthPx and kDefaultScreenHeightPx Done. https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:68: // image can be matched and corresponding callback can be fired. On 2015/07/10 23:39:16, joedow wrote: > These comments are better suited for the definition of the method (i.e. where > you want context of how since you are reading through the code). The > documenting comment should be simpler and be something like "Handles setting a > pattern and sending a frame which are expected to be matched by the TVR." Done. https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:112: RGBA32 CalculateAverageColorTripletsForFrame( On 2015/07/10 23:39:16, joedow wrote: > CalculateAverageRgbColorForFrame Done. https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:178: EXPECT_TRUE(frame); On 2015/07/10 23:39:16, joedow wrote: > I don't think you need EXPECT_TRUE since it is a helper method the test owns. > If this failed (frame were null) it would be a test bug that you'd find very > quickly. Done. https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:189: expected_color, run_loop_->QuitClosure())); On 2015/07/10 23:39:16, joedow wrote: > Why are you posting a task here instead of just calling the function on the > test_video_renderer_ pointer that have? Internally the TVR is just going to > post the task to its decode thread so I'm not sure what posting here as well is > doing for you. Yes, you are right, they are basically doing the same thing. https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:200: // Wait for image pattern matched reply to be fired. On 2015/07/10 23:39:16, joedow wrote: > If there is a product bug, then the MessageLoop could run forever. I think you > also want a timer here with a long max timeout (since you saw enourmously long > decode times on certain machines) so that you can ensure this test doesn't block > a run if a bug is introduced. the new mechanism send a copy packet to check if expected pattern is matched in both Match and NotMatch case. https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:249: // the callback has been fired or not. On 2015/07/10 23:39:16, joedow wrote: > Posting an empty packet will just cause your done callback to be called on the > current thread right? It won't get posted to the decode thread's messageloop You are right, there is a bug. The new mechanism is to send a copy of the test packet instead of an empty one. https://codereview.chromium.org/1219923011/diff/120001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:460: RGBA32 expected_color = 0xFF000000; On 2015/07/10 23:39:16, joedow wrote: > nit: I'd change the var name to reflect the color you've chosen since you know > exactly what it is. Done.
https://codereview.chromium.org/1219923011/diff/140001/remoting/test/test_vid... File remoting/test/test_video_renderer.cc (right): https://codereview.chromium.org/1219923011/diff/140001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:32: RGBValue ConvertRGBA32Tovalue(uint32_t color) { Why a uint32_t instead of an RGBA32 here? I'm actually thinking that the RGBA32 typedef doesn't provide much value since you have the name struct above. I'd be consistent in its usage but wondering if you should just use uint32_t. https://codereview.chromium.org/1219923011/diff/140001/remoting/test/test_vid... File remoting/test/test_video_renderer_unittest.cc (right): https://codereview.chromium.org/1219923011/diff/140001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:68: const webrtc::DesktopRect& expected_rect); I think the TestImagePattern*Match functions arte pretty similar, would it make sense to combine them and use a boolean to determine whether you expect them to match or not? https://codereview.chromium.org/1219923011/diff/140001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:103: // return the mean error of two frames over all pixels, where error at each nit: capitalize 'r' in return. https://codereview.chromium.org/1219923011/diff/140001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:138: // Generate a frame containing a gradient nit: add period to end of comment. https://codereview.chromium.org/1219923011/diff/140001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:157: // The original frame is compared to the decoded video frame to checks that nit: s/checks/check https://codereview.chromium.org/1219923011/diff/140001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:158: // the mean error over all pixels do not exceed a given limit. nit: s/do/does https://codereview.chromium.org/1219923011/diff/140001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:183: bool image_pattern_is_matched = true; This is really confusing, you are setting the image matched boolean to true, however it gets set to false if the image match callback is fired? https://codereview.chromium.org/1219923011/diff/140001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:184: base::Closure pattern_matched_verify_packet_done_callback = This is a pretty long name, I think you can just call this 'pattern_matched_callback' or something shorter. It is pretty clear what it is being used for in this context.
Hey Joe, Sergey, Anand, I have some minor update on naming and comments, also added a ridiculously long 10min timer to prevent the code bugs from hanging the system. https://codereview.chromium.org/1219923011/diff/140001/remoting/test/test_vid... File remoting/test/test_video_renderer.cc (right): https://codereview.chromium.org/1219923011/diff/140001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:32: RGBValue ConvertRGBA32Tovalue(uint32_t color) { On 2015/07/13 16:27:51, joedow wrote: > Why a uint32_t instead of an RGBA32 here? I'm actually thinking that the RGBA32 > typedef doesn't provide much value since you have the name struct above. I'd be > consistent in its usage but wondering if you should just use uint32_t. Now I remember the reason why I use uint32_t here is that I declared RGBA32 in remoting::test, and I don't want to use remoting::test::RGBA32, which is too long. I think you are right, the typedef doesn't provide much value here, and since I will also use uint32_t(RGBA32) elsewhere, maybe it's better to consistently use uint32_t. https://codereview.chromium.org/1219923011/diff/140001/remoting/test/test_vid... File remoting/test/test_video_renderer_unittest.cc (right): https://codereview.chromium.org/1219923011/diff/140001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:68: const webrtc::DesktopRect& expected_rect); On 2015/07/13 16:27:51, joedow wrote: > I think the TestImagePattern*Match functions arte pretty similar, would it make > sense to combine them and use a boolean to determine whether you expect them to > match or not? Done. https://codereview.chromium.org/1219923011/diff/140001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:103: // return the mean error of two frames over all pixels, where error at each On 2015/07/13 16:27:51, joedow wrote: > nit: capitalize 'r' in return. Done. https://codereview.chromium.org/1219923011/diff/140001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:138: // Generate a frame containing a gradient On 2015/07/13 16:27:51, joedow wrote: > nit: add period to end of comment. Done. https://codereview.chromium.org/1219923011/diff/140001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:157: // The original frame is compared to the decoded video frame to checks that On 2015/07/13 16:27:51, joedow wrote: > nit: s/checks/check Done. https://codereview.chromium.org/1219923011/diff/140001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:158: // the mean error over all pixels do not exceed a given limit. On 2015/07/13 16:27:51, joedow wrote: > nit: s/do/does Done. https://codereview.chromium.org/1219923011/diff/140001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:183: bool image_pattern_is_matched = true; On 2015/07/13 16:27:51, joedow wrote: > This is really confusing, you are setting the image matched boolean to true, > however it gets set to false if the image match callback is fired? Done. https://codereview.chromium.org/1219923011/diff/140001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:184: base::Closure pattern_matched_verify_packet_done_callback = On 2015/07/13 16:27:51, joedow wrote: > This is a pretty long name, I think you can just call this > 'pattern_matched_callback' or something shorter. It is pretty clear what it is > being used for in this context. Done.
https://codereview.chromium.org/1219923011/diff/160001/remoting/test/test_vid... File remoting/test/test_video_renderer.cc (right): https://codereview.chromium.org/1219923011/diff/160001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:25: RGBValue(int r, int g, int b) : red(r), green(g), blue(b) {} nit: newline between c'tor and members would be good here. https://codereview.chromium.org/1219923011/diff/160001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:31: // Convert a RGBA32 to a RGBValue. nit: This now converts a uint32_t to RgbValue since you do not have RGBA32s anymore. Can you fix your comments/function names up? https://codereview.chromium.org/1219923011/diff/160001/remoting/test/test_vid... File remoting/test/test_video_renderer_unittest.cc (right): https://codereview.chromium.org/1219923011/diff/160001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:26: void SecondPacketDoneHandler(const base::Closure& done_closure, I would remove the 'Second' string from the name of the function and parameter as it really make's this specific to a test case (if you wanted to use this function to wait until 5 frames had been processed, you'd use the same function here but the name would be wonky). Maybe just call it ProcessPacketDoneHandler (with a bool arg called handler_called) https://codereview.chromium.org/1219923011/diff/160001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:143: // Set an extremely long time: 10min to prevent bugs from hanging the system. nit: Can you describe why it isn't shorter, in the future someone may come along and decide this is too long, it is good to give them more information on why you didn't choose a shorter value initially. https://codereview.chromium.org/1219923011/diff/160001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:174: // Set an extremely long time: 10min to prevent bugs from hanging the system. same here, a comment would be good to help prevent future engineers from choosing a time that is too short.
https://codereview.chromium.org/1219923011/diff/160001/remoting/test/test_vid... File remoting/test/test_video_renderer.cc (right): https://codereview.chromium.org/1219923011/diff/160001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:72: RGBValue CalculateAverageColorvalue(const webrtc::DesktopRect& rect) const; s/value/Value/ or just remove "value" https://codereview.chromium.org/1219923011/diff/160001/remoting/test/test_vid... File remoting/test/test_video_renderer.h (right): https://codereview.chromium.org/1219923011/diff/160001/remoting/test/test_vid... remoting/test/test_video_renderer.h:60: void SetImagePatternAndMatchedCallback( Maybe call this differently to reflect actual behavior. 1. there is no pattern to match, it just compares average color 2. the name implies that it just sets the callback, while it also sets expected color and rect. Maybe something like ExpectAverageColorInRect() might be better. BTW, not for this CL: it would be a good idea to move matching code out of this class, to make it possible to use different matching algorithms without any changes using the same TestVideoRenderer. So PatternMatcher would be just a listener for incoming video frames and calling the callback, and then the threading logic in TestVideoRenderer would be isolated from pattern matching. That would also make your unittests simpler. https://codereview.chromium.org/1219923011/diff/160001/remoting/test/test_vid... remoting/test/test_video_renderer.h:62: uint32_t expected_color, call this expected_average_color and document it in the comments. The comment says "pattern is matched" while there is no actual pattern matching happening.
Patchset #9 (id:180001) has been deleted
Hey Joe, Sergey, Anand, Please take a look at patch set 9. Thank you very much! https://codereview.chromium.org/1219923011/diff/160001/remoting/test/test_vid... File remoting/test/test_video_renderer.cc (right): https://codereview.chromium.org/1219923011/diff/160001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:25: RGBValue(int r, int g, int b) : red(r), green(g), blue(b) {} On 2015/07/13 19:30:03, joedow wrote: > nit: newline between c'tor and members would be good here. Done. https://codereview.chromium.org/1219923011/diff/160001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:31: // Convert a RGBA32 to a RGBValue. On 2015/07/13 19:30:03, joedow wrote: > nit: This now converts a uint32_t to RgbValue since you do not have RGBA32s > anymore. Can you fix your comments/function names up? Done. https://codereview.chromium.org/1219923011/diff/160001/remoting/test/test_vid... remoting/test/test_video_renderer.cc:72: RGBValue CalculateAverageColorvalue(const webrtc::DesktopRect& rect) const; On 2015/07/13 19:42:18, Sergey Ulanov wrote: > s/value/Value/ > or just remove "value" Done. https://codereview.chromium.org/1219923011/diff/160001/remoting/test/test_vid... File remoting/test/test_video_renderer.h (right): https://codereview.chromium.org/1219923011/diff/160001/remoting/test/test_vid... remoting/test/test_video_renderer.h:60: void SetImagePatternAndMatchedCallback( On 2015/07/13 19:42:18, Sergey Ulanov wrote: > Maybe call this differently to reflect actual behavior. > 1. there is no pattern to match, it just compares average color > 2. the name implies that it just sets the callback, while it also sets expected > color and rect. > > Maybe something like ExpectAverageColorInRect() might be better. > > BTW, not for this CL: it would be a good idea to move matching code out of this > class, to make it possible to use different matching algorithms without any > changes using the same TestVideoRenderer. So PatternMatcher would be just a > listener for incoming video frames and calling the callback, and then the > threading logic in TestVideoRenderer would be isolated from pattern matching. > That would also make your unittests simpler. Totally agree with your idea to move matching code out, and I will refactor the code when I got chance. https://codereview.chromium.org/1219923011/diff/160001/remoting/test/test_vid... remoting/test/test_video_renderer.h:60: void SetImagePatternAndMatchedCallback( On 2015/07/13 19:42:18, Sergey Ulanov wrote: > Maybe call this differently to reflect actual behavior. > 1. there is no pattern to match, it just compares average color > 2. the name implies that it just sets the callback, while it also sets expected > color and rect. > > Maybe something like ExpectAverageColorInRect() might be better. > > BTW, not for this CL: it would be a good idea to move matching code out of this > class, to make it possible to use different matching algorithms without any > changes using the same TestVideoRenderer. So PatternMatcher would be just a > listener for incoming video frames and calling the callback, and then the > threading logic in TestVideoRenderer would be isolated from pattern matching. > That would also make your unittests simpler. Done. https://codereview.chromium.org/1219923011/diff/160001/remoting/test/test_vid... remoting/test/test_video_renderer.h:62: uint32_t expected_color, On 2015/07/13 19:42:18, Sergey Ulanov wrote: > call this expected_average_color and document it in the comments. The comment > says "pattern is matched" while there is no actual pattern matching happening. Done. https://codereview.chromium.org/1219923011/diff/160001/remoting/test/test_vid... File remoting/test/test_video_renderer_unittest.cc (right): https://codereview.chromium.org/1219923011/diff/160001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:26: void SecondPacketDoneHandler(const base::Closure& done_closure, On 2015/07/13 19:30:03, joedow wrote: > I would remove the 'Second' string from the name of the function and parameter > as it really make's this specific to a test case (if you wanted to use this > function to wait until 5 frames had been processed, you'd use the same function > here but the name would be wonky). Maybe just call it ProcessPacketDoneHandler > (with a bool arg called handler_called) Yes, this should be more general. https://codereview.chromium.org/1219923011/diff/160001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:143: // Set an extremely long time: 10min to prevent bugs from hanging the system. On 2015/07/13 19:30:03, joedow wrote: > nit: Can you describe why it isn't shorter, in the future someone may come along > and decide this is too long, it is good to give them more information on why you > didn't choose a shorter value initially. Done. https://codereview.chromium.org/1219923011/diff/160001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:174: // Set an extremely long time: 10min to prevent bugs from hanging the system. On 2015/07/13 19:30:03, joedow wrote: > same here, a comment would be good to help prevent future engineers from > choosing a time that is too short. Done.
lgtm https://codereview.chromium.org/1219923011/diff/200001/remoting/test/test_vid... File remoting/test/test_video_renderer_unittest.cc (right): https://codereview.chromium.org/1219923011/diff/200001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:145: // extremely long time as 10 min is chosen to void being variable/flaky. nit: s/void/avoid https://codereview.chromium.org/1219923011/diff/200001/remoting/test/test_vid... remoting/test/test_video_renderer_unittest.cc:178: // extremely long time as 10 min is chosen to void being variable/flaky. nit: s/void/avoid
Patchset #10 (id:220001) has been deleted
The CQ bit was checked by liaoyuke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from joedow@chromium.org Link to the patchset: https://codereview.chromium.org/1219923011/#ps240001 (title: ""Minor update on comments"")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1219923011/240001
Message was sent while issue was closed.
Committed patchset #10 (id:240001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/55bcb47addbc50f753487a73fb13ed5e370ae169 Cr-Commit-Position: refs/heads/master@{#338588} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
