Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22935009:
Add content::SurfaceCapturer (Closed)
|
Created:
7 years, 4 months ago by sheu Modified:
7 years, 3 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, jam, apatrick_chromium, sievers+watch_chromium.org, jbauman+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, danakj+watch_chromium.org, miu+watch_chromium.org, Ami GONE FROM CHROMIUM Base URL:
https://chromium.googlesource.com/chromium/src.git@screencast_stride Visibility:
Public. |
DescriptionThe SurfaceCapturer interface is for screen capturers that capture
contents directly from a surface. This change:
* Adds the SurfaceCapturer interface
* Adds GLSurfaceCapturer{,Host}, which expose the interface across IPC from
the GPU process to the browser process.
BUG=260210
BUG=221441
TEST=local build, run on CrOS snow
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220369
Patch Set 1 : cff149b4 WIP #
Total comments: 10
Patch Set 2 : 7794dca4 Rebase, WIP #Patch Set 3 : 893c8c05 Interface change due to hshi@. #Patch Set 4 : af28d80b WIP #Patch Set 5 : 229d7eef Complete. #Patch Set 6 : 89e2665f Rebase. #
Total comments: 23
Patch Set 7 : eb01d94a Comment fixes. #
Total comments: 2
Patch Set 8 : f7ced01c IPC check fix. #Patch Set 9 : af8f3e73 Separated from BrowserCompositorOutputSurface* components. #
Total comments: 6
Patch Set 10 : fc5be088 Comment fixes. #Patch Set 11 : a1372c98 Rebase for commit. #Messages
Total messages: 35 (0 generated)
https://chromiumcodereview.appspot.com/22935009/diff/3001/content/browser/aur... File content/browser/aura/browser_compositor_output_surface_capturer.cc (right): https://chromiumcodereview.appspot.com/22935009/diff/3001/content/browser/aur... content/browser/aura/browser_compositor_output_surface_capturer.cc:24: client_(client_ptr_factory_.GetWeakPtr()); typo: ';' -> ',' https://chromiumcodereview.appspot.com/22935009/diff/3001/content/common/gpu/... File content/common/gpu/client/gl_surface_capturer_host.h (right): https://chromiumcodereview.appspot.com/22935009/diff/3001/content/common/gpu/... content/common/gpu/client/gl_surface_capturer_host.h:73: const base::ThreadChecker thread_checker_; const thread_checker_ not initialized in ctor. https://chromiumcodereview.appspot.com/22935009/diff/3001/content/common/gpu/... File content/common/gpu/media/gl_surface_capturer.h (right): https://chromiumcodereview.appspot.com/22935009/diff/3001/content/common/gpu/... content/common/gpu/media/gl_surface_capturer.h:62: const base::ThreadChecker thread_checker_; const thread_checker_ not initialized in ctor.
Drive-by review comment: You've added a lot of new code and should be adding a lot of unit tests. ;-) https://chromiumcodereview.appspot.com/22935009/diff/3001/content/common/gpu/... File content/common/gpu/client/gl_surface_capturer_host.h (right): https://chromiumcodereview.appspot.com/22935009/diff/3001/content/common/gpu/... content/common/gpu/client/gl_surface_capturer_host.h:73: const base::ThreadChecker thread_checker_; On 2013/08/15 19:40:18, hshi1 wrote: > const thread_checker_ not initialized in ctor. Is this in the C++ style guide? On the surface, this seems perfectly reasonable since the class has only a zero-arg constructor.
https://chromiumcodereview.appspot.com/22935009/diff/3001/content/common/gpu/... File content/common/gpu/client/gl_surface_capturer_host.h (right): https://chromiumcodereview.appspot.com/22935009/diff/3001/content/common/gpu/... content/common/gpu/client/gl_surface_capturer_host.h:73: const base::ThreadChecker thread_checker_; On 2013/08/15 20:22:08, Yuri wrote: > On 2013/08/15 19:40:18, hshi1 wrote: > > const thread_checker_ not initialized in ctor. > > Is this in the C++ style guide? On the surface, this seems perfectly reasonable > since the class has only a zero-arg constructor. Not a style violation but compile fails with message "error: uninitialized member 'content::GLSurfaceCapturer::thread_checker_' with 'const' type 'const base::ThreadChecker' [-fpermissive]".
https://chromiumcodereview.appspot.com/22935009/diff/3001/media/base/video_fr... File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/22935009/diff/3001/media/base/video_fr... media/base/video_frame.cc:251: size_t VideoFrame::AllocationSize(Format format, const gfx::Size& coded_size) { The behavior of this function does not match the padding used by AllocYUV() and AllocRGB() in this class, and I worry that this is misleading. It seems that what is really implemented here is the padding convention that is assumed by the video capture code. VideoFrame::WrapExternalSharedMemory makes the same assumption for YV12 data. How can we move to a more consistent behavior? In general, I think the right approach is for whoever allocates the memory to pick strides/padding, and for this decision to be explicitly communicated to all subsequent users.
https://chromiumcodereview.appspot.com/22935009/diff/3001/media/base/video_fr... File media/base/video_frame.cc (right): https://chromiumcodereview.appspot.com/22935009/diff/3001/media/base/video_fr... media/base/video_frame.cc:251: size_t VideoFrame::AllocationSize(Format format, const gfx::Size& coded_size) { On 2013/08/15 20:46:59, ncarter wrote: > The behavior of this function does not match the padding used by AllocYUV() and > AllocRGB() in this class, and I worry that this is misleading. > > It seems that what is really implemented here is the padding convention that is > assumed by the video capture code. VideoFrame::WrapExternalSharedMemory makes > the same assumption for YV12 data. How can we move to a more consistent > behavior? > > In general, I think the right approach is for whoever allocates the memory to > pick strides/padding, and for this decision to be explicitly communicated to all > subsequent users. The problem is that I'm doing a lot of "if format then packed size = " kind of thing all over for the capture pipeline, and I'd like to centralize it in one place. I could rename this "PackedAllocationSize" or something if it makes things clearer. This is essentially a helper for whoever that is which picks the strides/padding. When the VideoFrame is actually constructed with WrapExternalSharedMemory, then width/stride are exposed to subsequent user as visible_rect/coded_size, respectively.
Can this be not one huge CL? It'd be nice to add APIs and new objects with tests in piece-wise CLs.
https://chromiumcodereview.appspot.com/22935009/diff/3001/content/browser/aur... File content/browser/aura/browser_compositor_output_surface_capturer.cc (right): https://chromiumcodereview.appspot.com/22935009/diff/3001/content/browser/aur... content/browser/aura/browser_compositor_output_surface_capturer.cc:91: void BrowserCompositorOutputSurfaceCapturer::NotifySwapBuffersOnImplThread() { Why are you capturing the compositor's output after swap buffers is done? Why not just put a CopyOutputRequest on the compositor's root layer and capture everything to a texture as it draws its frame?
https://chromiumcodereview.appspot.com/22935009/diff/3001/content/browser/aur... File content/browser/aura/browser_compositor_output_surface_capturer.cc (right): https://chromiumcodereview.appspot.com/22935009/diff/3001/content/browser/aur... content/browser/aura/browser_compositor_output_surface_capturer.cc:91: void BrowserCompositorOutputSurfaceCapturer::NotifySwapBuffersOnImplThread() { On 2013/08/15 21:39:41, danakj wrote: > Why are you capturing the compositor's output after swap buffers is done? Why > not just put a CopyOutputRequest on the compositor's root layer and capture > everything to a texture as it draws its frame? I'm doing this for the purposes of being able to copy into a texture backed by an allocation that actually comes from the color-conversion hardware, so as to be able to offload the conversion. Copying it to a texture means that I don't have access to it from the hardware, unless we're able to start passing these things around as EGLImages (which we don't have support presently in our GPU client GLES stack).
https://chromiumcodereview.appspot.com/22935009/diff/3001/content/browser/aur... File content/browser/aura/browser_compositor_output_surface_capturer.cc (right): https://chromiumcodereview.appspot.com/22935009/diff/3001/content/browser/aur... content/browser/aura/browser_compositor_output_surface_capturer.cc:91: void BrowserCompositorOutputSurfaceCapturer::NotifySwapBuffersOnImplThread() { On 2013/08/15 21:48:05, sheu wrote: > On 2013/08/15 21:39:41, danakj wrote: > > Why are you capturing the compositor's output after swap buffers is done? Why > > not just put a CopyOutputRequest on the compositor's root layer and capture > > everything to a texture as it draws its frame? > > I'm doing this for the purposes of being able to copy into a texture backed by > an allocation that actually comes from the color-conversion hardware, so as to > be able to offload the conversion. Copying it to a texture means that I don't > have access to it from the hardware, unless we're able to start passing these > things around as EGLImages (which we don't have support presently in our GPU > client GLES stack). Can we extend CopyOutputRequest to satisfy this rather than reimplement it?
danakj@: the ideal implementation would be to have CopyOutputRequest/Result work with texture mailboxes, plumb texture mailboxes throuhg WebRTC, and be able to pass them to the encoder. I'd like to see capturing working first, though, before I start optimizing. So pretty please? PTAL. cdn@: IPC additions in content/common/gpu/gpu_messages.h fischman@: pretty much everything in content/common/gpu/*, though you're only actually owner for a bit of that posciak@, hshi@: design
On Thu, Aug 22, 2013 at 9:21 PM, <sheu@chromium.org> wrote: > danakj@: the ideal implementation would be to have > CopyOutputRequest/Result work > with texture mailboxes, plumb texture mailboxes throuhg WebRTC, and be > able to > pass them to the encoder. > CopyOutputResult already returns a texture mailbox when using hardware compositing, and an SkBitmap when using software compositing. > > I'd like to see capturing working first, though, before I start > optimizing. So > pretty please? > I feel like this patch adds a ton of complexity to implement something that's already implemented quite simply. Maybe you can point out why it is needed complexity? > > PTAL. > > cdn@: IPC additions in content/common/gpu/gpu_**messages.h > fischman@: pretty much everything in content/common/gpu/*, though you're > only > actually owner for a bit of that > posciak@, hshi@: design > > https://chromiumcodereview.**appspot.com/22935009/<https://chromiumcodereview... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/08/23 01:31:38, danakj wrote: > CopyOutputResult already returns a texture mailbox when using hardware > compositing, and an SkBitmap when using software compositing. > > I feel like this patch adds a ton of complexity to implement something > that's already implemented quite simply. Maybe you can point out why it is > needed complexity? What we're gaining is the ability now to do the colorspace conversion on the hardware side on a low-end device, and capturing to a software buffer of the hardware compositing backbuffer asynchronously.
On Thu, Aug 22, 2013 at 9:42 PM, <sheu@chromium.org> wrote: > On 2013/08/23 01:31:38, danakj wrote: > >> CopyOutputResult already returns a texture mailbox when using hardware >> compositing, and an SkBitmap when using software compositing. >> > > I feel like this patch adds a ton of complexity to implement something >> that's already implemented quite simply. Maybe you can point out why it is >> needed complexity? >> > > What we're gaining is the ability now to do the colorspace conversion on > the > hardware side on a low-end device, and capturing to a software buffer of > the > hardware compositing backbuffer asynchronously. > Would you say those aren't optimizations? Those seem much more feasible to add to a simple implementation, than it does to move a complex and highly optimized solution into a simpler one later. > > https://chromiumcodereview.**appspot.com/22935009/<https://chromiumcodereview... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
My impression is that most of the complexity hooking up to the compositor we'd have to do anyways, CopyOutputResult or no. I have to be able to: (1) Copy the backbuffer to an intermediate cache, since we might have to capture while screen contents aren't changing so we aren't actually compositing (2) Run this from the compositor impl thread to the ui thread so we can actually operate on the captured contents. And as a bonus (3) we get to use hardware color conversion here. I'd like to have this piped the "right" way, again, but for now this works, is (relatively) self-contained, and has been more extensively tested (though this and its previous iteration) than another solution.
On Fri, Aug 23, 2013 at 3:34 PM, <sheu@chromium.org> wrote: > My impression is that most of the complexity hooking up to the compositor > we'd > have to do anyways, CopyOutputResult or no. > > I have to be able to: > > (1) Copy the backbuffer to an intermediate cache, since we might have to > capture > while screen contents aren't changing so we aren't actually compositing > Requesting a capture will cause the compositor to come alive and generate a texture. > (2) Run this from the compositor impl thread to the ui thread so we can > actually > operate on the captured contents. > The CopyOutputResult is returned in a callback on the main thread, which is the ui thread. Let's meet and discuss. > > And as a bonus (3) we get to use hardware color conversion here. > > I'd like to have this piped the "right" way, again, but for now this > works, is > (relatively) self-contained, and has been more extensively tested (though > this > and its previous iteration) than another solution. > > https://chromiumcodereview.**appspot.com/22935009/<https://chromiumcodereview... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... File content/common/gpu/media/gl_surface_capturer.cc (right): https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... content/common/gpu/media/gl_surface_capturer.cc:46: void GLSurfaceCapturer::NotifyCaptureParameters(const gfx::Size& buffer_size, As discussed over chat you should set reasonable upper bounds for these sizes.. otherwise we end up overflowing in the check later on.
https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... File content/common/gpu/client/command_buffer_proxy_impl.cc (right): https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... content/common/gpu/client/command_buffer_proxy_impl.cc:509: route_id_, &capturer_route_id))) { Indent off? https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... File content/common/gpu/gpu_messages.h (right): https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... content/common/gpu/gpu_messages.h:784: // Attempt to perform a capture. s/perform/start/ ? https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... File content/common/gpu/media/gl_surface_capturer.h (right): https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... content/common/gpu/media/gl_surface_capturer.h:22: }; // namespace gfx s/;// https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... File content/common/gpu/surface_capturer.h (right): https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... content/common/gpu/surface_capturer.h:16: }; // namespace ui s/;// https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... content/common/gpu/surface_capturer.h:38: // passed to Capture() should mind the new parameters. s/Capture()/CopyCaptureToVideoFrame()/ ? But personally I like Capture() much better. I don't see why bother saying "Copy". And we pass VF, so it should be obvious too. We just want the capturer to Capture() into our VF... https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... content/common/gpu/surface_capturer.h:41: // to capture to (corresponds to |frame->coded_size()| from in Capture(). s/from// https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... content/common/gpu/surface_capturer.h:53: virtual void NotifyCopyCaptureDone( NotifyFrameReady? https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... content/common/gpu/surface_capturer.h:71: virtual void Initialize(media::VideoFrame::Format format) = 0; I'm assuming NotifyError() if the format is unsupported? Doco. How do we know Initialize succeeded (i.e. that we can now capture stuff)? We get NotifyCaptureParameters? Doco? https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... content/common/gpu/surface_capturer.h:77: virtual void TryCapture() = 0; You mean this starts capturing frames continuously? Doco. What happens on failure? NotifyError? I'm wondering, should we try to specify fps? Most of the time we would not be interested in capturing at 60fps, sometimes not even 30? How do we stop? Destroy? https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... content/common/gpu/surface_capturer.h:82: virtual void CopyCaptureToVideoFrame( What happens on error? I assume we get NotifyError after TryCapture. Do we get NotifyError after this call too? What if we TryCapture(), then not receive NotifyError if it fails and do CopyCaptureToVideoFrame()? We get two NotifyError()s?
https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... File content/common/gpu/client/command_buffer_proxy_impl.cc (right): https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... content/common/gpu/client/command_buffer_proxy_impl.cc:509: route_id_, &capturer_route_id))) { On 2013/08/24 01:29:31, posciak wrote: > Indent off? It's correct -- 4 from the starting opening '(' of the "if". If nothing else, "git cl format" likes it this way. https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... File content/common/gpu/gpu_messages.h (right): https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... content/common/gpu/gpu_messages.h:784: // Attempt to perform a capture. On 2013/08/24 01:29:31, posciak wrote: > s/perform/start/ ? Done. https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... File content/common/gpu/media/gl_surface_capturer.cc (right): https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... content/common/gpu/media/gl_surface_capturer.cc:46: void GLSurfaceCapturer::NotifyCaptureParameters(const gfx::Size& buffer_size, On 2013/08/23 22:29:39, Cris Neckar wrote: > As discussed over chat you should set reasonable upper bounds for these sizes.. > otherwise we end up overflowing in the check later on. Done. https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... File content/common/gpu/media/gl_surface_capturer.h (right): https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... content/common/gpu/media/gl_surface_capturer.h:22: }; // namespace gfx On 2013/08/24 01:29:31, posciak wrote: > s/;// Done. https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... File content/common/gpu/surface_capturer.h (right): https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... content/common/gpu/surface_capturer.h:16: }; // namespace ui On 2013/08/24 01:29:31, posciak wrote: > s/;// Done. https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... content/common/gpu/surface_capturer.h:38: // passed to Capture() should mind the new parameters. On 2013/08/24 01:29:31, posciak wrote: > s/Capture()/CopyCaptureToVideoFrame()/ ? > But personally I like Capture() much better. I don't see why bother saying > "Copy". And we pass VF, so it should be obvious too. We just want the capturer > to Capture() into our VF... I wanted to make the two-stage nature of the capture clear. TryCapture only captures into a temporary buffer, and CopyCaptureToVideoFrame copies this temporary into the output video frame. https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... content/common/gpu/surface_capturer.h:41: // to capture to (corresponds to |frame->coded_size()| from in Capture(). On 2013/08/24 01:29:31, posciak wrote: > s/from// Done. https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... content/common/gpu/surface_capturer.h:53: virtual void NotifyCopyCaptureDone( On 2013/08/24 01:29:31, posciak wrote: > NotifyFrameReady? I'd like the notification name to reflect the API entry point it callbacks for. https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... content/common/gpu/surface_capturer.h:71: virtual void Initialize(media::VideoFrame::Format format) = 0; On 2013/08/24 01:29:31, posciak wrote: > I'm assuming NotifyError() if the format is unsupported? Doco. > How do we know Initialize succeeded (i.e. that we can now capture stuff)? We get > NotifyCaptureParameters? Doco? Done. https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... content/common/gpu/surface_capturer.h:77: virtual void TryCapture() = 0; On 2013/08/24 01:29:31, posciak wrote: > You mean this starts capturing frames continuously? Doco. > What happens on failure? NotifyError? > > I'm wondering, should we try to specify fps? Most of the time we would not be > interested in capturing at 60fps, sometimes not even 30? > > How do we stop? Destroy? This callback makes one frame capture attempt. Updated the comment. Basically, this call should be made every time the surface contents have changed and are about to be swapped, to cache its contents. Every time you want the result of the capture, the CopyCaptureToVideoFrame() entry point is used. https://chromiumcodereview.appspot.com/22935009/diff/66001/content/common/gpu... content/common/gpu/surface_capturer.h:82: virtual void CopyCaptureToVideoFrame( On 2013/08/24 01:29:31, posciak wrote: > What happens on error? I assume we get NotifyError after TryCapture. Do we get > NotifyError after this call too? > > What if we TryCapture(), then not receive NotifyError if it fails and do > CopyCaptureToVideoFrame()? We get two NotifyError()s? We should be getting NotifyError always on error condition. So if TryCapture fails for platform reasons (not just a we-tried-but-couldn't-do-it-at-this-time, we'd get a NotifyError for that. And then a subsequent attempt at CopyCaptureToVideoFrame() could fail separately, if it couldn't copy from the cached contents, creating another NotifyError callback. I don't think this is unusual?
https://codereview.chromium.org/22935009/diff/66001/content/common/gpu/media/... File content/common/gpu/media/gl_surface_capturer.cc (right): https://codereview.chromium.org/22935009/diff/66001/content/common/gpu/media/... content/common/gpu/media/gl_surface_capturer.cc:46: void GLSurfaceCapturer::NotifyCaptureParameters(const gfx::Size& buffer_size, On 2013/08/26 21:30:52, sheu wrote: > On 2013/08/23 22:29:39, Cris Neckar wrote: > > As discussed over chat you should set reasonable upper bounds for these > sizes.. > > otherwise we end up overflowing in the check later on. > > Done. Sorry, where is this being checked now?
On 2013/08/26 22:33:47, Cris Neckar wrote: > https://codereview.chromium.org/22935009/diff/66001/content/common/gpu/media/... > File content/common/gpu/media/gl_surface_capturer.cc (right): > > https://codereview.chromium.org/22935009/diff/66001/content/common/gpu/media/... > content/common/gpu/media/gl_surface_capturer.cc:46: void > GLSurfaceCapturer::NotifyCaptureParameters(const gfx::Size& buffer_size, > On 2013/08/26 21:30:52, sheu wrote: > > On 2013/08/23 22:29:39, Cris Neckar wrote: > > > As discussed over chat you should set reasonable upper bounds for these > > sizes.. > > > otherwise we end up overflowing in the check later on. > > > > Done. > > Sorry, where is this being checked now? gl_surface_capturer{,_host}.cc
nvm I was looking at the wrong version ;) https://codereview.chromium.org/22935009/diff/79001/content/common/gpu/media/... File content/common/gpu/media/gl_surface_capturer.cc (right): https://codereview.chromium.org/22935009/diff/79001/content/common/gpu/media/... content/common/gpu/media/gl_surface_capturer.cc:57: if (buffer_size.width() > kMaxCaptureSize || Aren't these signed ints underneath. Negatives would be bad here too..
https://chromiumcodereview.appspot.com/22935009/diff/79001/content/common/gpu... File content/common/gpu/media/gl_surface_capturer.cc (right): https://chromiumcodereview.appspot.com/22935009/diff/79001/content/common/gpu... content/common/gpu/media/gl_surface_capturer.cc:57: if (buffer_size.width() > kMaxCaptureSize || On 2013/08/26 22:40:03, Cris Neckar wrote: > Aren't these signed ints underneath. Negatives would be bad here too.. Done.
On 2013/08/27 01:20:03, sheu wrote: > https://chromiumcodereview.appspot.com/22935009/diff/79001/content/common/gpu... > File content/common/gpu/media/gl_surface_capturer.cc (right): > > https://chromiumcodereview.appspot.com/22935009/diff/79001/content/common/gpu... > content/common/gpu/media/gl_surface_capturer.cc:57: if (buffer_size.width() > > kMaxCaptureSize || > On 2013/08/26 22:40:03, Cris Neckar wrote: > > Aren't these signed ints underneath. Negatives would be bad here too.. > > Done. IPC changes lgtm
Per discussion with danakj@ and piman@, I've split off the BrowserCompositorOutputSurface* components of this CL. piman@: PTAL:
https://chromiumcodereview.appspot.com/22935009/diff/99001/content/common/gpu... File content/common/gpu/client/gl_surface_capturer_host.cc (right): https://chromiumcodereview.appspot.com/22935009/diff/99001/content/common/gpu... content/common/gpu/client/gl_surface_capturer_host.cc:109: client_ = NULL; client_ptr_factory_.InvalidateWeakPtrs() (but see below). https://chromiumcodereview.appspot.com/22935009/diff/99001/content/common/gpu... content/common/gpu/client/gl_surface_capturer_host.cc:132: base::Bind(&SurfaceCapturer::Client::NotifyError, Why not PostTask(&GLSurfaceCapturerHost::OnNotifyError, weak_this_factory_.GetWeakPtr(), error) ? That way you don't need client_ptr_factory_ https://chromiumcodereview.appspot.com/22935009/diff/99001/content/common/gpu... File content/common/gpu/client/gl_surface_capturer_host.h (right): https://chromiumcodereview.appspot.com/22935009/diff/99001/content/common/gpu... content/common/gpu/client/gl_surface_capturer_host.h:81: base::WeakPtrFactory<GLSurfaceCapturerHost> weak_this_factory_; nit: move this to the end, so that it's cleared before the other fields on destruction?
Updated. Also with a leak fix. piman@: PTAL https://chromiumcodereview.appspot.com/22935009/diff/99001/content/common/gpu... File content/common/gpu/client/gl_surface_capturer_host.cc (right): https://chromiumcodereview.appspot.com/22935009/diff/99001/content/common/gpu... content/common/gpu/client/gl_surface_capturer_host.cc:109: client_ = NULL; On 2013/08/28 21:59:44, piman wrote: > client_ptr_factory_.InvalidateWeakPtrs() > (but see below). Done. https://chromiumcodereview.appspot.com/22935009/diff/99001/content/common/gpu... content/common/gpu/client/gl_surface_capturer_host.cc:132: base::Bind(&SurfaceCapturer::Client::NotifyError, On 2013/08/28 21:59:44, piman wrote: > Why not PostTask(&GLSurfaceCapturerHost::OnNotifyError, > weak_this_factory_.GetWeakPtr(), error) ? > That way you don't need client_ptr_factory_ Done. https://chromiumcodereview.appspot.com/22935009/diff/99001/content/common/gpu... File content/common/gpu/client/gl_surface_capturer_host.h (right): https://chromiumcodereview.appspot.com/22935009/diff/99001/content/common/gpu... content/common/gpu/client/gl_surface_capturer_host.h:81: base::WeakPtrFactory<GLSurfaceCapturerHost> weak_this_factory_; On 2013/08/28 21:59:44, piman wrote: > nit: move this to the end, so that it's cleared before the other fields on > destruction? I think the sort order makes sense this way, and shouldn't be relying on destruction order for any correctness.
On Wed, Aug 28, 2013 at 3:20 PM, <sheu@chromium.org> wrote: > Updated. Also with a leak fix. > > piman@: PTAL > > > > https://chromiumcodereview.**appspot.com/22935009/diff/** > 99001/content/common/gpu/**client/gl_surface_capturer_**host.cc<https://chromiumcodereview.appspot.com/22935009/diff/99001/content/common/gpu/client/gl_surface_capturer_host.cc> > File content/common/gpu/client/gl_**surface_capturer_host.cc (right): > > https://chromiumcodereview.**appspot.com/22935009/diff/** > 99001/content/common/gpu/**client/gl_surface_capturer_**host.cc#newcode109<https://chromiumcodereview.appspot.com/22935009/diff/99001/content/common/gpu/client/gl_surface_capturer_host.cc#newcode109> > content/common/gpu/client/gl_**surface_capturer_host.cc:109: client_ = > NULL; > On 2013/08/28 21:59:44, piman wrote: > >> client_ptr_factory_.**InvalidateWeakPtrs() >> (but see below). >> > > Done. > > > https://chromiumcodereview.**appspot.com/22935009/diff/** > 99001/content/common/gpu/**client/gl_surface_capturer_**host.cc#newcode132<https://chromiumcodereview.appspot.com/22935009/diff/99001/content/common/gpu/client/gl_surface_capturer_host.cc#newcode132> > content/common/gpu/client/gl_**surface_capturer_host.cc:132: > base::Bind(&SurfaceCapturer::**Client::NotifyError, > On 2013/08/28 21:59:44, piman wrote: > >> Why not PostTask(&**GLSurfaceCapturerHost::**OnNotifyError, >> weak_this_factory_.GetWeakPtr(**), error) ? >> That way you don't need client_ptr_factory_ >> > > Done. > > > https://chromiumcodereview.**appspot.com/22935009/diff/** > 99001/content/common/gpu/**client/gl_surface_capturer_**host.h<https://chromiumcodereview.appspot.com/22935009/diff/99001/content/common/gpu/client/gl_surface_capturer_host.h> > File content/common/gpu/client/gl_**surface_capturer_host.h (right): > > https://chromiumcodereview.**appspot.com/22935009/diff/** > 99001/content/common/gpu/**client/gl_surface_capturer_**host.h#newcode81<https://chromiumcodereview.appspot.com/22935009/diff/99001/content/common/gpu/client/gl_surface_capturer_host.h#newcode81> > content/common/gpu/client/gl_**surface_capturer_host.h:81: > base::WeakPtrFactory<**GLSurfaceCapturerHost> weak_this_factory_; > On 2013/08/28 21:59:44, piman wrote: > >> nit: move this to the end, so that it's cleared before the other >> > fields on > >> destruction? >> > > I think the sort order makes sense this way, and shouldn't be relying on > destruction order for any correctness. > Better safe than sorry. > > https://chromiumcodereview.**appspot.com/22935009/<https://chromiumcodereview... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/08/28 23:20:03, piman wrote: > Better safe than sorry. It's not actually being safe though. And I think this sort order makes more sense -- general variables at the top, and descending in terms of increasing specificity to specific functions. If it makes you feel better, there's an explicit InvalidateWeakPtrs() in the destructor.
On Wed, Aug 28, 2013 at 4:34 PM, <sheu@chromium.org> wrote: > On 2013/08/28 23:20:03, piman wrote: > >> Better safe than sorry. >> > > It's not actually being safe though. The destruction order is fully specified by C++. You can rely on it for the dependencies between your fields. It's a very common pattern everywhere in the code. Putting this at the end forces it to be destroyed before anything else, and documents that all the other fields outlive it. That way, it's always safe to dereference those weak pointers if they're not NULL. You don't have to look through the implementation to reason about it. See also recent discussion about SupportsWeakPtr and why it is generally unsafe. > And I think this sort order makes more sense -- general variables at the top, and descending in terms of increasing > specificity to specific functions. > I guess I'm never thinking of fields as being attached to specific functions. All the fields in the object are inter-dependent state. If it makes you feel better, there's an explicit InvalidateWeakPtrs() in the > destructor. > Thanks, that works. Just be aware of any potential changes to the destructor, if it ends up calling into the client (directly or indirectly), that ends up calling back in and getting a new weak ptr from the factory, which would then be incorrect to dereference. So, LGTM for now if you want. > https://chromiumcodereview.**appspot.com/22935009/<https://chromiumcodereview... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/22935009/105001
Failed to apply patch for content/content_common.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file content/content_common.gypi
Hunk #1 succeeded at 192 (offset -1 lines).
Hunk #2 FAILED at 222.
1 out of 2 hunks FAILED -- saving rejects to file
content/content_common.gypi.rej
Patch: content/content_common.gypi
Index: content/content_common.gypi
diff --git a/content/content_common.gypi b/content/content_common.gypi
index
18ecdeaa036c638bb63b5721eb7cf92ef479fd9c..a34d808d513cc7af8aee6c3c2543aa3dca6e8bb3
100644
--- a/content/content_common.gypi
+++ b/content/content_common.gypi
@@ -193,6 +193,8 @@
'common/gpu/client/gl_helper.h',
'common/gpu/client/gl_helper_scaling.cc',
'common/gpu/client/gl_helper_scaling.h',
+ 'common/gpu/client/gl_surface_capturer_host.cc',
+ 'common/gpu/client/gl_surface_capturer_host.h',
'common/gpu/client/gpu_channel_host.cc',
'common/gpu/client/gpu_channel_host.h',
'common/gpu/client/gpu_video_decode_accelerator_host.cc',
@@ -220,29 +222,33 @@
'common/gpu/gpu_process_launch_causes.h',
'common/gpu/gpu_rendering_stats.cc',
'common/gpu/gpu_rendering_stats.h',
- 'common/gpu/gpu_surface_lookup.h',
'common/gpu/gpu_surface_lookup.cc',
- 'common/gpu/stream_texture_manager_android.cc',
- 'common/gpu/stream_texture_manager_android.h',
+ 'common/gpu/gpu_surface_lookup.h',
'common/gpu/gpu_watchdog.h',
- 'common/gpu/image_transport_surface.h',
'common/gpu/image_transport_surface.cc',
+ 'common/gpu/image_transport_surface.h',
'common/gpu/image_transport_surface_android.cc',
'common/gpu/image_transport_surface_linux.cc',
'common/gpu/image_transport_surface_mac.cc',
'common/gpu/image_transport_surface_win.cc',
- 'common/gpu/media/h264_bit_reader.cc',
- 'common/gpu/media/h264_bit_reader.h',
- 'common/gpu/media/h264_parser.cc',
- 'common/gpu/media/h264_parser.h',
+ 'common/gpu/media/gl_surface_capturer.cc',
+ 'common/gpu/media/gl_surface_capturer.h',
'common/gpu/media/gpu_video_decode_accelerator.cc',
'common/gpu/media/gpu_video_decode_accelerator.h',
'common/gpu/media/gpu_video_encode_accelerator.cc',
'common/gpu/media/gpu_video_encode_accelerator.h',
- 'common/gpu/sync_point_manager.h',
+ 'common/gpu/media/h264_bit_reader.cc',
+ 'common/gpu/media/h264_bit_reader.h',
+ 'common/gpu/media/h264_parser.cc',
+ 'common/gpu/media/h264_parser.h',
+ 'common/gpu/stream_texture_manager_android.cc',
+ 'common/gpu/stream_texture_manager_android.h',
+ 'common/gpu/surface_capturer.cc',
+ 'common/gpu/surface_capturer.h',
'common/gpu/sync_point_manager.cc',
- 'common/gpu/texture_image_transport_surface.h',
+ 'common/gpu/sync_point_manager.h',
'common/gpu/texture_image_transport_surface.cc',
+ 'common/gpu/texture_image_transport_surface.h',
'common/handle_enumerator_win.cc',
'common/handle_enumerator_win.h',
'common/image_messages.h',
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/22935009/114001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sheu@chromium.org/22935009/114001
Message was sent while issue was closed.
Change committed as 220369 |
