Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2512123002:
Shape Detection: QR detection in Chrome Android using Play Services (Closed)
|
Created:
4 years, 1 month ago by mcasas Modified:
4 years ago CC:
chromium-reviews, creis+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, nasko+codewatch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, dglazkov+blink, darin-cc_chromium.org, agrieve+watch_chromium.org, blink-reviews, darin (slow to review), blink-reviews-api_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShape Detection: QR detection in Chrome Android using Play Services
This CL adds a java interface implementation for Barcode
detection. Since it uses the Google Play Services "vision"
category [1], this is landed under chrome/, and hangs under
the ChromeInterfaceRegistrar.java.
Intent to implement: https://groups.google.com/a/chromium.org/forum/m/#!topic/blink-dev/JkdoxpINjxQ
Test: I'm planning to add a browser_test soon; however since
bots might now have the necessary Play services, I'd rather
land it in a followup CL.
BUG=665150
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
TEST=http://codepen.io/miguelao/pen/bBWOzM, see e.g.
https://drive.google.com/file/d/0B82Jhdx0kSTVb3g5b2xOTWozd3M/view?pli=1
[1] https://developers.google.com/android/reference/com/google/android/gms/vision/barcode/package-summary
Committed: https://crrev.com/a9431a4ed6f1bccd426271370ddd6b9530ff39c9
Cr-Commit-Position: refs/heads/master@{#435781}
Patch Set 1 : #
Total comments: 4
Patch Set 2 : dgn@ and dcheng@s comments, rebase and checking |web_contents| before use. #
Total comments: 5
Patch Set 3 : dcheng@ request to check for |numPixels| overflow #
Total comments: 4
Patch Set 4 : moar comments #
Messages
Total messages: 58 (29 generated)
Description was changed from ========== Shape Detection: QR detection in Chrome Android using Play Services BUG=665150 ========== to ========== Shape Detection: QR detection in Chrome Android using Play Services BUG=665150 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Shape Detection: QR detection in Chrome Android using Play Services BUG=665150 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Shape Detection: QR detection in Chrome Android using Play Services This CL adds a java interface implementation for Barcode detection. Since it uses the Google Play Services "vision" category [1], this is landed under chrome/, and hangs under the ChromeInterfaceRegistrar.java. Test: I'm planning to add a browser_test soon; however since bots might now have the necessary Play services, I'd rather land in a separate CL. BUG=665150 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TEST=http://codepen.io/miguelao/pen/bBWOzM, see e.g. https://drive.google.com/file/d/0B82Jhdx0kSTVb3g5b2xOTWozd3M/view?pli=1 [1] https://developers.google.com/android/reference/com/google/android/gms/vision... ==========
Description was changed from ========== Shape Detection: QR detection in Chrome Android using Play Services This CL adds a java interface implementation for Barcode detection. Since it uses the Google Play Services "vision" category [1], this is landed under chrome/, and hangs under the ChromeInterfaceRegistrar.java. Test: I'm planning to add a browser_test soon; however since bots might now have the necessary Play services, I'd rather land in a separate CL. BUG=665150 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TEST=http://codepen.io/miguelao/pen/bBWOzM, see e.g. https://drive.google.com/file/d/0B82Jhdx0kSTVb3g5b2xOTWozd3M/view?pli=1 [1] https://developers.google.com/android/reference/com/google/android/gms/vision... ========== to ========== Shape Detection: QR detection in Chrome Android using Play Services This CL adds a java interface implementation for Barcode detection. Since it uses the Google Play Services "vision" category [1], this is landed under chrome/, and hangs under the ChromeInterfaceRegistrar.java. Test: I'm planning to add a browser_test soon; however since bots might now have the necessary Play services, I'd rather land it in a followup CL. BUG=665150 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TEST=http://codepen.io/miguelao/pen/bBWOzM, see e.g. https://drive.google.com/file/d/0B82Jhdx0kSTVb3g5b2xOTWozd3M/view?pli=1 [1] https://developers.google.com/android/reference/com/google/android/gms/vision... ==========
Patchset #1 (id:60001) has been deleted
mcasas@chromium.org changed reviewers: + dgn@chromium.org
dgn@ PTAL
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
https://codereview.chromium.org/2512123002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java (right): https://codereview.chromium.org/2512123002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:67: || numPixels > MOJO_SHAREDBUFFER_MAX_BYTES / 4) { Drive-by: this number can (and has) changed over time. Just check the result of the map(): if the map() fails, then we can assume the number is invalid. Also, I know java doesn't have unsigned... but what are mojo's guarantees here? It should know it's reading an unsigned off the wire, but what happens if the unsigned > int?
not lgtm, because using the new APIs has to wait until they are available downstream too. Otherwise this CL will just break the internal builders and get reverted. I'll ping here when it's ready. looks okay for Play services related code though. https://codereview.chromium.org/2512123002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java (right): https://codereview.chromium.org/2512123002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:49: if (GoogleApiAvailability.getInstance().isGooglePlayServicesAvailable(mContext) Consider using ExternalAuthUtils#canUseGooglePlayService() for chrome-standard way of handling missing gmscore
CL up for review at https://chrome-internal-review.googlesource.com/#/c/307576/
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Notwithstanding https://chrome-internal-review.googlesource.com/#/c/307576/, and just to have this CL ready when the moment arrives, PTAL. https://codereview.chromium.org/2512123002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java (right): https://codereview.chromium.org/2512123002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:49: if (GoogleApiAvailability.getInstance().isGooglePlayServicesAvailable(mContext) On 2016/11/24 10:32:32, dgn wrote: > Consider using ExternalAuthUtils#canUseGooglePlayService() for chrome-standard > way of handling missing gmscore Done. https://codereview.chromium.org/2512123002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:67: || numPixels > MOJO_SHAREDBUFFER_MAX_BYTES / 4) { On 2016/11/24 04:31:39, dcheng wrote: > Drive-by: this number can (and has) changed over time. Just check the result of > the map(): if the map() fails, then we can assume the number is invalid. > Done. > Also, I know java doesn't have unsigned... but what are mojo's guarantees here? > It should know it's reading an unsigned off the wire, but what happens if the > unsigned > int? Since you posted this in the chromium-mojo@ mailing list, I guess we'll take it there.
The CQ bit was checked by mcasas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dgn@, dcheng@ ping
dgn@, dcheng@ ping
CL landed, lgtm :)
mcasas@chromium.org changed reviewers: + dtrainor@chromium.org, thakis@chromium.org
dtrainor@ plz RS chrome/android changes /PTAL thakis@: RS plz chrome/browser/ DEPS and chrome_content_browser_client.cc thx
bauerb@chromium.org changed reviewers: + bauerb@chromium.org
I do have a security-ish question, but given that IPC OWNERS will need to sign-off anyway, I'll defer to them. The rest LGTM. https://codereview.chromium.org/2512123002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java (right): https://codereview.chromium.org/2512123002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:62: final long numPixels = (long) width * height; Are we doing any additional sanity checks of the values here? I'm going to assume that the Mojo internals would be able to check the really dangerous stuff (like OOB memory accesses in memory mapping), but could a compromised renderer here get the browser to try to allocate a gigantic bitmap and subsequently crash due to OOM?
lgtm % bauerb@'s question.
Patchset #3 (id:160001) has been deleted
dcheng@ PTAL https://codereview.chromium.org/2512123002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java (right): https://codereview.chromium.org/2512123002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:62: final long numPixels = (long) width * height; On 2016/11/30 17:09:45, Bernhard Bauer wrote: > Are we doing any additional sanity checks of the values here? I'm going to > assume that the Mojo internals would be able to check the really dangerous stuff > (like OOB memory accesses in memory mapping), but could a compromised renderer > here get the browser to try to allocate a gigantic bitmap and subsequently crash > due to OOM? The input |frameData| is a shared memory, so it needs to be mapped in l.68 and that's where the checks take place: map() will fail if we try to map too large a memory. Also, mojo SharedBuffers have a limit in its intrinsic size which used to be ~16MB, and now is |max_shared_memory_num_bytes| in mojo::edk::Configuration [1]. I added a comment to make all this evident. [1] https://cs.chromium.org/chromium/src/mojo/edk/embedder/configuration.h?dr=C&q...
I can't find the "intent to implement" blink-dev thread for this. can you point me to it?
On 2016/11/30 20:00:49, Nico wrote: > I can't find the "intent to implement" blink-dev thread for this. can you point > me to it? Should be: https://groups.google.com/a/chromium.org/forum/m/#!topic/blink-dev/JkdoxpINjxQ (it's under shape detection API)
can you link to that on the bug? On Wed, Nov 30, 2016 at 3:11 PM, <mcasas@chromium.org> wrote: > On 2016/11/30 20:00:49, Nico wrote: > > I can't find the "intent to implement" blink-dev thread for this. can you > point > > me to it? > > Should be: > > https://groups.google.com/a/chromium.org/forum/m/#!topic/ > blink-dev/JkdoxpINjxQ > > (it's under shape detection API) > > https://codereview.chromium.org/2512123002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
can you link to that on the bug? On Wed, Nov 30, 2016 at 3:11 PM, <mcasas@chromium.org> wrote: > On 2016/11/30 20:00:49, Nico wrote: > > I can't find the "intent to implement" blink-dev thread for this. can you > point > > me to it? > > Should be: > > https://groups.google.com/a/chromium.org/forum/m/#!topic/ > blink-dev/JkdoxpINjxQ > > (it's under shape detection API) > > https://codereview.chromium.org/2512123002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== Shape Detection: QR detection in Chrome Android using Play Services This CL adds a java interface implementation for Barcode detection. Since it uses the Google Play Services "vision" category [1], this is landed under chrome/, and hangs under the ChromeInterfaceRegistrar.java. Test: I'm planning to add a browser_test soon; however since bots might now have the necessary Play services, I'd rather land it in a followup CL. BUG=665150 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TEST=http://codepen.io/miguelao/pen/bBWOzM, see e.g. https://drive.google.com/file/d/0B82Jhdx0kSTVb3g5b2xOTWozd3M/view?pli=1 [1] https://developers.google.com/android/reference/com/google/android/gms/vision... ========== to ========== Shape Detection: QR detection in Chrome Android using Play Services This CL adds a java interface implementation for Barcode detection. Since it uses the Google Play Services "vision" category [1], this is landed under chrome/, and hangs under the ChromeInterfaceRegistrar.java. Intent to implement: https://groups.google.com/a/chromium.org/forum/m/#!topic/blink-dev/JkdoxpINjxQ Test: I'm planning to add a browser_test soon; however since bots might now have the necessary Play services, I'd rather land it in a followup CL. BUG=665150 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TEST=http://codepen.io/miguelao/pen/bBWOzM, see e.g. https://drive.google.com/file/d/0B82Jhdx0kSTVb3g5b2xOTWozd3M/view?pli=1 [1] https://developers.google.com/android/reference/com/google/android/gms/vision... ==========
On 2016/11/30 20:17:59, Nico wrote: > can you link to that on the bug? > > On Wed, Nov 30, 2016 at 3:11 PM, <mailto:mcasas@chromium.org> wrote: > > > On 2016/11/30 20:00:49, Nico wrote: > > > I can't find the "intent to implement" blink-dev thread for this. can you > > point > > > me to it? > > > > Should be: > > > > https://groups.google.com/a/chromium.org/forum/m/#!topic/ > > blink-dev/JkdoxpINjxQ > > > > (it's under shape detection API) > > > > https://codereview.chromium.org/2512123002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Done
https://codereview.chromium.org/2512123002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java (right): https://codereview.chromium.org/2512123002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:62: final long numPixels = (long) width * height; On 2016/11/30 19:57:09, mcasas wrote: > On 2016/11/30 17:09:45, Bernhard Bauer wrote: > > Are we doing any additional sanity checks of the values here? I'm going to > > assume that the Mojo internals would be able to check the really dangerous > stuff > > (like OOB memory accesses in memory mapping), but could a compromised renderer > > here get the browser to try to allocate a gigantic bitmap and subsequently > crash > > due to OOM? > > The input |frameData| is a shared memory, so > it needs to be mapped in l.68 and that's where > the checks take place: map() will fail if we try > to map too large a memory. Also, mojo SharedBuffers > have a limit in its intrinsic size which used > to be ~16MB, and now is |max_shared_memory_num_bytes| > in mojo::edk::Configuration [1]. I added a comment > to make all this evident. > > [1] > https://cs.chromium.org/chromium/src/mojo/edk/embedder/configuration.h?dr=C&q... I'm actually less concerned about the mapping failing, because that memory already exists somewhere. What I'm more concerned about is allocating the Bitmap. I think if the passed width * height * 4 would be bigger than the actual size of the buffer, we would abort before that happens, but that still means an allocation of up to 16 MB. It doesn't sound like much, but I'm pretty certain 16 MB can push a process on Android into OOM territory – as an example, we take screenshots of the New Tab Page for using in the tab switcher, and we used to get OOM crashes for that allocation (which I think is less than 16MB). Would it be totally unreasonable to have the renderer send the data in one of the accepted formats for https://developers.google.com/android/reference/com/google/android/gms/vision..., int, int, int) and then use that? Reading the docs for Frame, it looks like going through a Bitmap would actually require two allocations :-/ https://codereview.chromium.org/2512123002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java (right): https://codereview.chromium.org/2512123002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:69: // than its actual capacitywhich is limited by the appropriate Nit: "capacity, which"
chrome/ rs-lgtm
Patchset #3 (id:180001) has been deleted
https://codereview.chromium.org/2512123002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java (right): https://codereview.chromium.org/2512123002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:63: if (!frameData.isValid() || width <= 0 || height <= 0 || (numPixels / 4) > Long.MAX_VALUE) { This overflow check isn't quite correct: it should be numPixels > Long.MAX_VALUE / 4 Also, let's add a TODO(https://crbug.com/670028) to switch to some standardized routines. (It would nice to bring the corresponding interface in FaceDetectionImpl up-to-date on this as well)
https://codereview.chromium.org/2512123002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java (right): https://codereview.chromium.org/2512123002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:62: final long numPixels = (long) width * height; On 2016/11/30 23:01:34, Bernhard Bauer wrote: > On 2016/11/30 19:57:09, mcasas wrote: > > On 2016/11/30 17:09:45, Bernhard Bauer wrote: > > > Are we doing any additional sanity checks of the values here? I'm going to > > > assume that the Mojo internals would be able to check the really dangerous > > stuff > > > (like OOB memory accesses in memory mapping), but could a compromised > renderer > > > here get the browser to try to allocate a gigantic bitmap and subsequently > > crash > > > due to OOM? > > > > The input |frameData| is a shared memory, so > > it needs to be mapped in l.68 and that's where > > the checks take place: map() will fail if we try > > to map too large a memory. Also, mojo SharedBuffers > > have a limit in its intrinsic size which used > > to be ~16MB, and now is |max_shared_memory_num_bytes| > > in mojo::edk::Configuration [1]. I added a comment > > to make all this evident. > > > > [1] > > > https://cs.chromium.org/chromium/src/mojo/edk/embedder/configuration.h?dr=C&q... > > I'm actually less concerned about the mapping failing, because that memory > already exists somewhere. What I'm more concerned about is allocating the > Bitmap. I think if the passed width * height * 4 would be bigger than the actual > size of the buffer, we would abort before that happens, but that still means an > allocation of up to 16 MB. It doesn't sound like much, but I'm pretty certain 16 > MB can push a process on Android into OOM territory – as an example, we take > screenshots of the New Tab Page for using in the tab switcher, and we used to > get OOM crashes for that allocation (which I think is less than 16MB). > This is interesting and sad information. For the sister Face Detection, we are writing a //services/ so that we can move the implementation out of process to avoid any potential crashes from spreading. I believe, however, that //services cannot implement //chrome code, so we might need some further tweaking there. (Just FTR, this feature is behind an experimental flag). In the meantime, would verifying that we are not in a TRIM_MEMORY_RUNNING_*/TRIM_MEMORY_MODERATE [1] situation help? [1] https://developer.android.com/reference/android/content/ComponentCallbacks2.html > Would it be totally unreasonable to have the renderer send the data in one of > the accepted formats for > https://developers.google.com/android/reference/com/google/android/gms/vision..., > int, int, int) and then use that? Reading the docs for Frame, it looks like > going through a Bitmap would actually require two allocations :-/ Sadly, correct: the incoming |frameData| has to be wrapped into |bitmap|, which is then converted to a |frame|, which implies a conversion to either NV16, NV21, or YV12 in l.83 RHS. The issue is that data is coming directly from Blink where Skia primitives like to work in 32bpp formats and we have to avoid data conversions there... I see this as a matter of optimizations? https://codereview.chromium.org/2512123002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java (right): https://codereview.chromium.org/2512123002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:63: if (!frameData.isValid() || width <= 0 || height <= 0 || (numPixels / 4) > Long.MAX_VALUE) { On 2016/12/01 06:15:06, dcheng wrote: > This overflow check isn't quite correct: it should be numPixels > Long.MAX_VALUE > / 4 > Done. > Also, let's add a TODO(https://crbug.com/670028) to switch to some standardized > routines. > done. > (It would nice to bring the corresponding interface in FaceDetectionImpl > up-to-date on this as well) Filed https://crbug.com/670309. https://codereview.chromium.org/2512123002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:69: // than its actual capacitywhich is limited by the appropriate On 2016/11/30 23:01:35, Bernhard Bauer wrote: > Nit: "capacity, which" Done.
The CQ bit was checked by mcasas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mojo lgtm https://codereview.chromium.org/2512123002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java (right): https://codereview.chromium.org/2512123002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:62: final long numPixels = (long) width * height; On 2016/12/01 16:05:51, mcasas wrote: > On 2016/11/30 23:01:34, Bernhard Bauer wrote: > > On 2016/11/30 19:57:09, mcasas wrote: > > > On 2016/11/30 17:09:45, Bernhard Bauer wrote: > > > > Are we doing any additional sanity checks of the values here? I'm going to > > > > assume that the Mojo internals would be able to check the really dangerous > > > stuff > > > > (like OOB memory accesses in memory mapping), but could a compromised > > renderer > > > > here get the browser to try to allocate a gigantic bitmap and subsequently > > > crash > > > > due to OOM? > > > > > > The input |frameData| is a shared memory, so > > > it needs to be mapped in l.68 and that's where > > > the checks take place: map() will fail if we try > > > to map too large a memory. Also, mojo SharedBuffers > > > have a limit in its intrinsic size which used > > > to be ~16MB, and now is |max_shared_memory_num_bytes| > > > in mojo::edk::Configuration [1]. I added a comment > > > to make all this evident. > > > > > > [1] > > > > > > https://cs.chromium.org/chromium/src/mojo/edk/embedder/configuration.h?dr=C&q... > > > > I'm actually less concerned about the mapping failing, because that memory > > already exists somewhere. What I'm more concerned about is allocating the > > Bitmap. I think if the passed width * height * 4 would be bigger than the > actual > > size of the buffer, we would abort before that happens, but that still means > an > > allocation of up to 16 MB. It doesn't sound like much, but I'm pretty certain > 16 > > MB can push a process on Android into OOM territory – as an example, we take > > screenshots of the New Tab Page for using in the tab switcher, and we used to > > get OOM crashes for that allocation (which I think is less than 16MB). > > > > This is interesting and sad information. > > For the sister Face Detection, we are writing a //services/ > so that we can move the implementation out of process > to avoid any potential crashes from spreading. I believe, > however, that //services cannot implement //chrome code, so > we might need some further tweaking there. (Just FTR, > this feature is behind an experimental flag). > > In the meantime, would verifying that we are not in a > TRIM_MEMORY_RUNNING_*/TRIM_MEMORY_MODERATE [1] situation > help? > > [1] > https://developer.android.com/reference/android/content/ComponentCallbacks2.html > > > Would it be totally unreasonable to have the renderer send the data in one of > > the accepted formats for > > > https://developers.google.com/android/reference/com/google/android/gms/vision..., > > int, int, int) and then use that? Reading the docs for Frame, it looks like > > going through a Bitmap would actually require two allocations :-/ > > Sadly, correct: the incoming |frameData| has to be wrapped > into |bitmap|, which is then converted to a |frame|, which > implies a conversion to either NV16, NV21, or YV12 in l.83 > RHS. The issue is that data is coming directly from Blink > where Skia primitives like to work in 32bpp formats and we > have to avoid data conversions there... I see this as a > matter of optimizations? > As a counter point... a number of security team members aren't entirely comfortable with the idea of browser process acting on shared memory that could be mutated from underneath it. (It's not to say that it's impossible for this to be safe, but if we somehow changed this code so we never had to copy and could directly use the mapped in buffer, we'd want to audit the processing code very very carefully to make sure that changing pixels out from underneath it can't cause it to explode.)
On 2016/12/01 22:10:46, dcheng wrote: > mojo lgtm > > https://codereview.chromium.org/2512123002/diff/140001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java > (right): > > https://codereview.chromium.org/2512123002/diff/140001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:62: > final long numPixels = (long) width * height; > On 2016/12/01 16:05:51, mcasas wrote: > > On 2016/11/30 23:01:34, Bernhard Bauer wrote: > > > On 2016/11/30 19:57:09, mcasas wrote: > > > > On 2016/11/30 17:09:45, Bernhard Bauer wrote: > > > > > Are we doing any additional sanity checks of the values here? I'm going > to > > > > > assume that the Mojo internals would be able to check the really > dangerous > > > > stuff > > > > > (like OOB memory accesses in memory mapping), but could a compromised > > > renderer > > > > > here get the browser to try to allocate a gigantic bitmap and > subsequently > > > > crash > > > > > due to OOM? > > > > > > > > The input |frameData| is a shared memory, so > > > > it needs to be mapped in l.68 and that's where > > > > the checks take place: map() will fail if we try > > > > to map too large a memory. Also, mojo SharedBuffers > > > > have a limit in its intrinsic size which used > > > > to be ~16MB, and now is |max_shared_memory_num_bytes| > > > > in mojo::edk::Configuration [1]. I added a comment > > > > to make all this evident. > > > > > > > > [1] > > > > > > > > > > https://cs.chromium.org/chromium/src/mojo/edk/embedder/configuration.h?dr=C&q... > > > > > > I'm actually less concerned about the mapping failing, because that memory > > > already exists somewhere. What I'm more concerned about is allocating the > > > Bitmap. I think if the passed width * height * 4 would be bigger than the > > actual > > > size of the buffer, we would abort before that happens, but that still means > > an > > > allocation of up to 16 MB. It doesn't sound like much, but I'm pretty > certain > > 16 > > > MB can push a process on Android into OOM territory – as an example, we take > > > screenshots of the New Tab Page for using in the tab switcher, and we used > to > > > get OOM crashes for that allocation (which I think is less than 16MB). > > > > > > > This is interesting and sad information. > > > > For the sister Face Detection, we are writing a //services/ > > so that we can move the implementation out of process > > to avoid any potential crashes from spreading. I believe, > > however, that //services cannot implement //chrome code, so > > we might need some further tweaking there. (Just FTR, > > this feature is behind an experimental flag). > > > > In the meantime, would verifying that we are not in a > > TRIM_MEMORY_RUNNING_*/TRIM_MEMORY_MODERATE [1] situation > > help? > > > > [1] > > > https://developer.android.com/reference/android/content/ComponentCallbacks2.html > > > > > Would it be totally unreasonable to have the renderer send the data in one > of > > > the accepted formats for > > > > > > https://developers.google.com/android/reference/com/google/android/gms/vision..., > > > int, int, int) and then use that? Reading the docs for Frame, it looks like > > > going through a Bitmap would actually require two allocations :-/ > > > > Sadly, correct: the incoming |frameData| has to be wrapped > > into |bitmap|, which is then converted to a |frame|, which > > implies a conversion to either NV16, NV21, or YV12 in l.83 > > RHS. The issue is that data is coming directly from Blink > > where Skia primitives like to work in 32bpp formats and we > > have to avoid data conversions there... I see this as a > > matter of optimizations? > > > > As a counter point... a number of security team members aren't entirely > comfortable with the idea of browser process acting on shared memory that could > be mutated from underneath it. > > (It's not to say that it's impossible for this to be safe, but if we somehow > changed this code so we never had to copy and could directly use the mapped in > buffer, we'd want to audit the processing code very very carefully to make sure > that changing pixels out from underneath it can't cause it to explode.) Ugh… okay, if you feel copying the data is the lesser of two evils, LGTM. It would probably still be a good idea to keep an eye on crash reports due to OOM in this area, then we can deal with that if and when it happens.
On 2016/12/01 23:51:49, Bernhard Bauer wrote: > On 2016/12/01 22:10:46, dcheng wrote: > > mojo lgtm > > > > > https://codereview.chromium.org/2512123002/diff/140001/chrome/android/java/sr... > > File > > > chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java > > (right): > > > > > https://codereview.chromium.org/2512123002/diff/140001/chrome/android/java/sr... > > > chrome/android/java/src/org/chromium/chrome/browser/shapedetection/BarcodeDetectionImpl.java:62: > > final long numPixels = (long) width * height; > > On 2016/12/01 16:05:51, mcasas wrote: > > > On 2016/11/30 23:01:34, Bernhard Bauer wrote: > > > > On 2016/11/30 19:57:09, mcasas wrote: > > > > > On 2016/11/30 17:09:45, Bernhard Bauer wrote: > > > > > > Are we doing any additional sanity checks of the values here? I'm > going > > to > > > > > > assume that the Mojo internals would be able to check the really > > dangerous > > > > > stuff > > > > > > (like OOB memory accesses in memory mapping), but could a compromised > > > > renderer > > > > > > here get the browser to try to allocate a gigantic bitmap and > > subsequently > > > > > crash > > > > > > due to OOM? > > > > > > > > > > The input |frameData| is a shared memory, so > > > > > it needs to be mapped in l.68 and that's where > > > > > the checks take place: map() will fail if we try > > > > > to map too large a memory. Also, mojo SharedBuffers > > > > > have a limit in its intrinsic size which used > > > > > to be ~16MB, and now is |max_shared_memory_num_bytes| > > > > > in mojo::edk::Configuration [1]. I added a comment > > > > > to make all this evident. > > > > > > > > > > [1] > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/mojo/edk/embedder/configuration.h?dr=C&q... > > > > > > > > I'm actually less concerned about the mapping failing, because that memory > > > > already exists somewhere. What I'm more concerned about is allocating the > > > > Bitmap. I think if the passed width * height * 4 would be bigger than the > > > actual > > > > size of the buffer, we would abort before that happens, but that still > means > > > an > > > > allocation of up to 16 MB. It doesn't sound like much, but I'm pretty > > certain > > > 16 > > > > MB can push a process on Android into OOM territory – as an example, we > take > > > > screenshots of the New Tab Page for using in the tab switcher, and we used > > to > > > > get OOM crashes for that allocation (which I think is less than 16MB). > > > > > > > > > > This is interesting and sad information. > > > > > > For the sister Face Detection, we are writing a //services/ > > > so that we can move the implementation out of process > > > to avoid any potential crashes from spreading. I believe, > > > however, that //services cannot implement //chrome code, so > > > we might need some further tweaking there. (Just FTR, > > > this feature is behind an experimental flag). > > > > > > In the meantime, would verifying that we are not in a > > > TRIM_MEMORY_RUNNING_*/TRIM_MEMORY_MODERATE [1] situation > > > help? > > > > > > [1] > > > > > > https://developer.android.com/reference/android/content/ComponentCallbacks2.html > > > > > > > Would it be totally unreasonable to have the renderer send the data in one > > of > > > > the accepted formats for > > > > > > > > > > https://developers.google.com/android/reference/com/google/android/gms/vision..., > > > > int, int, int) and then use that? Reading the docs for Frame, it looks > like > > > > going through a Bitmap would actually require two allocations :-/ > > > > > > Sadly, correct: the incoming |frameData| has to be wrapped > > > into |bitmap|, which is then converted to a |frame|, which > > > implies a conversion to either NV16, NV21, or YV12 in l.83 > > > RHS. The issue is that data is coming directly from Blink > > > where Skia primitives like to work in 32bpp formats and we > > > have to avoid data conversions there... I see this as a > > > matter of optimizations? > > > > > > > As a counter point... a number of security team members aren't entirely > > comfortable with the idea of browser process acting on shared memory that > could > > be mutated from underneath it. > > > > (It's not to say that it's impossible for this to be safe, but if we somehow > > changed this code so we never had to copy and could directly use the mapped in > > buffer, we'd want to audit the processing code very very carefully to make > sure > > that changing pixels out from underneath it can't cause it to explode.) > > Ugh… okay, if you feel copying the data is the lesser of two evils, LGTM. It > would probably still be a good idea to keep an eye on crash reports due to OOM > in this area, then we can deal with that if and when it happens. 16 MB of allocation sounds too big to me on low-RAM devices...
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, dgn@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2512123002/#ps220001 (title: "moar comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1480636622482160,
"parent_rev": "034328abeb06e8ac9b5120af6cb1a0c077264b9e", "commit_rev":
"4fee1d76acbd3b544d090a0c5bbdc0e56e48af26"}
Message was sent while issue was closed.
Description was changed from ========== Shape Detection: QR detection in Chrome Android using Play Services This CL adds a java interface implementation for Barcode detection. Since it uses the Google Play Services "vision" category [1], this is landed under chrome/, and hangs under the ChromeInterfaceRegistrar.java. Intent to implement: https://groups.google.com/a/chromium.org/forum/m/#!topic/blink-dev/JkdoxpINjxQ Test: I'm planning to add a browser_test soon; however since bots might now have the necessary Play services, I'd rather land it in a followup CL. BUG=665150 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TEST=http://codepen.io/miguelao/pen/bBWOzM, see e.g. https://drive.google.com/file/d/0B82Jhdx0kSTVb3g5b2xOTWozd3M/view?pli=1 [1] https://developers.google.com/android/reference/com/google/android/gms/vision... ========== to ========== Shape Detection: QR detection in Chrome Android using Play Services This CL adds a java interface implementation for Barcode detection. Since it uses the Google Play Services "vision" category [1], this is landed under chrome/, and hangs under the ChromeInterfaceRegistrar.java. Intent to implement: https://groups.google.com/a/chromium.org/forum/m/#!topic/blink-dev/JkdoxpINjxQ Test: I'm planning to add a browser_test soon; however since bots might now have the necessary Play services, I'd rather land it in a followup CL. BUG=665150 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TEST=http://codepen.io/miguelao/pen/bBWOzM, see e.g. https://drive.google.com/file/d/0B82Jhdx0kSTVb3g5b2xOTWozd3M/view?pli=1 [1] https://developers.google.com/android/reference/com/google/android/gms/vision... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Shape Detection: QR detection in Chrome Android using Play Services This CL adds a java interface implementation for Barcode detection. Since it uses the Google Play Services "vision" category [1], this is landed under chrome/, and hangs under the ChromeInterfaceRegistrar.java. Intent to implement: https://groups.google.com/a/chromium.org/forum/m/#!topic/blink-dev/JkdoxpINjxQ Test: I'm planning to add a browser_test soon; however since bots might now have the necessary Play services, I'd rather land it in a followup CL. BUG=665150 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TEST=http://codepen.io/miguelao/pen/bBWOzM, see e.g. https://drive.google.com/file/d/0B82Jhdx0kSTVb3g5b2xOTWozd3M/view?pli=1 [1] https://developers.google.com/android/reference/com/google/android/gms/vision... ========== to ========== Shape Detection: QR detection in Chrome Android using Play Services This CL adds a java interface implementation for Barcode detection. Since it uses the Google Play Services "vision" category [1], this is landed under chrome/, and hangs under the ChromeInterfaceRegistrar.java. Intent to implement: https://groups.google.com/a/chromium.org/forum/m/#!topic/blink-dev/JkdoxpINjxQ Test: I'm planning to add a browser_test soon; however since bots might now have the necessary Play services, I'd rather land it in a followup CL. BUG=665150 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation TEST=http://codepen.io/miguelao/pen/bBWOzM, see e.g. https://drive.google.com/file/d/0B82Jhdx0kSTVb3g5b2xOTWozd3M/view?pli=1 [1] https://developers.google.com/android/reference/com/google/android/gms/vision... Committed: https://crrev.com/a9431a4ed6f1bccd426271370ddd6b9530ff39c9 Cr-Commit-Position: refs/heads/master@{#435781} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a9431a4ed6f1bccd426271370ddd6b9530ff39c9 Cr-Commit-Position: refs/heads/master@{#435781}
Message was sent while issue was closed.
On Thu, Dec 1, 2016 at 6:55 PM, <haraken@chromium.org> wrote: > On 2016/12/01 23:51:49, Bernhard Bauer wrote: > > On 2016/12/01 22:10:46, dcheng wrote: > > > mojo lgtm > > > > > > > > > https://codereview.chromium.org/2512123002/diff/140001/ > chrome/android/java/src/org/chromium/chrome/browser/shapedetection/ > BarcodeDetectionImpl.java > > > File > > > > > > chrome/android/java/src/org/chromium/chrome/browser/shapedetection/ > BarcodeDetectionImpl.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2512123002/diff/140001/ > chrome/android/java/src/org/chromium/chrome/browser/shapedetection/ > BarcodeDetectionImpl.java#newcode62 > > > > > > chrome/android/java/src/org/chromium/chrome/browser/shapedetection/ > BarcodeDetectionImpl.java:62: > > > final long numPixels = (long) width * height; > > > On 2016/12/01 16:05:51, mcasas wrote: > > > > On 2016/11/30 23:01:34, Bernhard Bauer wrote: > > > > > On 2016/11/30 19:57:09, mcasas wrote: > > > > > > On 2016/11/30 17:09:45, Bernhard Bauer wrote: > > > > > > > Are we doing any additional sanity checks of the values here? > I'm > > going > > > to > > > > > > > assume that the Mojo internals would be able to check the > really > > > dangerous > > > > > > stuff > > > > > > > (like OOB memory accesses in memory mapping), but could a > compromised > > > > > renderer > > > > > > > here get the browser to try to allocate a gigantic bitmap and > > > subsequently > > > > > > crash > > > > > > > due to OOM? > > > > > > > > > > > > The input |frameData| is a shared memory, so > > > > > > it needs to be mapped in l.68 and that's where > > > > > > the checks take place: map() will fail if we try > > > > > > to map too large a memory. Also, mojo SharedBuffers > > > > > > have a limit in its intrinsic size which used > > > > > > to be ~16MB, and now is |max_shared_memory_num_bytes| > > > > > > in mojo::edk::Configuration [1]. I added a comment > > > > > > to make all this evident. > > > > > > > > > > > > [1] > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/mojo/edk/ > embedder/configuration.h?dr=C&q=mojo/edk/embedder/ > configuration.h&sq=package:chromium&l=63 > > > > > > > > > > I'm actually less concerned about the mapping failing, because that > memory > > > > > already exists somewhere. What I'm more concerned about is > allocating > the > > > > > Bitmap. I think if the passed width * height * 4 would be bigger > than > the > > > > actual > > > > > size of the buffer, we would abort before that happens, but that > still > > means > > > > an > > > > > allocation of up to 16 MB. It doesn't sound like much, but I'm > pretty > > > certain > > > > 16 > > > > > MB can push a process on Android into OOM territory – as an > example, we > > take > > > > > screenshots of the New Tab Page for using in the tab switcher, and > we > used > > > to > > > > > get OOM crashes for that allocation (which I think is less than > 16MB). > > > > > > > > > > > > > This is interesting and sad information. > > > > > > > > For the sister Face Detection, we are writing a //services/ > > > > so that we can move the implementation out of process > > > > to avoid any potential crashes from spreading. I believe, > > > > however, that //services cannot implement //chrome code, so > > > > we might need some further tweaking there. (Just FTR, > > > > this feature is behind an experimental flag). > > > > > > > > In the meantime, would verifying that we are not in a > > > > TRIM_MEMORY_RUNNING_*/TRIM_MEMORY_MODERATE [1] situation > > > > help? > > > > > > > > [1] > > > > > > > > > > https://developer.android.com/reference/android/content/ > ComponentCallbacks2.html > > > > > > > > > Would it be totally unreasonable to have the renderer send the > data in > one > > > of > > > > > the accepted formats for > > > > > > > > > > > > > > > https://developers.google.com/android/reference/com/google/ > android/gms/vision/Frame.Builder.html#setImageData(java.nio.ByteBuffer, > > > > > int, int, int) and then use that? Reading the docs for Frame, it > looks > > like > > > > > going through a Bitmap would actually require two allocations :-/ > > > > > > > > Sadly, correct: the incoming |frameData| has to be wrapped > > > > into |bitmap|, which is then converted to a |frame|, which > > > > implies a conversion to either NV16, NV21, or YV12 in l.83 > > > > RHS. The issue is that data is coming directly from Blink > > > > where Skia primitives like to work in 32bpp formats and we > > > > have to avoid data conversions there... I see this as a > > > > matter of optimizations? > > > > > > > > > > As a counter point... a number of security team members aren't entirely > > > comfortable with the idea of browser process acting on shared memory > that > > could > > > be mutated from underneath it. > > > > > > (It's not to say that it's impossible for this to be safe, but if we > somehow > > > changed this code so we never had to copy and could directly use the > mapped > in > > > buffer, we'd want to audit the processing code very very carefully to > make > > sure > > > that changing pixels out from underneath it can't cause it to explode.) > > > > Ugh… okay, if you feel copying the data is the lesser of two evils, > LGTM. It > > would probably still be a good idea to keep an eye on crash reports due > to OOM > > in this area, then we can deal with that if and when it happens. > > 16 MB of allocation sounds too big to me on low-RAM devices... > (Might be something to bring up on the intent-to-implement thread) > > https://codereview.chromium.org/2512123002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On Thu, Dec 1, 2016 at 6:55 PM, <haraken@chromium.org> wrote: > On 2016/12/01 23:51:49, Bernhard Bauer wrote: > > On 2016/12/01 22:10:46, dcheng wrote: > > > mojo lgtm > > > > > > > > > https://codereview.chromium.org/2512123002/diff/140001/ > chrome/android/java/src/org/chromium/chrome/browser/shapedetection/ > BarcodeDetectionImpl.java > > > File > > > > > > chrome/android/java/src/org/chromium/chrome/browser/shapedetection/ > BarcodeDetectionImpl.java > > > (right): > > > > > > > > > https://codereview.chromium.org/2512123002/diff/140001/ > chrome/android/java/src/org/chromium/chrome/browser/shapedetection/ > BarcodeDetectionImpl.java#newcode62 > > > > > > chrome/android/java/src/org/chromium/chrome/browser/shapedetection/ > BarcodeDetectionImpl.java:62: > > > final long numPixels = (long) width * height; > > > On 2016/12/01 16:05:51, mcasas wrote: > > > > On 2016/11/30 23:01:34, Bernhard Bauer wrote: > > > > > On 2016/11/30 19:57:09, mcasas wrote: > > > > > > On 2016/11/30 17:09:45, Bernhard Bauer wrote: > > > > > > > Are we doing any additional sanity checks of the values here? > I'm > > going > > > to > > > > > > > assume that the Mojo internals would be able to check the > really > > > dangerous > > > > > > stuff > > > > > > > (like OOB memory accesses in memory mapping), but could a > compromised > > > > > renderer > > > > > > > here get the browser to try to allocate a gigantic bitmap and > > > subsequently > > > > > > crash > > > > > > > due to OOM? > > > > > > > > > > > > The input |frameData| is a shared memory, so > > > > > > it needs to be mapped in l.68 and that's where > > > > > > the checks take place: map() will fail if we try > > > > > > to map too large a memory. Also, mojo SharedBuffers > > > > > > have a limit in its intrinsic size which used > > > > > > to be ~16MB, and now is |max_shared_memory_num_bytes| > > > > > > in mojo::edk::Configuration [1]. I added a comment > > > > > > to make all this evident. > > > > > > > > > > > > [1] > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/mojo/edk/ > embedder/configuration.h?dr=C&q=mojo/edk/embedder/ > configuration.h&sq=package:chromium&l=63 > > > > > > > > > > I'm actually less concerned about the mapping failing, because that > memory > > > > > already exists somewhere. What I'm more concerned about is > allocating > the > > > > > Bitmap. I think if the passed width * height * 4 would be bigger > than > the > > > > actual > > > > > size of the buffer, we would abort before that happens, but that > still > > means > > > > an > > > > > allocation of up to 16 MB. It doesn't sound like much, but I'm > pretty > > > certain > > > > 16 > > > > > MB can push a process on Android into OOM territory – as an > example, we > > take > > > > > screenshots of the New Tab Page for using in the tab switcher, and > we > used > > > to > > > > > get OOM crashes for that allocation (which I think is less than > 16MB). > > > > > > > > > > > > > This is interesting and sad information. > > > > > > > > For the sister Face Detection, we are writing a //services/ > > > > so that we can move the implementation out of process > > > > to avoid any potential crashes from spreading. I believe, > > > > however, that //services cannot implement //chrome code, so > > > > we might need some further tweaking there. (Just FTR, > > > > this feature is behind an experimental flag). > > > > > > > > In the meantime, would verifying that we are not in a > > > > TRIM_MEMORY_RUNNING_*/TRIM_MEMORY_MODERATE [1] situation > > > > help? > > > > > > > > [1] > > > > > > > > > > https://developer.android.com/reference/android/content/ > ComponentCallbacks2.html > > > > > > > > > Would it be totally unreasonable to have the renderer send the > data in > one > > > of > > > > > the accepted formats for > > > > > > > > > > > > > > > https://developers.google.com/android/reference/com/google/ > android/gms/vision/Frame.Builder.html#setImageData(java.nio.ByteBuffer, > > > > > int, int, int) and then use that? Reading the docs for Frame, it > looks > > like > > > > > going through a Bitmap would actually require two allocations :-/ > > > > > > > > Sadly, correct: the incoming |frameData| has to be wrapped > > > > into |bitmap|, which is then converted to a |frame|, which > > > > implies a conversion to either NV16, NV21, or YV12 in l.83 > > > > RHS. The issue is that data is coming directly from Blink > > > > where Skia primitives like to work in 32bpp formats and we > > > > have to avoid data conversions there... I see this as a > > > > matter of optimizations? > > > > > > > > > > As a counter point... a number of security team members aren't entirely > > > comfortable with the idea of browser process acting on shared memory > that > > could > > > be mutated from underneath it. > > > > > > (It's not to say that it's impossible for this to be safe, but if we > somehow > > > changed this code so we never had to copy and could directly use the > mapped > in > > > buffer, we'd want to audit the processing code very very carefully to > make > > sure > > > that changing pixels out from underneath it can't cause it to explode.) > > > > Ugh… okay, if you feel copying the data is the lesser of two evils, > LGTM. It > > would probably still be a good idea to keep an eye on crash reports due > to OOM > > in this area, then we can deal with that if and when it happens. > > 16 MB of allocation sounds too big to me on low-RAM devices... > (Might be something to bring up on the intent-to-implement thread) > > https://codereview.chromium.org/2512123002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
