Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
24072012:
Hold video frame in Bitmap instead of keeping a ByteBuffer reference. (Closed)
|
Created:
7 years, 3 months ago by Lambros Modified:
7 years, 2 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, rmsousa+watch_chromium.org, weitaosu+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionHold video frame in Bitmap instead of keeping a ByteBuffer reference.
Sometimes, the DesktopView was repainted whilst |JniInterface.sBuffer|
no longer referred to valid frame data. This CL cleans things up by
replacing the ByteBuffer with a Bitmap, and having JniFrameConsumer
copy the completely-decoded data directly into the Java Bitmap.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228898
Patch Set 1 : #
Total comments: 9
Patch Set 2 : Add graphics thread check #Patch Set 3 : rebase #Patch Set 4 : Pass DesktopFrame to ChromotingJniRuntime #Patch Set 5 : rebase #Patch Set 6 : Allow FrameConsumer to operate directly on Bitmap pixels #
Total comments: 21
Patch Set 7 : Address comments #
Total comments: 12
Patch Set 8 : Keep track of buffers in JniFrameConsumer #
Messages
Total messages: 18 (0 generated)
Found this bug whilst working on the perf improvements we discussed (byte-swapping the video after a frame is decoded). I was seeing partially-byte-swapped video frames which occurred whenever a repaint was triggered from a Java event-handler. sergeyu: ptal at jni_frame_consumer.cc solb: ptal at the rest of it
https://codereview.chromium.org/24072012/diff/2001/remoting/android/java/src/... File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java (right): https://codereview.chromium.org/24072012/diff/2001/remoting/android/java/src/... remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:315: int[] frame = new int[width * height]; Can you add a check here to ensure that this is being called on the graphics thread?
ping? https://chromiumcodereview.appspot.com/24072012/diff/2001/remoting/android/ja... File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java (right): https://chromiumcodereview.appspot.com/24072012/diff/2001/remoting/android/ja... remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:315: int[] frame = new int[width * height]; On 2013/09/23 19:47:43, garykac wrote: > Can you add a check here to ensure that this is being called on the graphics > thread? Done.
https://codereview.chromium.org/24072012/diff/2001/remoting/android/java/src/... File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java (right): https://codereview.chromium.org/24072012/diff/2001/remoting/android/java/src/... remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:318: buffer.asIntBuffer().get(frame, 0, frame.length); This copies content of the buffer. I think we can avoid doing it. You just need a custom webrtc::DesktopFrame implementation that stores data in an int array in java heap and then allows releasing it to pass to this function. https://codereview.chromium.org/24072012/diff/2001/remoting/client/jni/chromo... File remoting/client/jni/chromoting_jni_runtime.cc (right): https://codereview.chromium.org/24072012/diff/2001/remoting/client/jni/chromo... remoting/client/jni/chromoting_jni_runtime.cc:180: env->NewDirectByteBuffer(data, width * height * kBytesPerPixel)); frame stride may not be equal width*kBytesPerPixel (though it's not the case with BasicDesktopFrame). I think you also need to pass frame stride to setVideoFrame. https://codereview.chromium.org/24072012/diff/2001/remoting/client/jni/chromo... File remoting/client/jni/chromoting_jni_runtime.h (right): https://codereview.chromium.org/24072012/diff/2001/remoting/client/jni/chromo... remoting/client/jni/chromoting_jni_runtime.h:85: void UpdateImageBuffer(int width, int height, uint8* buffer); Pass the frame as webrtc::DesktopFrame reference. Then you wouldn't need the comment about ownership.
lgtm https://codereview.chromium.org/24072012/diff/2001/remoting/android/java/src/... File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java (right): https://codereview.chromium.org/24072012/diff/2001/remoting/android/java/src/... remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:318: buffer.asIntBuffer().get(frame, 0, frame.length); On 2013/09/25 23:32:08, Sergey Ulanov wrote: > This copies content of the buffer. I think we can avoid doing it. You just need > a custom webrtc::DesktopFrame implementation that stores data in an int array in > java heap and then allows releasing it to pass to this function. If I understand the patches correctly, https://codereview.chromium.org/23677011/ will be applied on top of this, and it eliminates this copy. https://codereview.chromium.org/24072012/diff/2001/remoting/client/jni/chromo... File remoting/client/jni/chromoting_jni_runtime.cc (right): https://codereview.chromium.org/24072012/diff/2001/remoting/client/jni/chromo... remoting/client/jni/chromoting_jni_runtime.cc:180: env->NewDirectByteBuffer(data, width * height * kBytesPerPixel)); On 2013/09/25 23:32:08, Sergey Ulanov wrote: > frame stride may not be equal width*kBytesPerPixel (though it's not the case > with BasicDesktopFrame). I think you also need to pass frame stride to > setVideoFrame. I can see the argument for this, but at the same time, the setVideoFrame() method's choice of BitmapConfig assumes kBytesPerPixel will always be 4, and supporting different values would require additional code in that method to handle cases we don't need. At the very most, we'd be using the passed value to print an error/warning if it didn't match our expectations.
Sergey, ptal https://codereview.chromium.org/24072012/diff/2001/remoting/android/java/src/... File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java (right): https://codereview.chromium.org/24072012/diff/2001/remoting/android/java/src/... remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:318: buffer.asIntBuffer().get(frame, 0, frame.length); On 2013/09/26 14:21:36, solb wrote: > On 2013/09/25 23:32:08, Sergey Ulanov wrote: > > This copies content of the buffer. I think we can avoid doing it. You just > need > > a custom webrtc::DesktopFrame implementation that stores data in an int array > in > > java heap and then allows releasing it to pass to this function. > > If I understand the patches correctly, https://codereview.chromium.org/23677011/ > will be applied on top of this, and it eliminates this copy. Yes - with these two CLs landed, there will only be a single copy from C++ video frame pixels to a Java Bitmap. Unless Java Bitmap can provide direct access to its pixels to C++ code, or Android can draw pixels directly from a ByteBuffer (or byte[] array) onto the screen, we cannot avoid this copy. https://codereview.chromium.org/24072012/diff/2001/remoting/client/jni/chromo... File remoting/client/jni/chromoting_jni_runtime.cc (right): https://codereview.chromium.org/24072012/diff/2001/remoting/client/jni/chromo... remoting/client/jni/chromoting_jni_runtime.cc:180: env->NewDirectByteBuffer(data, width * height * kBytesPerPixel)); On 2013/09/25 23:32:08, Sergey Ulanov wrote: > frame stride may not be equal width*kBytesPerPixel (though it's not the case > with BasicDesktopFrame). I think you also need to pass frame stride to > setVideoFrame. It's possible to deal with strides that are different from the width, but it may be less efficient. The Bitmap ctors that accept a stride argument also require passing an int[] array holding the pixels. That means the new Bitmap will read and process the pixels from int[], as well as during copyPixelsFromBuffer(). Also, Bitmap expects a stride measured in pixels but DesktopFrame exposes its stride in bytes. So we'd have to divide it by 4 before passing it to Java (and DCHECK that it's a multiple of 4). I'm not sure if it's worth supporting different strides, so I've just added a DCHECK that the stride is width * kBytesPerPixel.
ptal This restricts copying to only the region that was decoded, instead of copying the whole buffer each time. I've also added code to drop the Bitmap reference on disconnection, to free up the Bitmap for GC. +yfriedman for the DEPS change - OK for Android Chromoting client to use code in ui/gfx/android/ ?
https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_runtime.h (right): https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_runtime.h:21: class DesktopFrame; Not used in this file. https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_runtime.h:92: void UpdateFrameBitmap(jobject bitmap); please comment that |bitmat| must be an android.graphics.Bitmap. https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/jni_f... File remoting/client/jni/jni_frame_consumer.cc (right): https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/jni_f... remoting/client/jni/jni_frame_consumer.cc:49: view_size.height() > view_size_.height()) { I think if you get different view_size you can just ignore the frame here - it belongs to the previous generation, before SetSourceSize() called SetOutputSizeAndClip(). https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/jni_f... remoting/client/jni/jni_frame_consumer.cc:56: AllocateBuffer(); This also calls DrawBuffer() internally. That doesn't look right to me. https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/jni_f... remoting/client/jni/jni_frame_consumer.cc:100: frame_producer_->RequestReturnBuffers(base::Bind( I'm not sure why we need this. Frame producer should return old buffers as result of SetOutputSizeAndClip() anyway. Instead you can call AllocateBuffers directly(). https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/jni_f... remoting/client/jni/jni_frame_consumer.cc:123: jobject bitmap_global_ref = env->NewGlobalRef( Do you need to call NewGlobalRef() here? ScopedJavaGlobalRef should call NewGlobalRef() internally, so you will leak the reference by calling NewGlobalRef() without corresponding DeleteGlobalRef(). https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/jni_f... File remoting/client/jni/jni_frame_consumer.h (right): https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/jni_f... remoting/client/jni/jni_frame_consumer.h:69: scoped_ptr<gfx::JavaBitmap> bitmap_; JavaBitmap holds bitmap content locked. I think it shouldn't be class member. Instead you create it only when you need to modify a bitmap object.
That dep is fine. However, I think you'll really want to switch to jni generator and use the jni registrar from ui/ sooner rather than later. Otherwise you'll start hitting crashes as soon as you use jni functionality from the rest of chromium. https://codereview.chromium.org/24072012/diff/37001/remoting/android/java/src... File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java (right): https://codereview.chromium.org/24072012/diff/37001/remoting/android/java/src... remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:322: synchronized (JniInterface.class) { I believe this will trigger a findbugs error. We discouraging synchronizing on a class object since they're effectively global and anyone can lock on it leading to possible deadlocks. Instead, use a private member variable and lock on that. https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_runtime.cc (right): https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_runtime.cc:177: jobject bitmap = env->CallStaticObjectMethod( Any reason not to start switching this over to the JNI generator? https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/jni_f... File remoting/client/jni/jni_frame_consumer.cc (right): https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/jni_f... remoting/client/jni/jni_frame_consumer.cc:123: jobject bitmap_global_ref = env->NewGlobalRef( On 2013/10/08 22:15:02, Sergey Ulanov wrote: > Do you need to call NewGlobalRef() here? ScopedJavaGlobalRef should call > NewGlobalRef() internally, so you will leak the reference by calling > NewGlobalRef() without corresponding DeleteGlobalRef(). This is why our base APIs are designed to return Scoped refs so it's clear and leak free.
Also fixed some order-of-destruction issues. ptal https://codereview.chromium.org/24072012/diff/37001/remoting/android/java/src... File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java (right): https://codereview.chromium.org/24072012/diff/37001/remoting/android/java/src... remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:322: synchronized (JniInterface.class) { On 2013/10/09 06:42:30, Yaron wrote: > I believe this will trigger a findbugs error. We discouraging synchronizing on a > class object since they're effectively global and anyone can lock on it leading > to possible deadlocks. Instead, use a private member variable and lock on that. There are already lots of these in this file. I've fixed this one, but fixing all the others is probably beyond the scope of this CL. Filed https://code.google.com/p/chromium/issues/detail?id=305770 https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_runtime.cc (right): https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_runtime.cc:177: jobject bitmap = env->CallStaticObjectMethod( On 2013/10/09 06:42:30, Yaron wrote: > Any reason not to start switching this over to the JNI generator? Can it be realistically done as part of this CL? I'd prefer not to hold up landing this CL on that work, as I've spent too much time on this already :) We have a bug to track that work here: https://code.google.com/p/chromium/issues/detail?id=304225 and I want to get to it ASAP. https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_runtime.h (right): https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_runtime.h:21: class DesktopFrame; On 2013/10/08 22:15:02, Sergey Ulanov wrote: > Not used in this file. Removed. https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_runtime.h:92: void UpdateFrameBitmap(jobject bitmap); On 2013/10/08 22:15:02, Sergey Ulanov wrote: > please comment that |bitmat| must be an android.graphics.Bitmap. Done. https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/jni_f... File remoting/client/jni/jni_frame_consumer.cc (right): https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/jni_f... remoting/client/jni/jni_frame_consumer.cc:49: view_size.height() > view_size_.height()) { On 2013/10/08 22:15:02, Sergey Ulanov wrote: > I think if you get different view_size you can just ignore the frame here - it > belongs to the previous generation, before SetSourceSize() called > SetOutputSizeAndClip(). Done. https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/jni_f... remoting/client/jni/jni_frame_consumer.cc:56: AllocateBuffer(); On 2013/10/08 22:15:02, Sergey Ulanov wrote: > This also calls DrawBuffer() internally. That doesn't look right to me. I think that's OK. Whenever you allocate a new buffer, you always want the FrameProducer to draw into it. https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/jni_f... remoting/client/jni/jni_frame_consumer.cc:100: frame_producer_->RequestReturnBuffers(base::Bind( On 2013/10/08 22:15:02, Sergey Ulanov wrote: > I'm not sure why we need this. Frame producer should return old buffers as > result of SetOutputSizeAndClip() anyway. Instead you can call AllocateBuffers > directly(). Done. https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/jni_f... remoting/client/jni/jni_frame_consumer.cc:123: jobject bitmap_global_ref = env->NewGlobalRef( On 2013/10/09 06:42:30, Yaron wrote: > On 2013/10/08 22:15:02, Sergey Ulanov wrote: > > Do you need to call NewGlobalRef() here? ScopedJavaGlobalRef should call > > NewGlobalRef() internally, so you will leak the reference by calling > > NewGlobalRef() without corresponding DeleteGlobalRef(). > > This is why our base APIs are designed to return Scoped refs so it's clear and > leak free. Ouch! I thought I checked this, but I messed up. So, IIUC, new ScopedJavaLocalRef<>(ref) takes ownership of |ref|. But new ScopedJavaGlobalRef<>(ref) does *not* take ownership of |ref| and instead creates a new global reference internally. I think the header comments should be improved to explain this. https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/jni_f... File remoting/client/jni/jni_frame_consumer.h (right): https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/jni_f... remoting/client/jni/jni_frame_consumer.h:69: scoped_ptr<gfx::JavaBitmap> bitmap_; On 2013/10/08 22:15:02, Sergey Ulanov wrote: > JavaBitmap holds bitmap content locked. I think it shouldn't be class member. > Instead you create it only when you need to modify a bitmap object. We need to keep the Bitmap pixels locked as long as the FrameProducer is decoding video data into that memory. So the lifetime of the JavaBitmap needs to be at least as long. Storing the JavaBitmap in a DesktopFrame-derived class might work, but seems a bit overkill since we're only creating one buffer at a time?
lgtm for original question (deps). Please wait for other reviewers https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/chrom... File remoting/client/jni/chromoting_jni_runtime.cc (right): https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/chrom... remoting/client/jni/chromoting_jni_runtime.cc:177: jobject bitmap = env->CallStaticObjectMethod( On 2013/10/10 01:35:58, Lambros wrote: > On 2013/10/09 06:42:30, Yaron wrote: > > Any reason not to start switching this over to the JNI generator? > > Can it be realistically done as part of this CL? I'd prefer not to hold up > landing this CL on that work, as I've spent too much time on this already :) We > have a bug to track that work here: > https://code.google.com/p/chromium/issues/detail?id=304225 and I want to get to > it ASAP. Ok fair enough https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/jni_f... File remoting/client/jni/jni_frame_consumer.cc (right): https://codereview.chromium.org/24072012/diff/37001/remoting/client/jni/jni_f... remoting/client/jni/jni_frame_consumer.cc:123: jobject bitmap_global_ref = env->NewGlobalRef( On 2013/10/10 01:35:58, Lambros wrote: > On 2013/10/09 06:42:30, Yaron wrote: > > On 2013/10/08 22:15:02, Sergey Ulanov wrote: > > > Do you need to call NewGlobalRef() here? ScopedJavaGlobalRef should call > > > NewGlobalRef() internally, so you will leak the reference by calling > > > NewGlobalRef() without corresponding DeleteGlobalRef(). > > > > This is why our base APIs are designed to return Scoped refs so it's clear and > > leak free. > Ouch! I thought I checked this, but I messed up. > > So, IIUC, new ScopedJavaLocalRef<>(ref) takes ownership of |ref|. But new > ScopedJavaGlobalRef<>(ref) does *not* take ownership of |ref| and instead > creates a new global reference internally. I think the header comments should be > improved to explain this. I think when used in concert with ScopedJavaLocalRef the simple/straightforward usage behaves as you'd want. The class comment for ScopedGlobalRef could possibly be improved. Please feel free to send something my way.
https://codereview.chromium.org/24072012/diff/45001/remoting/client/jni/jni_f... File remoting/client/jni/jni_frame_consumer.cc (right): https://codereview.chromium.org/24072012/diff/45001/remoting/client/jni/jni_f... remoting/client/jni/jni_frame_consumer.cc:27: in_dtor_ = true; Don't need this. https://codereview.chromium.org/24072012/diff/45001/remoting/client/jni/jni_f... remoting/client/jni/jni_frame_consumer.cc:32: base::Bind(&base::WaitableEvent::Signal, base::Unretained(&done_event))); This doesn't really cleanup the buffers. ReturnBuffers() will not be called by the proxy so we are leaking the frames. PepperView::~PepperView() deleting the frames in the queue, and so RequestReturnBuffers() is required to make sure they are not reused on the decode thread. In this case RequestReturnBuffers() doesn't really do anything. The leak can be fixed later, please TODO() and open a bug. I think you'll need to do something similar to PepperView - keep list of buffers and delete them here explicitly. https://codereview.chromium.org/24072012/diff/45001/remoting/client/jni/jni_f... remoting/client/jni/jni_frame_consumer.cc:65: webrtc::DesktopRect rect(i.rect()); nit: make this a reference. https://codereview.chromium.org/24072012/diff/45001/remoting/client/jni/jni_f... remoting/client/jni/jni_frame_consumer.cc:74: if (!in_dtor_) Don't need this. This method will not be called from the destructor. https://codereview.chromium.org/24072012/diff/45001/remoting/client/jni/jni_f... remoting/client/jni/jni_frame_consumer.cc:106: if (in_dtor_) Don't need this. https://codereview.chromium.org/24072012/diff/45001/remoting/client/jni/jni_f... remoting/client/jni/jni_frame_consumer.cc:110: webrtc::DesktopFrame* buffer = new webrtc::BasicDesktopFrame(size); nit: move this line just before DrawBuffer.
On 2013/10/11 20:32:17, Sergey Ulanov wrote: > https://codereview.chromium.org/24072012/diff/45001/remoting/client/jni/jni_f... > File remoting/client/jni/jni_frame_consumer.cc (right): > > https://codereview.chromium.org/24072012/diff/45001/remoting/client/jni/jni_f... > remoting/client/jni/jni_frame_consumer.cc:27: in_dtor_ = true; > Don't need this. > > https://codereview.chromium.org/24072012/diff/45001/remoting/client/jni/jni_f... > remoting/client/jni/jni_frame_consumer.cc:32: > base::Bind(&base::WaitableEvent::Signal, base::Unretained(&done_event))); > This doesn't really cleanup the buffers. ReturnBuffers() will not be called by > the proxy so we are leaking the frames. > PepperView::~PepperView() deleting the frames in the queue, and so > RequestReturnBuffers() is required to make sure they are not reused on the > decode thread. In this case RequestReturnBuffers() doesn't really do anything. > The leak can be fixed later, please TODO() and open a bug. I think you'll need > to do something similar to PepperView - keep list of buffers and delete them > here explicitly. > > https://codereview.chromium.org/24072012/diff/45001/remoting/client/jni/jni_f... > remoting/client/jni/jni_frame_consumer.cc:65: webrtc::DesktopRect > rect(i.rect()); > nit: make this a reference. > > https://codereview.chromium.org/24072012/diff/45001/remoting/client/jni/jni_f... > remoting/client/jni/jni_frame_consumer.cc:74: if (!in_dtor_) > Don't need this. This method will not be called from the destructor. > > https://codereview.chromium.org/24072012/diff/45001/remoting/client/jni/jni_f... > remoting/client/jni/jni_frame_consumer.cc:106: if (in_dtor_) > Don't need this. > > https://codereview.chromium.org/24072012/diff/45001/remoting/client/jni/jni_f... > remoting/client/jni/jni_frame_consumer.cc:110: webrtc::DesktopFrame* buffer = > new webrtc::BasicDesktopFrame(size); > nit: move this line just before DrawBuffer. What's the status of this patch? Without it, the blue screen regions bug runs rampant. Let me know if you want me to re-review it.
Sergey and Sol: awaiting your LGs :) https://codereview.chromium.org/24072012/diff/45001/remoting/client/jni/jni_f... File remoting/client/jni/jni_frame_consumer.cc (right): https://codereview.chromium.org/24072012/diff/45001/remoting/client/jni/jni_f... remoting/client/jni/jni_frame_consumer.cc:27: in_dtor_ = true; On 2013/10/11 20:32:17, Sergey Ulanov wrote: > Don't need this. Done. https://codereview.chromium.org/24072012/diff/45001/remoting/client/jni/jni_f... remoting/client/jni/jni_frame_consumer.cc:32: base::Bind(&base::WaitableEvent::Signal, base::Unretained(&done_event))); On 2013/10/11 20:32:17, Sergey Ulanov wrote: > This doesn't really cleanup the buffers. ReturnBuffers() will not be called by > the proxy so we are leaking the frames. > PepperView::~PepperView() deleting the frames in the queue, and so > RequestReturnBuffers() is required to make sure they are not reused on the > decode thread. In this case RequestReturnBuffers() doesn't really do anything. > The leak can be fixed later, please TODO() and open a bug. I think you'll need > to do something similar to PepperView - keep list of buffers and delete them > here explicitly. Done. https://codereview.chromium.org/24072012/diff/45001/remoting/client/jni/jni_f... remoting/client/jni/jni_frame_consumer.cc:65: webrtc::DesktopRect rect(i.rect()); On 2013/10/11 20:32:17, Sergey Ulanov wrote: > nit: make this a reference. Done. https://codereview.chromium.org/24072012/diff/45001/remoting/client/jni/jni_f... remoting/client/jni/jni_frame_consumer.cc:74: if (!in_dtor_) On 2013/10/11 20:32:17, Sergey Ulanov wrote: > Don't need this. This method will not be called from the destructor. Done. https://codereview.chromium.org/24072012/diff/45001/remoting/client/jni/jni_f... remoting/client/jni/jni_frame_consumer.cc:106: if (in_dtor_) On 2013/10/11 20:32:17, Sergey Ulanov wrote: > Don't need this. Done. https://codereview.chromium.org/24072012/diff/45001/remoting/client/jni/jni_f... remoting/client/jni/jni_frame_consumer.cc:110: webrtc::DesktopFrame* buffer = new webrtc::BasicDesktopFrame(size); On 2013/10/11 20:32:17, Sergey Ulanov wrote: > nit: move this line just before DrawBuffer. Done.
LGTM, thanks for fixing the leak!
On 2013/10/15 23:55:16, Sergey Ulanov wrote: > LGTM, thanks for fixing the leak! LGTM as well; everything looks great! And thanks for taking care of that leak.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/24072012/5...
Message was sent while issue was closed.
Change committed as 228898 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
