Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1988183002:
Reimplement PooledI420Buffer, without an extra indirection. (Closed)
|
Created:
4 years, 7 months ago by nisse-webrtc Modified:
4 years, 6 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionReimplement PooledI420Buffer, without an extra indirection.
Adds a method RefCountedObject::GetRefCount.
Let PooledI420Buffer inherit the plain I420Buffer, and override
IsMutable to allow two references to managed buffers. Only tricky
issue is to support the pool Release method.
BUG=webrtc:5921, webrtc:5682
Patch Set 1 #Patch Set 2 : Fix IsFree logic. Comment improvements. #Patch Set 3 : Add destructor, calling Release(). #
Total comments: 12
Patch Set 4 : Addressed pbos' comments, except thread checker. #
Total comments: 1
Patch Set 5 : Fix missing-parens bug. #Patch Set 6 : Document IsMutable race issues, and the plan for its removal. #
Total comments: 5
Patch Set 7 : Added GetRefCount TODO comment. #
Messages
Total messages: 38 (12 generated)
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988183002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988183002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/15136) win_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/14808)
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988183002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988183002/20001
nisse@webrtc.org changed reviewers: + magjed@webrtc.org, pbos@webrtc.org, tommi@webrtc.org
This is a change I've been considering for some time. Main motivation to do it now is to get the buffer pool to return a (subclass of) I420Buffer. We should strive to have all pixel data mutation apply to I420Buffers only, and move the common VideoFrameBuffer interface read-only. Helper methods, e.g., scaling, make a lot more sense on an I420Buffer than on a VideoFrameBuffer or (even worse) cricket::VideoFrame. I really need review from someone more familiar with the AtomicOps semantics. See the comment on I420BufferPool::PooledI420Buffer::IsMutable() for the race which must be handled correctly. Memory barriers are needed to ensure that the stores to managed_ and ref_count_ always become visible to other threads in the right order. As for the new GetRefCount method, I think that's going to be independently useful for debugging. Another question: The public method I420BufferPool::Release appear to not be used outside of this class. Should it be demoted to private?
Description was changed from ========== Reimplement PooledI420Buffer, without an extra indirection. Adds a method RefCountedObject::GetRefCount. Let PooledI420Buffer inherit the plain I420Buffer, and override IsMutable to allow two references to managed buffers. Only tricky issue is to support the pool Release method. BUG= ========== to ========== Reimplement PooledI420Buffer, without an extra indirection. Adds a method RefCountedObject::GetRefCount. Let PooledI420Buffer inherit the plain I420Buffer, and override IsMutable to allow two references to managed buffers. Only tricky issue is to support the pool Release method. BUG=webrtc:5682 ==========
On 2016/05/19 08:26:34, nisse-webrtc wrote: > I really need review from someone more familiar with the AtomicOps semantics. > See the comment on I420BufferPool::PooledI420Buffer::IsMutable() for the race > which must be handled correctly. Memory barriers are needed to ensure that the > stores to managed_ and ref_count_ always become visible to other threads in the > right order. This sounds very complicated. Can't we make VideoFrameBuffer read-only and only have the MutableData functions in I420Buffer instead? We don't need IsMutable or HasOneRef checks - the type I420Buffer would imply non-const. We create mutable I420Buffers directly with the ctor or with a pool, set the pixel values once and for all, and upcast it to a read-only VideoFrameBuffer. > Another question: The public method I420BufferPool::Release appear to not be > used outside of this class. Should it be demoted to private? It's used from vp8_impl.cc according to code search.
On 2016/05/19 09:16:39, magjed_webrtc wrote: > This sounds very complicated. The only thing which is complicated is to make IsMutable work reliably after the pool is destroyed or released. If eventually we get rid of IsMutable, that complication goes away. > Can't we make VideoFrameBuffer read-only and only > have the MutableData functions in I420Buffer instead? We should. Problem is that what's returned from I420BufferPool::CreateBuffer ought to be mutable, but before this cl, the return value isn't an I420Buffer at all, only a proxy class, PooledI420Buffer, a direct subclass of VideoFrameBuffer. So the main point of this cl is to change I420PooledBuffer from a proxy to a subclass. > > Another question: The public method I420BufferPool::Release appear to not be > > used outside of this class. Should it be demoted to private? > It's used from vp8_impl.cc according to code search. Thanks, I missed that one for some reason. So it remains public (and it is needed anyway, called by the destructor).
On 2016/05/19 09:38:58, nisse-webrtc wrote: > On 2016/05/19 09:16:39, magjed_webrtc wrote: > > > This sounds very complicated. > > The only thing which is complicated is to make IsMutable work reliably after the > pool is destroyed or released. If eventually we get rid of IsMutable, that > complication goes away. Why can't we get rid of IsMutable in this CL? > We should. Problem is that what's returned from I420BufferPool::CreateBuffer > ought to be mutable, but before this cl, the return value isn't an I420Buffer at > all, only a proxy class, PooledI420Buffer, a direct subclass of > VideoFrameBuffer. > > So the main point of this cl is to change I420PooledBuffer from a proxy to a > subclass. I understand that, but if you remove IsMutable, you don't need a subclass at all in I420PooledBuffer and can return I420Buffers directly.
On 2016/05/19 09:56:54, magjed_webrtc wrote: >> Why can't we get rid of IsMutable in this CL? I'm afraid there's lots of code modifying a VideoFrame using ->video_frame_buffer()->MutableDataY(). All such callers must be modified to keep a pointer to the I420Buffer they own. Doing this change first makes it possible to change webrtc itself to only mutate I420Buffers, and iron out any issues with that, before fixing all applications. > I understand that, but if you remove IsMutable, you don't need a subclass at all > in I420PooledBuffer and can return I420Buffers directly. Agreed.
https://codereview.webrtc.org/1988183002/diff/40001/webrtc/common_video/i420_... File webrtc/common_video/i420_buffer_pool.cc (right): https://codereview.webrtc.org/1988183002/diff/40001/webrtc/common_video/i420_... webrtc/common_video/i420_buffer_pool.cc:46: int I420BufferPool::PooledI420Buffer::IsManaged() const { bool, even if that means you have to do IsManaged() ? 1 : 0 https://codereview.webrtc.org/1988183002/diff/40001/webrtc/common_video/i420_... webrtc/common_video/i420_buffer_pool.cc:47: return rtc::AtomicOps::AcquireLoad(&managed_); == 1 https://codereview.webrtc.org/1988183002/diff/40001/webrtc/common_video/i420_... webrtc/common_video/i420_buffer_pool.cc:51: bool is_free = (GetRefCount() == 1); Add a thread_checker_ to IsFree and UnManage. https://codereview.webrtc.org/1988183002/diff/40001/webrtc/common_video/i420_... webrtc/common_video/i420_buffer_pool.cc:91: // If the buffer is in use, the ref count will be 2, one from the list we This comment barely applies to the current implementation since we don't check refcount right here either way, I think you can remove the comment completely. https://codereview.webrtc.org/1988183002/diff/40001/webrtc/common_video/i420_... webrtc/common_video/i420_buffer_pool.cc:99: rtc::scoped_refptr<PooledI420Buffer> buffer = do buffer(new ...) for initialization here. https://codereview.webrtc.org/1988183002/diff/40001/webrtc/common_video/i420_... webrtc/common_video/i420_buffer_pool.cc:101: if (zero_initialize_) Do we need the zero_initialize_ argument ever?
https://codereview.webrtc.org/1988183002/diff/40001/webrtc/common_video/i420_... File webrtc/common_video/i420_buffer_pool.cc (right): https://codereview.webrtc.org/1988183002/diff/40001/webrtc/common_video/i420_... webrtc/common_video/i420_buffer_pool.cc:46: int I420BufferPool::PooledI420Buffer::IsManaged() const { On 2016/05/19 15:03:22, pbos-webrtc wrote: > bool, even if that means you have to do IsManaged() ? 1 : 0 Done. https://codereview.webrtc.org/1988183002/diff/40001/webrtc/common_video/i420_... webrtc/common_video/i420_buffer_pool.cc:47: return rtc::AtomicOps::AcquireLoad(&managed_); On 2016/05/19 15:03:21, pbos-webrtc wrote: > == 1 Done. https://codereview.webrtc.org/1988183002/diff/40001/webrtc/common_video/i420_... webrtc/common_video/i420_buffer_pool.cc:51: bool is_free = (GetRefCount() == 1); On 2016/05/19 15:03:22, pbos-webrtc wrote: > Add a thread_checker_ to IsFree and UnManage. At first I thought that it's kind-of redundant. The methods are called from the buffer bool, which has a thread checker of its own. And even if the methods formally are "public", they belong to a private class so no one else should be able to access them. But in fact, UnManaged is called by Release, which is called by the pool destructor. Which may be called on some other thread. Example backtrace after adding a thread checker: #2 0x000000000075e792 in rtc::FatalMessage::~FatalMessage (this=0x7fffffffc190) at ../../webrtc/base/checks.cc:109 #3 0x0000000000f5abdd in webrtc::I420BufferPool::PooledI420Buffer::UnManage ( this=0x7fffd8000d60) at ../../webrtc/common_video/i420_buffer_pool.cc:59 #4 0x0000000000f5ad39 in webrtc::I420BufferPool::Release (this=0x14e4520) at ../../webrtc/common_video/i420_buffer_pool.cc:74 #5 0x0000000000f5ac89 in webrtc::I420BufferPool::~I420BufferPool (this=0x14e4520) at ../../webrtc/common_video/i420_buffer_pool.cc:69 #6 0x0000000000f5c9e9 in webrtc::Scaler::~Scaler (this=0x14e4508) at ../../webrtc/common_video/libyuv/scaler.cc:28 #7 0x0000000000bea251 in webrtc::VPMSimpleSpatialResampler::~VPMSimpleSpatialResampler (this=0x14e44f0) at ../../webrtc/modules/video_processing/spatial_resampler.cc:21 #8 0x0000000000bea289 in webrtc::VPMSimpleSpatialResampler::~VPMSimpleSpatialResampler (this=0x14e44f0) at ../../webrtc/modules/video_processing/spatial_resampler.cc:21 #9 0x0000000000be9527 in webrtc::VPMFramePreprocessor::~VPMFramePreprocessor ( this=0x14e4450) at ../../webrtc/modules/video_processing/frame_preprocessor.cc:28 #10 0x0000000000be8fe1 in webrtc::VideoProcessingImpl::~VideoProcessingImpl ( this=0x14e4410) at ../../webrtc/modules/video_processing/video_processing_impl.cc:26 #11 0x0000000000be9029 in webrtc::VideoProcessingImpl::~VideoProcessingImpl ( this=0x14e4410) at ../../webrtc/modules/video_processing/video_processing_impl.cc:26 ... #15 0x00000000008f6eec in webrtc::ViEEncoder::~ViEEncoder (this=0x14e3330) at ../../webrtc/video/vie_encoder.cc:64 #16 0x00000000008f0b6c in webrtc::internal::VideoSendStream::~VideoSendStream ( this=0x14e2ac0) at ../../webrtc/video/video_send_stream.cc:492 #17 0x00000000008f0d89 in webrtc::internal::VideoSendStream::~VideoSendStream ( this=0x14e2ac0) at ../../webrtc/video/video_send_stream.cc:468 ... #21 0x0000000000691eed in cricket::WebRtcVideoChannel2::RemoveSendStream ( this=0x14e0830, ssrc=1234) at ../../webrtc/media/engine/webrtcvideoengine2.cc:1160 #22 0x00000000004f1d62 in cricket::WebRtcVideoEngine2Test_UseExternalFactoryForVp8WhenSupported_Test::TestBody (this=0x13dec70) at ../../webrtc/media/engine/webrtcvideoengine2_unittest.cc:391 So it seems we have to tolerate UnManaged being called from any arbitrary thread. I could still add a thread checker for IsFree only if you like, be to me it seems a bit pointless. https://codereview.webrtc.org/1988183002/diff/40001/webrtc/common_video/i420_... webrtc/common_video/i420_buffer_pool.cc:91: // If the buffer is in use, the ref count will be 2, one from the list we On 2016/05/19 15:03:21, pbos-webrtc wrote: > This comment barely applies to the current implementation since we don't check > refcount right here either way, I think you can remove the comment completely. Removed, and added a shorter comment to IsFree. https://codereview.webrtc.org/1988183002/diff/40001/webrtc/common_video/i420_... webrtc/common_video/i420_buffer_pool.cc:99: rtc::scoped_refptr<PooledI420Buffer> buffer = On 2016/05/19 15:03:21, pbos-webrtc wrote: > do buffer(new ...) for initialization here. Done. https://codereview.webrtc.org/1988183002/diff/40001/webrtc/common_video/i420_... webrtc/common_video/i420_buffer_pool.cc:101: if (zero_initialize_) On 2016/05/19 15:03:22, pbos-webrtc wrote: > Do we need the zero_initialize_ argument ever? Comment says it's ffmpeg-related. It was added by hbos in cl https://codereview.webrtc.org/1645543003, used by H264DecoderImpl. So I'm afraid we have to keep it for now.
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988183002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988183002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/15182) win_x64_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn_rel/builds/9397)
https://codereview.webrtc.org/1988183002/diff/60001/webrtc/common_video/i420_... File webrtc/common_video/i420_buffer_pool.cc (right): https://codereview.webrtc.org/1988183002/diff/60001/webrtc/common_video/i420_... webrtc/common_video/i420_buffer_pool.cc:42: return refs <= IsManaged() ? 2 : 1; This is somewhat scary missing-parens-bug...
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988183002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988183002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_ubsan_vptr on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
I've extended the comments, including the plan for eventual removal of IsMutable. There remains a race where IsMutable may return false negative if called at an unfortunate moment. Not sure if the race can happen with the current code. Could happen if IsMutable is called by anyone not on the thread owning the pool (do we ever do that?), or if called by the owning thread at the same time as some other thread destroys the pool. I think I and pbos believe that false-positive race is excluded, but I'd like to have a third opinion, since this is a bit tricky and I don't fully understand the C++ memory model and the semantics of AtomicOps.
The CQ bit was checked by nisse@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1988183002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1988183002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
https://codereview.webrtc.org/1988183002/diff/100001/webrtc/base/refcount.h File webrtc/base/refcount.h (right): https://codereview.webrtc.org/1988183002/diff/100001/webrtc/base/refcount.h#n... webrtc/base/refcount.h:114: virtual int GetRefCount() const { I think this method should also be going away, so pref adding a TODO to remove it here? https://codereview.webrtc.org/1988183002/diff/100001/webrtc/common_video/i420... File webrtc/common_video/i420_buffer_pool.cc (right): https://codereview.webrtc.org/1988183002/diff/100001/webrtc/common_video/i420... webrtc/common_video/i420_buffer_pool.cc:80: return refs <= (IsManaged() ? 2 : 1); I think we can avoid the transient state: int refs = GetRefCount(); if (!IsManaged()) { // Not managed, since management could've disappeared between GetRefCount and IsManaged call we need to check the refcount again to make sure it matches the unmanaged state. return GetRefCount() == 1; } return refs <= 2; But write a better comment.
Description was changed from ========== Reimplement PooledI420Buffer, without an extra indirection. Adds a method RefCountedObject::GetRefCount. Let PooledI420Buffer inherit the plain I420Buffer, and override IsMutable to allow two references to managed buffers. Only tricky issue is to support the pool Release method. BUG=webrtc:5682 ========== to ========== Reimplement PooledI420Buffer, without an extra indirection. Adds a method RefCountedObject::GetRefCount. Let PooledI420Buffer inherit the plain I420Buffer, and override IsMutable to allow two references to managed buffers. Only tricky issue is to support the pool Release method. BUG=webrtc:5921,webrtc:5682 ==========
https://codereview.webrtc.org/1988183002/diff/100001/webrtc/base/refcount.h File webrtc/base/refcount.h (right): https://codereview.webrtc.org/1988183002/diff/100001/webrtc/base/refcount.h#n... webrtc/base/refcount.h:114: virtual int GetRefCount() const { On 2016/05/20 13:32:21, pbos-webrtc wrote: > I think this method should also be going away, so pref adding a TODO to remove > it here? I can add a TODO like "Currently used only for I420PooledBuffer::IsMutable, consider deleting when IsMutable is deleted." But I think it might be useful for debugging, I remember I've been wanting to call it from gdb sometime. https://codereview.webrtc.org/1988183002/diff/100001/webrtc/common_video/i420... File webrtc/common_video/i420_buffer_pool.cc (right): https://codereview.webrtc.org/1988183002/diff/100001/webrtc/common_video/i420... webrtc/common_video/i420_buffer_pool.cc:80: return refs <= (IsManaged() ? 2 : 1); On 2016/05/20 13:32:21, pbos-webrtc wrote: > I think we can avoid the transient state: > > int refs = GetRefCount(); > if (!IsManaged()) { > // Not managed, since management could've disappeared between GetRefCount and > IsManaged call we need to check the refcount again to make sure it matches the > unmanaged state. > return GetRefCount() == 1; > } > return refs <= 2; > > But write a better comment. That will only reduce the probability of false negative. We will usually not stay in the transient state (refs_ == 2, managed_ == 0) for a long time, but the thread calling UnManage could well be preempted so that it's a more or less arbitrary delay between the assignment to managed_ and refs_ dropping to 1. And until that thread makes progress, rereading the reference count makes no difference. If we want to eliminate the race, I'm afraid we'd need a way to wait until the transient state is exited, e.g., using a critical section to protect access to the managed_ flag, and arrange things so that we clear the flag and drop the reference before leaving the critical section. Another critical section here might be ok, IsMutable and UnManaged shouldn't be very performance critical, except that IsMutable is used in a bunch of RTC_DCHECKS. I can try that if you really want, but I think we can live with the false-negative race for the time being. I just filed a bug to track removal of IsMutable.
https://codereview.webrtc.org/1988183002/diff/100001/webrtc/common_video/i420... File webrtc/common_video/i420_buffer_pool.cc (right): https://codereview.webrtc.org/1988183002/diff/100001/webrtc/common_video/i420... webrtc/common_video/i420_buffer_pool.cc:80: return refs <= (IsManaged() ? 2 : 1); On 2016/05/23 07:35:06, nisse-webrtc wrote: > On 2016/05/20 13:32:21, pbos-webrtc wrote: > > I think we can avoid the transient state: > > > > int refs = GetRefCount(); > > if (!IsManaged()) { > > // Not managed, since management could've disappeared between GetRefCount > and > > IsManaged call we need to check the refcount again to make sure it matches the > > unmanaged state. > > return GetRefCount() == 1; > > } > > return refs <= 2; > > > > But write a better comment. > > That will only reduce the probability of false negative. We will usually not > stay in the transient state (refs_ == 2, managed_ == 0) for a long time, but the > thread calling UnManage could well be preempted so that it's a more or less > arbitrary delay between the assignment to managed_ and refs_ dropping to 1. And > until that thread makes progress, rereading the reference count makes no > difference. > > If we want to eliminate the race, I'm afraid we'd need a way to wait until the > transient state is exited, e.g., using a critical section to protect access to > the managed_ flag, and arrange things so that we clear the flag and drop the > reference before leaving the critical section. > > Another critical section here might be ok, IsMutable and UnManaged shouldn't be > very performance critical, except that IsMutable is used in a bunch of > RTC_DCHECKS. I can try that if you really want, but I think we can live with the > false-negative race for the time being. I just filed a bug to track removal of > IsMutable. Right, I thought the reference was dropped before managed_. Never mind then. :\
Ping. What can I do for you to make progress on this cl?
On 2016/05/25 07:38:29, nisse-webrtc wrote: > Ping. > > What can I do for you to make progress on this cl? who is being pinged?
On 2016/05/25 08:28:44, tommi-webrtc wrote: > On 2016/05/25 07:38:29, nisse-webrtc wrote: > > Ping. > > > > What can I do for you to make progress on this cl? > > who is being pinged? Tommi can you take a look and then I'll take a final look depending on comments that you have. :)
On 2016/05/25 10:31:46, pbos-webrtc wrote: > On 2016/05/25 08:28:44, tommi-webrtc wrote: > > On 2016/05/25 07:38:29, nisse-webrtc wrote: > > > Ping. > > > > > > What can I do for you to make progress on this cl? > > > > who is being pinged? > > Tommi can you take a look and then I'll take a final look depending on comments > that you have. :) I'm trying a different approach now... If that works out, GetRefCount isn't needed. See https://codereview.webrtc.org/2009193002/
On 2016/05/25 11:00:55, nisse-webrtc wrote: > On 2016/05/25 10:31:46, pbos-webrtc wrote: > > On 2016/05/25 08:28:44, tommi-webrtc wrote: > > > On 2016/05/25 07:38:29, nisse-webrtc wrote: > > > > Ping. > > > > > > > > What can I do for you to make progress on this cl? > > > > > > who is being pinged? > > > > Tommi can you take a look and then I'll take a final look depending on > comments > > that you have. :) > > I'm trying a different approach now... If that works out, GetRefCount isn't > needed. See https://codereview.webrtc.org/2009193002/ Closing this now, superseded by the other cl. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
