Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2798413002:
stop using copyPixelsTo -- deprecated (Closed)
Descriptionstop using copyPixelsTo -- deprecated
BUG=skia:6465
Review-Url: https://codereview.chromium.org/2798413002
Cr-Commit-Position: refs/heads/master@{#463303}
Committed: https://chromium.googlesource.com/chromium/src/+/3e81a3f2ab68a0f96e7b1ef8b32606b7ffef30cd
Patch Set 1 #
Total comments: 16
Patch Set 2 : fix cast and comment #Patch Set 3 : support dst_stride being signed #Messages
Total messages: 28 (19 generated)
The CQ bit was checked by reed@google.com to run a CQ dry run
reed@google.com changed reviewers: + kelvinp@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
reed@google.com changed reviewers: + msarett@chromium.org
lgtm
sergeyu@chromium.org changed reviewers: + sergeyu@chromium.org
lgtm when my comments are addressed https://codereview.chromium.org/2798413002/diff/1/remoting/test/frame_generat... File remoting/test/frame_generator_util.cc (right): https://codereview.chromium.org/2798413002/diff/1/remoting/test/frame_generat... remoting/test/frame_generator_util.cc:18: size_t dst_stride) { In webrtc::DesktopFrame stride may be negative, so this should be int. This doesn't matter here because in BasicDesktopFrame stride is always positive, still better to use int. https://codereview.chromium.org/2798413002/diff/1/remoting/test/frame_generat... remoting/test/frame_generator_util.cc:23: // only need to copy the important parts of the row I don't think this comment is necessary, but if you keep it then please change it to "Only need to copy the important parts of the row." (capital O, period at the end). https://codereview.chromium.org/2798413002/diff/1/remoting/test/frame_generat... remoting/test/frame_generator_util.cc:25: DCHECK(bytes_per_row <= dst_stride); stride may be negative for webrtc::DesktopFrame, so I don't think this DCHECK is valid, just remove it? https://codereview.chromium.org/2798413002/diff/1/remoting/test/frame_generat... remoting/test/frame_generator_util.cc:29: dst_pixels = (char*)dst_pixels + dst_stride; I don't think you need the cast here. dst_pixels += dst_stride; This is more consistent with the previous line. In either case it shouldn't be C-style cast: https://google.github.io/styleguide/cppguide.html#Casting https://codereview.chromium.org/2798413002/diff/1/remoting/test/frame_generat... remoting/test/frame_generator_util.cc:32: } // namespace
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2798413002/diff/1/remoting/test/frame_generat... File remoting/test/frame_generator_util.cc (right): https://codereview.chromium.org/2798413002/diff/1/remoting/test/frame_generat... remoting/test/frame_generator_util.cc:18: size_t dst_stride) { On 2017/04/06 19:04:28, Sergey Ulanov wrote: > In webrtc::DesktopFrame stride may be negative, so this should be int. This > doesn't matter here because in BasicDesktopFrame stride is always positive, > still better to use int. The old logic cast it to size_t when it called copyPixelsTo(). What is the meaning of negative? Should this function fail if it sees a negative frame stride? https://codereview.chromium.org/2798413002/diff/1/remoting/test/frame_generat... remoting/test/frame_generator_util.cc:23: // only need to copy the important parts of the row On 2017/04/06 19:04:28, Sergey Ulanov wrote: > I don't think this comment is necessary, but if you keep it then please change > it to "Only need to copy the important parts of the row." (capital O, period at > the end). Done. https://codereview.chromium.org/2798413002/diff/1/remoting/test/frame_generat... remoting/test/frame_generator_util.cc:25: DCHECK(bytes_per_row <= dst_stride); On 2017/04/06 19:04:28, Sergey Ulanov wrote: > stride may be negative for webrtc::DesktopFrame, so I don't think this DCHECK is > valid, just remove it? Should this fail if it sees a negative? https://codereview.chromium.org/2798413002/diff/1/remoting/test/frame_generat... remoting/test/frame_generator_util.cc:29: dst_pixels = (char*)dst_pixels + dst_stride; On 2017/04/06 19:04:28, Sergey Ulanov wrote: > I don't think you need the cast here. > dst_pixels += dst_stride; > This is more consistent with the previous line. > In either case it shouldn't be C-style cast: > https://google.github.io/styleguide/cppguide.html#Casting dst_pixels is void*, so it cannot perform += without a cast. Will use c++ style cast. https://codereview.chromium.org/2798413002/diff/1/remoting/test/frame_generat... remoting/test/frame_generator_util.cc:32: } On 2017/04/06 19:04:28, Sergey Ulanov wrote: > // namespace Done.
The CQ bit was checked by reed@google.com 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.
lgtm, but please see my comments https://codereview.chromium.org/2798413002/diff/1/remoting/test/frame_generat... File remoting/test/frame_generator_util.cc (right): https://codereview.chromium.org/2798413002/diff/1/remoting/test/frame_generat... remoting/test/frame_generator_util.cc:18: size_t dst_stride) { On 2017/04/07 12:28:13, reed1 wrote: > On 2017/04/06 19:04:28, Sergey Ulanov wrote: > > In webrtc::DesktopFrame stride may be negative, so this should be int. This > > doesn't matter here because in BasicDesktopFrame stride is always positive, > > still better to use int. > > The old logic cast it to size_t when it called copyPixelsTo(). What is the > meaning of negative? It's used for the case when rows are stored from bottom to top. This is useful for some capturers, see https://codesearch.chromium.org/chromium/src/third_party/webrtc/modules/deskt... In all cases DesktopFrame::data() points to the top row and adding stride() moves to the row below it. > Should this function fail if it sees a negative frame stride? No, it will still work correctly with negative stride as is. Just need to remove the DCHECK and use signed type for dst_stride. https://codereview.chromium.org/2798413002/diff/1/remoting/test/frame_generat... remoting/test/frame_generator_util.cc:25: DCHECK(bytes_per_row <= dst_stride); On 2017/04/07 12:28:13, reed1 wrote: > On 2017/04/06 19:04:28, Sergey Ulanov wrote: > > stride may be negative for webrtc::DesktopFrame, so I don't think this DCHECK > is > > valid, just remove it? > > Should this fail if it sees a negative? No. https://codereview.chromium.org/2798413002/diff/1/remoting/test/frame_generat... remoting/test/frame_generator_util.cc:29: dst_pixels = (char*)dst_pixels + dst_stride; On 2017/04/07 12:28:13, reed1 wrote: > On 2017/04/06 19:04:28, Sergey Ulanov wrote: > > I don't think you need the cast here. > > dst_pixels += dst_stride; > > This is more consistent with the previous line. > > In either case it shouldn't be C-style cast: > > https://google.github.io/styleguide/cppguide.html#Casting > > dst_pixels is void*, so it cannot perform += without a cast. Maybe change it to uint8_t*? It's the type used for DesktopFrame::data() and I don't think it's necessary to cast it to void* when CopyPixelsToBuffer() is called. > > Will use c++ style cast.
https://codereview.chromium.org/2798413002/diff/1/remoting/test/frame_generat... File remoting/test/frame_generator_util.cc (right): https://codereview.chromium.org/2798413002/diff/1/remoting/test/frame_generat... remoting/test/frame_generator_util.cc:18: size_t dst_stride) { On 2017/04/07 18:05:12, Sergey Ulanov wrote: > On 2017/04/07 12:28:13, reed1 wrote: > > On 2017/04/06 19:04:28, Sergey Ulanov wrote: > > > In webrtc::DesktopFrame stride may be negative, so this should be int. This > > > doesn't matter here because in BasicDesktopFrame stride is always positive, > > > still better to use int. > > > > The old logic cast it to size_t when it called copyPixelsTo(). What is the > > meaning of negative? > It's used for the case when rows are stored from bottom to top. This is useful > for some capturers, see > https://codesearch.chromium.org/chromium/src/third_party/webrtc/modules/deskt... > > In all cases DesktopFrame::data() points to the top row and adding stride() > moves to the row below it. > > > > Should this function fail if it sees a negative frame stride? > > No, it will still work correctly with negative stride as is. Just need to remove > the DCHECK and use signed type for dst_stride. > Got it. The old code-path (the method being deprecated) was definitely not ready for a negative stride. https://codereview.chromium.org/2798413002/diff/1/remoting/test/frame_generat... remoting/test/frame_generator_util.cc:25: DCHECK(bytes_per_row <= dst_stride); On 2017/04/06 19:04:28, Sergey Ulanov wrote: > stride may be negative for webrtc::DesktopFrame, so I don't think this DCHECK is > valid, just remove it? Done. https://codereview.chromium.org/2798413002/diff/1/remoting/test/frame_generat... remoting/test/frame_generator_util.cc:29: dst_pixels = (char*)dst_pixels + dst_stride; On 2017/04/07 18:05:12, Sergey Ulanov wrote: > On 2017/04/07 12:28:13, reed1 wrote: > > On 2017/04/06 19:04:28, Sergey Ulanov wrote: > > > I don't think you need the cast here. > > > dst_pixels += dst_stride; > > > This is more consistent with the previous line. > > > In either case it shouldn't be C-style cast: > > > https://google.github.io/styleguide/cppguide.html#Casting > > > > dst_pixels is void*, so it cannot perform += without a cast. > > Maybe change it to uint8_t*? It's the type used for DesktopFrame::data() and I > don't think it's necessary to cast it to void* when CopyPixelsToBuffer() is > called. > > > > > Will use c++ style cast. > Done.
The CQ bit was checked by reed@google.com 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.
The CQ bit was checked by reed@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from msarett@chromium.org, sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2798413002/#ps40001 (title: "support dst_stride being signed")
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": 40001, "attempt_start_ts": 1491843736564610,
"parent_rev": "90bce3cf40ac6f6cfaf1e3b742bf47f86f2d4e6e", "commit_rev":
"3e81a3f2ab68a0f96e7b1ef8b32606b7ffef30cd"}
Message was sent while issue was closed.
Description was changed from ========== stop using copyPixelsTo -- deprecated BUG=skia:6465 ========== to ========== stop using copyPixelsTo -- deprecated BUG=skia:6465 Review-Url: https://codereview.chromium.org/2798413002 Cr-Commit-Position: refs/heads/master@{#463303} Committed: https://chromium.googlesource.com/chromium/src/+/3e81a3f2ab68a0f96e7b1ef8b326... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/3e81a3f2ab68a0f96e7b1ef8b326... |
