Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
108333004:
[telemetry] Implement per-pixel algorithms in Bitmap as a C++ extension. (Closed)
|
Created:
7 years ago by szym Modified:
7 years ago CC:
chromium-reviews, chrome-speed-team+watch_google.com, telemetry+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Description[telemetry] Implement per-pixel algorithms in Bitmap as a C++ extension.
BUG=323813
TEST=telemetry bitmap_unittest
R=bulach@chromium.org, tonyg@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241066
Patch Set 1 : . #Patch Set 2 : merge ColorCount into BoundingBox #Patch Set 3 : update StopVideoCapture #
Total comments: 16
Patch Set 4 : fix Equal to handle width crop #Patch Set 5 : added comment that IsEqual does not compare alpha #
Total comments: 19
Patch Set 6 : old Crop interface; add copyright #
Total comments: 6
Patch Set 7 : Build bitmaptools in a temp directory #Patch Set 8 : workaround for distutils with 64-bit msvc9 #Patch Set 9 : import _winreg #
Total comments: 4
Patch Set 10 : style; simplify #
Total comments: 2
Patch Set 11 : use framework and sdk afterall #Patch Set 12 : shorter workaround #Patch Set 13 : another attempt #Patch Set 14 : Refactor. Always try to import bitmaptools. #Patch Set 15 : new assert #Patch Set 16 : add faster IsEqual for zero-tolerance, bpp=3 comparison #Patch Set 17 : delint #
Messages
Total messages: 34 (0 generated)
very nice indeed!! although I'm a bit familiar with JNI, this the first time I'm seeing a python module.. looks very similar, thanks for teaching me! :) some nits and suggestions, feel free to go ahead once tony is happy.. https://codereview.chromium.org/108333004/diff/70001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/bitmaptools/bitmaptools.cc (right): https://codereview.chromium.org/108333004/diff/70001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/bitmaptools/bitmaptools.cc:101: int total_size, row_size; // in bytes nit: maybe zero-initialize these in the constructor list? also, I think we normally use one per line, without commas. https://codereview.chromium.org/108333004/diff/70001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/bitmaptools/bitmaptools.cc:106: PyObject* Histogram(PyObject *self, PyObject *bmp_object) { style nit: move the "*" to the type rather than the name https://codereview.chromium.org/108333004/diff/70001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/bitmaptools/bitmaptools.cc:138: // Note: this works for both RGB and RGBA. nit: ...but the alpha value is ignored? https://codereview.chromium.org/108333004/diff/70001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/bitmaptools/bitmaptools.cc:152: PyObject *bmp_obj1, *bmp_obj2; nit: * on type, and I guess one per line https://codereview.chromium.org/108333004/diff/70001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/bitmaptools/bitmaptools.cc:213: PyObject* Crop(PyObject *self, PyObject *bmp_object) { nit: * on type https://codereview.chromium.org/108333004/diff/70001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/bitmaptools/bitmaptools.cc:218: int out_size = bmp.row_size * bmp.box.height(); question: shouldn't this be bmp.box.width() * bmp.box.height() * bmp.bpp ? https://codereview.chromium.org/108333004/diff/70001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/bitmaptools/bitmaptools.cc:221: for (const unsigned char* row = bmp.data; depending on the above, should this be ... row = bmp.data * bmp.box.top * bmp.row_size https://codereview.chromium.org/108333004/diff/70001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/bitmaptools/bitmaptools.cc:224: memcpy(dst, row, bmp.row_size); ...and again, bmp.box.width() * bmp.bpp?
https://codereview.chromium.org/108333004/diff/70001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/bitmaptools/bitmaptools.cc (right): https://codereview.chromium.org/108333004/diff/70001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/bitmaptools/bitmaptools.cc:101: int total_size, row_size; // in bytes On 2013/12/12 14:00:35, bulach wrote: > nit: maybe zero-initialize these in the constructor list? > also, I think we normally use one per line, without commas. Done. https://codereview.chromium.org/108333004/diff/70001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/bitmaptools/bitmaptools.cc:106: PyObject* Histogram(PyObject *self, PyObject *bmp_object) { On 2013/12/12 14:00:35, bulach wrote: > style nit: move the "*" to the type rather than the name Done. https://codereview.chromium.org/108333004/diff/70001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/bitmaptools/bitmaptools.cc:138: // Note: this works for both RGB and RGBA. On 2013/12/12 14:00:35, bulach wrote: > nit: ...but the alpha value is ignored? Yes. Added comment. https://codereview.chromium.org/108333004/diff/70001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/bitmaptools/bitmaptools.cc:152: PyObject *bmp_obj1, *bmp_obj2; On 2013/12/12 14:00:35, bulach wrote: > nit: * on type, and I guess one per line Done. https://codereview.chromium.org/108333004/diff/70001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/bitmaptools/bitmaptools.cc:213: PyObject* Crop(PyObject *self, PyObject *bmp_object) { On 2013/12/12 14:00:35, bulach wrote: > nit: * on type Done. https://codereview.chromium.org/108333004/diff/70001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/bitmaptools/bitmaptools.cc:218: int out_size = bmp.row_size * bmp.box.height(); On 2013/12/12 14:00:35, bulach wrote: > question: shouldn't this be bmp.box.width() * bmp.box.height() * bmp.bpp ? |row_size| is cropped, so they are the same: bmp.box.width() * bmp.box.height() * bmp.bpp = bmp.box.width() * bmp.bpp * bmp.box.height() = (bmp.box.right - bmp.box.left) * bmp.bpp * bmp.box.height() = bmp.row_size * bmp.box.height() https://codereview.chromium.org/108333004/diff/70001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/bitmaptools/bitmaptools.cc:221: for (const unsigned char* row = bmp.data; On 2013/12/12 14:00:35, bulach wrote: > depending on the above, should this be > ... row = bmp.data * bmp.box.top * bmp.row_size Certainly not. bmp.data = bmp.pixels.buf + bmp.box.top * bmp.row_stride + bmp.box.left * bmp.pixel_stride https://codereview.chromium.org/108333004/diff/70001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/bitmaptools/bitmaptools.cc:224: memcpy(dst, row, bmp.row_size); On 2013/12/12 14:00:35, bulach wrote: > ...and again, bmp.box.width() * bmp.bpp? And again, they are the same.
https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/bitmap.py (right): https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmap.py:135: Ignores alpha channel.""" I don't think ignoring alpha was the old semantics. But I imagine it doesn't matter for all usages of Telemetry bitmaps. Maybe we should think about just stripping alpha handling from this class altogether (not in this CL). https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmap.py:186: def Crop(self, new_box): We either have to leave this API unchanged or else update that gpu test. My preference would be to leave it unchanged. https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/bitmap_unittest.py (right): https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmap_unittest.py:74: self.assertTrue(bmp.IsEqual(file_bmp), 1) I don't understand, why the second test? Maybe a msg other than 1 would explain. https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmap_unittest.py:111: box, count = bmp.GetBoundingBox(bitmap.RgbaColor(1, 0, 0)) Should this really return count? I don't understand why. https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/bitmaptools/__init__.py (right): https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmaptools/__init__.py:1: """ Bitmap processing routines. Please add the standard copyright. https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmaptools/__init__.py:8: """Builds the extension library in place.""" Clever! https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmaptools/__init__.py:30: dist.run_commands() I don't follow exactly how this works. Does this routine avoid building every time once the user has built once? https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmaptools/__init__.py:36: raise In practice, when will we hit this?
https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/bitmap.py (right): https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmap.py:135: Ignores alpha channel.""" On 2013/12/12 22:17:48, tonyg wrote: > I don't think ignoring alpha was the old semantics. You are right. The old semantics where: compare alpha like any other channel. If alpha missing, assume 255. > But I imagine it doesn't > matter for all usages of Telemetry bitmaps. Maybe we should think about just > stripping alpha handling from this class altogether (not in this CL). https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmap.py:186: def Crop(self, new_box): On 2013/12/12 22:17:48, tonyg wrote: > We either have to leave this API unchanged or else update that gpu test. My > preference would be to leave it unchanged. Done. https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/bitmap_unittest.py (right): https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmap_unittest.py:74: self.assertTrue(bmp.IsEqual(file_bmp), 1) On 2013/12/12 22:17:48, tonyg wrote: > I don't understand, why the second test? Maybe a msg other than 1 would explain. Leftover from when Equal had two distinct codepaths. Removed. https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmap_unittest.py:111: box, count = bmp.GetBoundingBox(bitmap.RgbaColor(1, 0, 0)) On 2013/12/12 22:17:48, tonyg wrote: > Should this really return count? I don't understand why. See Tab.StopVideoCapture. Checking pixel count is more robust than checking bounding box size. I would even consider using relative pixel count to account for different capture resolution. https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/bitmaptools/__init__.py (right): https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmaptools/__init__.py:1: """ Bitmap processing routines. On 2013/12/12 22:17:48, tonyg wrote: > Please add the standard copyright. Done. https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmaptools/__init__.py:30: dist.run_commands() On 2013/12/12 22:17:48, tonyg wrote: > I don't follow exactly how this works. Does this routine avoid building every > time once the user has built once? This _BuildExtension() is equivalent to running make. If the sources are older than the .so, no action is taken. https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmaptools/__init__.py:36: raise On 2013/12/12 22:17:48, tonyg wrote: > In practice, when will we hit this? On Ubuntu, no python-dev or build-essential.
https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/bitmaptools/__init__.py (right): https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmaptools/__init__.py:30: dist.run_commands() On 2013/12/12 22:54:12, szym wrote: > On 2013/12/12 22:17:48, tonyg wrote: > > I don't follow exactly how this works. Does this routine avoid building every > > time once the user has built once? > > This _BuildExtension() is equivalent to running make. If the sources are older > than the .so, no action is taken. Even with 'clean' in script_args? https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmaptools/__init__.py:36: raise On 2013/12/12 22:54:12, szym wrote: > On 2013/12/12 22:17:48, tonyg wrote: > > In practice, when will we hit this? > > On Ubuntu, no python-dev or build-essential. The GPU tests do rely on the Bitmap class on all platforms. We can't land anything that would temporarily break win, mac or linux. If that is going to be a problem, one possible approach would be to split this CL and check in the bitmaptools directory first. Then I could build on each platform and do the cloud storage uploads. https://codereview.chromium.org/108333004/diff/130001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/bitmap.py (right): https://codereview.chromium.org/108333004/diff/130001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmap.py:180: """Finds the minimum box surrounding all occurences of |color|. We should add a Returns: section to this pydoc. I initially thought count was always w*h, but I understand now that it is count of pixels of |color| within the box. https://codereview.chromium.org/108333004/diff/130001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmap.py:201: Returns a list of 3x256 integers.""" Could you explain the format of the return value a little more thoroughly in the pydoc. https://codereview.chromium.org/108333004/diff/130001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/tab.py (right): https://codereview.chromium.org/108333004/diff/130001/tools/telemetry/telemet... tools/telemetry/telemetry/core/tab.py:150: 'Unexpectedly small tab contents' I follow your logic behind count now. Given that, I think we should leave this assert as-is and add a new assert that count is greater than 95% (or some other threshold) of w*h.
https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/bitmaptools/__init__.py (right): https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmaptools/__init__.py:30: dist.run_commands() On 2013/12/12 23:19:02, tonyg wrote: > On 2013/12/12 22:54:12, szym wrote: > > On 2013/12/12 22:17:48, tonyg wrote: > > > I don't follow exactly how this works. Does this routine avoid building > every > > > time once the user has built once? > > > > This _BuildExtension() is equivalent to running make. If the sources are older > > than the .so, no action is taken. > > Even with 'clean' in script_args? 'clean' removes the build-temp directory, not the build-dir. But now I realized that I didn't set build-temp for clean, and doing it in tools/telemetry/build is a bad idea. Fixed it to use a temporary directory. https://codereview.chromium.org/108333004/diff/110001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmaptools/__init__.py:36: raise On 2013/12/12 23:19:02, tonyg wrote: > On 2013/12/12 22:54:12, szym wrote: > > On 2013/12/12 22:17:48, tonyg wrote: > > > In practice, when will we hit this? > > > > On Ubuntu, no python-dev or build-essential. > > The GPU tests do rely on the Bitmap class on all platforms. We can't land > anything that would temporarily break win, mac or linux. > > If that is going to be a problem, one possible approach would be to split this > CL and check in the bitmaptools directory first. Then I could build on each > platform and do the cloud storage uploads. How can we check if it works on all platforms?
> How can we check if it works on all platforms? kick off a try job on win_rel, mac_rel, win_gpu, linux_gpu, mac_gpu, and mac_gpu_retina
On 2013/12/12 23:40:14, tonyg wrote: > > How can we check if it works on all platforms? > > kick off a try job on win_rel, mac_rel, win_gpu, linux_gpu, mac_gpu, and > mac_gpu_retina Note that the *_gpu ones are not default trybots, you have to run them explicitly.
On 2013/12/12 23:40:58, tonyg wrote: > On 2013/12/12 23:40:14, tonyg wrote: > > > How can we check if it works on all platforms? > > > > kick off a try job on win_rel, mac_rel, win_gpu, linux_gpu, mac_gpu, and > > mac_gpu_retina > > Note that the *_gpu ones are not default trybots, you have to run them > explicitly. Fails to compile on win: http://build.chromium.org/p/tryserver.chromium/builders/win_gpu/builds/2492/s... u'path': u'C:\\b\\depot_tools\\python276_bin;C:\\b\\depot_tools\\python276_bin\\Scripts;C:\\b\\depot_tools;C:\\Windows\\system32;C:\\Windows\\system32\\WBEM;C:\\b\\build_internal\\tools'} stderr : '\'"vsvars32.bat"\' is not recognized as an internal or external command,\r\noperable program or batch file.\r\n' vcvarsall : u'c:\\Program Files (x86)\\Microsoft Visual Studio 9.0\\VC\\vcvarsall.bat' I suspect http://bugs.python.org/issue7511
On 2013/12/13 00:23:49, szym wrote: > On 2013/12/12 23:40:58, tonyg wrote: > > On 2013/12/12 23:40:14, tonyg wrote: > > > > How can we check if it works on all platforms? > > > > > > kick off a try job on win_rel, mac_rel, win_gpu, linux_gpu, mac_gpu, and > > > mac_gpu_retina > > > > Note that the *_gpu ones are not default trybots, you have to run them > > explicitly. > > Fails to compile on win: > http://build.chromium.org/p/tryserver.chromium/builders/win_gpu/builds/2492/s... > > u'path': > u'C:\\b\\depot_tools\\python276_bin;C:\\b\\depot_tools\\python276_bin\\Scripts;C:\\b\\depot_tools;C:\\Windows\\system32;C:\\Windows\\system32\\WBEM;C:\\b\\build_internal\\tools'} > stderr : '\'"vsvars32.bat"\' is not recognized as an internal or external > command,\r\noperable program or batch file.\r\n' > vcvarsall : u'c:\\Program Files (x86)\\Microsoft Visual Studio > 9.0\\VC\\vcvarsall.bat' > > I suspect http://bugs.python.org/issue7511 Workaround ideas? Looks like linux is green, so one idea might be to keep python implementations for equals and crop and fall back to them if the bitmaputils import fails. The histogram and bounding box methods could just raise NotImplementedErrors if bitmaputils can't be imported.
On Thu, Dec 12, 2013 at 4:42 PM, <tonyg@chromium.org> wrote: > > Fails to compile on win: >> > > http://build.chromium.org/p/tryserver.chromium/builders/ > win_gpu/builds/2492/steps/maps_pixel_test/logs/stdio > > u'path': >> > > u'C:\\b\\depot_tools\\python276_bin;C:\\b\\depot_ > tools\\python276_bin\\Scripts;C:\\b\\depot_tools;C:\\ > Windows\\system32;C:\\Windows\\system32\\WBEM;C:\\b\\build_ > internal\\tools'} > >> stderr : '\'"vsvars32.bat"\' is not recognized as an internal or >> > external > >> command,\r\noperable program or batch file.\r\n' >> vcvarsall : u'c:\\Program Files (x86)\\Microsoft Visual Studio >> 9.0\\VC\\vcvarsall.bat' >> > > I suspect http://bugs.python.org/issue7511 >> > > Workaround ideas? I'm experimenting with the patch from: http://bugs.python.org/issue7511 > Looks like linux is green, so one idea might be to keep python > implementations > for equals and crop and fall back to them if the bitmaputils import fails. > The > histogram and bounding box methods could just raise NotImplementedErrors if > bitmaputils can't be imported. > mac_gpu is on good track as well. I wonder if we should just compile the .dll and use cloud storage for windows. > https://codereview.chromium.org/108333004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/12/13 01:07:25, szym wrote: > On Thu, Dec 12, 2013 at 4:42 PM, <mailto:tonyg@chromium.org> wrote: > > > > > Fails to compile on win: > >> > > > > http://build.chromium.org/p/tryserver.chromium/builders/ > > win_gpu/builds/2492/steps/maps_pixel_test/logs/stdio > > > > u'path': > >> > > > > u'C:\\b\\depot_tools\\python276_bin;C:\\b\\depot_ > > tools\\python276_bin\\Scripts;C:\\b\\depot_tools;C:\\ > > Windows\\system32;C:\\Windows\\system32\\WBEM;C:\\b\\build_ > > internal\\tools'} > > > >> stderr : '\'"vsvars32.bat"\' is not recognized as an internal or > >> > > external > > > >> command,\r\noperable program or batch file.\r\n' > >> vcvarsall : u'c:\\Program Files (x86)\\Microsoft Visual Studio > >> 9.0\\VC\\vcvarsall.bat' > >> > > > > I suspect http://bugs.python.org/issue7511 > >> > > > > Workaround ideas? > > > I'm experimenting with the patch from: http://bugs.python.org/issue7511 Fascinating hack :) > > > > Looks like linux is green, so one idea might be to keep python > > implementations > > for equals and crop and fall back to them if the bitmaputils import fails. > > The > > histogram and bounding box methods could just raise NotImplementedErrors if > > bitmaputils can't be imported. > > > > mac_gpu is on good track as well. > I wonder if we should just compile the .dll and use cloud storage for > windows. If the hack works, let's go with that. If not, then I think you are right that a .dll in the cloud would be cleaner than 2 implementations. Do you know whether a single dll will work for 32+64bit and all versions of python?
Seems to be working. This should do fine for the bots. For end-users of telemetry who don't have vs, I think a DLL will be better. I will need to check but I suspect 32bit version should be enough for both the architectures. On Dec 12, 2013 5:13 PM, <tonyg@chromium.org> wrote: > On 2013/12/13 01:07:25, szym wrote: > >> On Thu, Dec 12, 2013 at 4:42 PM, <mailto:tonyg@chromium.org> wrote: >> > > > >> > Fails to compile on win: >> >> >> > >> > http://build.chromium.org/p/tryserver.chromium/builders/ >> > win_gpu/builds/2492/steps/maps_pixel_test/logs/stdio >> > >> > u'path': >> >> >> > >> > u'C:\\b\\depot_tools\\python276_bin;C:\\b\\depot_ >> > tools\\python276_bin\\Scripts;C:\\b\\depot_tools;C:\\ >> > Windows\\system32;C:\\Windows\\system32\\WBEM;C:\\b\\build_ >> > internal\\tools'} >> > >> >> stderr : '\'"vsvars32.bat"\' is not recognized as an internal >> or >> >> >> > external >> > >> >> command,\r\noperable program or batch file.\r\n' >> >> vcvarsall : u'c:\\Program Files (x86)\\Microsoft Visual Studio >> >> 9.0\\VC\\vcvarsall.bat' >> >> >> > >> > I suspect http://bugs.python.org/issue7511 >> >> >> > >> > Workaround ideas? >> > > > I'm experimenting with the patch from: http://bugs.python.org/issue7511 >> > > Fascinating hack :) > > > > > Looks like linux is green, so one idea might be to keep python >> > implementations >> > for equals and crop and fall back to them if the bitmaputils import >> fails. >> > The >> > histogram and bounding box methods could just raise >> NotImplementedErrors if >> > bitmaputils can't be imported. >> > >> > > mac_gpu is on good track as well. >> I wonder if we should just compile the .dll and use cloud storage for >> windows. >> > > If the hack works, let's go with that. If not, then I think you are right > that a > .dll in the cloud would be cleaner than 2 implementations. Do you know > whether a > single dll will work for 32+64bit and all versions of python? > > > https://codereview.chromium.org/108333004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm assuming these comments and my previous ones are all address and the trybots including the gpu trybots are all happy. https://codereview.chromium.org/108333004/diff/230001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/bitmaptools/__init__.py (right): https://codereview.chromium.org/108333004/diff/230001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmaptools/__init__.py:11: def _FixedQueryVCVarsAll(version, arch="x86"): Please fix the style (e.g. indentation, line width, etc) and add a comment explaining why this is necessary along with a link to the bug. https://codereview.chromium.org/108333004/diff/230001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmaptools/__init__.py:181: raise Let's make this a NotImplementedError with a user-friendly message.
lgtm, thanks for the clarifications! ignorable nit below, looking forward to see the results! https://codereview.chromium.org/108333004/diff/250001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/bitmaptools/bitmaptools.cc (right): https://codereview.chromium.org/108333004/diff/250001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmaptools/bitmaptools.cc:177: if (!PixelsEqual(pixel1, pixel2, tolerance)) nit: not sure how much it'd matter in practice, but if bpp == 3 for both bitmaps && tolerance == 0, perhaps a memcmp for the whole row would be faster? no idea..
https://codereview.chromium.org/108333004/diff/230001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/bitmaptools/__init__.py (right): https://codereview.chromium.org/108333004/diff/230001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmaptools/__init__.py:11: def _FixedQueryVCVarsAll(version, arch="x86"): On 2013/12/13 03:59:11, tonyg wrote: > Please fix the style (e.g. indentation, line width, etc) and add a comment > explaining why this is necessary along with a link to the bug. Done. Managed to find a much simpler workaround. https://codereview.chromium.org/108333004/diff/230001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmaptools/__init__.py:181: raise On 2013/12/13 03:59:11, tonyg wrote: > Let's make this a NotImplementedError with a user-friendly message. Unbelievably, I found that with VS Express 12 the linker fails, but the generated .pyd is perfectly usable, so I changed this to always try to import the library, and raise NotImplementedError only if that fails.
re-lgtm Nice! This looks really solid now. Feel free to land whenever all the bots are happy. https://codereview.chromium.org/108333004/diff/130001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/tab.py (right): https://codereview.chromium.org/108333004/diff/130001/tools/telemetry/telemet... tools/telemetry/telemetry/core/tab.py:150: 'Unexpectedly small tab contents' On 2013/12/12 23:19:02, tonyg wrote: > I follow your logic behind count now. Given that, I think we should leave this > assert as-is and add a new assert that count is greater than 95% (or some other > threshold) of w*h. ping on this suggestion
https://codereview.chromium.org/108333004/diff/130001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/tab.py (right): https://codereview.chromium.org/108333004/diff/130001/tools/telemetry/telemet... tools/telemetry/telemetry/core/tab.py:150: 'Unexpectedly small tab contents' On 2013/12/14 14:20:43, tonyg wrote: > On 2013/12/12 23:19:02, tonyg wrote: > > I follow your logic behind count now. Given that, I think we should leave this > > assert as-is and add a new assert that count is greater than 95% (or some > other > > threshold) of w*h. > > ping on this suggestion Sorry, somehow I missed it. Done, although note that in my experiments the threshold should be ~75%. https://codereview.chromium.org/108333004/diff/130001/tools/telemetry/telemet... tools/telemetry/telemetry/core/tab.py:158: 'Unexpected change in tab contents box.' I'm concerned that Chrome's popup boxes can change this. Let's say the current page requested location access. A yellow bar pops up from the bottom. The pop-up is at at max z-order, so it obscures our flash div, and we think content_box is smaller.
On 2013/12/14 21:01:42, szym wrote: > https://codereview.chromium.org/108333004/diff/130001/tools/telemetry/telemet... > File tools/telemetry/telemetry/core/tab.py (right): > > https://codereview.chromium.org/108333004/diff/130001/tools/telemetry/telemet... > tools/telemetry/telemetry/core/tab.py:150: 'Unexpectedly small tab contents' > On 2013/12/14 14:20:43, tonyg wrote: > > On 2013/12/12 23:19:02, tonyg wrote: > > > I follow your logic behind count now. Given that, I think we should leave > this > > > assert as-is and add a new assert that count is greater than 95% (or some > > other > > > threshold) of w*h. > > > > ping on this suggestion > > Sorry, somehow I missed it. > > Done, although note that in my experiments the threshold should be ~75%. > > https://codereview.chromium.org/108333004/diff/130001/tools/telemetry/telemet... > tools/telemetry/telemetry/core/tab.py:158: 'Unexpected change in tab contents > box.' > I'm concerned that Chrome's popup boxes can change this. > > Let's say the current page requested location access. A yellow bar pops up from > the bottom. The pop-up is at at max z-order, so it obscures our flash div, and > we think content_box is smaller. You are probably right. I'd like to start as strict as possible with our asserts and then try to run all of the page cyclers we have. As we find cases like this, we can relax them appropriately.
https://codereview.chromium.org/108333004/diff/250001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/bitmaptools/bitmaptools.cc (right): https://codereview.chromium.org/108333004/diff/250001/tools/telemetry/telemet... tools/telemetry/telemetry/core/bitmaptools/bitmaptools.cc:177: if (!PixelsEqual(pixel1, pixel2, tolerance)) On 2013/12/13 11:43:14, bulach wrote: > nit: not sure how much it'd matter in practice, but if bpp == 3 for both bitmaps > && tolerance == 0, perhaps a memcmp for the whole row would be faster? no idea.. Done. In practice, this will only matter for multi-megapixel bitmaps.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/szym@chromium.org/108333004/470001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/szym@chromium.org/108333004/490001
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/szym@chromium.org/108333004/490001
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/szym@chromium.org/108333004/490001
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/szym@chromium.org/108333004/490001
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
On 2013/12/16 22:51:30, I haz the power (commit-bot) wrote: > Retried try job too often on linux_rel for step(s) browser_tests > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... Maybe it is time to NOTRY=True the thing. The only bots that run Telemetry tests are win_rel and mac_rel.
Message was sent while issue was closed.
Committed patchset #17 manually as r241066 (presubmit successful).
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/116903002/ by szym@chromium.org. The reason for reverting is: Fails on ChromiumOS on main waterfall.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
