Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1688033005:
Monitor VideoResourceUpdater reusing destructed resource in Debug mode. (Closed)
DescriptionMonitor VideoResourceUpdater reusing destructed resource in Debug mode.
VideoResourceUpdater software planes use VideoFrame raw pointer and
video timestamp as the unique ID for each VideoFrame. However, if the
VideoFrame is destructed and the pointer is reused by a new VideoFrame,
and if the timestamp is not correctly set, the resource maybe
incorrectly reused. This CL marks the resource as destructed when the
original VideoFrame is destructed, and monitors if the destructed
resource is re-used. The CL only takes effect in Debug mode.
BUG=581480
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/66b273c41ace18613ce65175bbcf47f9bd3ddf94
Cr-Commit-Position: refs/heads/master@{#381739}
Patch Set 1 : #
Total comments: 2
Patch Set 2 : Rebase and add new VideoFrameMetada field. #
Total comments: 8
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Total comments: 4
Patch Set 5 : Add VideoFrame destructor callback in Debug mode. #
Total comments: 6
Patch Set 6 : Address miu's comments. #
Total comments: 11
Patch Set 7 : #
Total comments: 2
Patch Set 8 : Rebase and address danakj's comments. #
Messages
Total messages: 55 (21 generated)
Description was changed from ========== Add VideoFrame destructor callback to invalid cashed resources in VideoResourceUpdater. BUG=581480 ========== to ========== Add VideoFrame destructor callback to invalid cashed resources in VideoResourceUpdater. BUG=581480 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
xjz@chromium.org changed reviewers: + miu@chromium.org
PTAL
https://codereview.chromium.org/1688033005/diff/40001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/1688033005/diff/40001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:120: task_runner->PostTask(FROM_HERE, task); This will introduce an unknown time delay before the VideoFrame is invalidated. It'd be rare, but not impossible for a new VideoFrame to be created with the same pointer in the meantime...and even harder bug to catch! ;-) Instead of this approach, how about this idea: 1. Add a new VideoFrameMetadata field called RESOURCE_UPDATER_FLAG. 2. When a VideoFrame is passed in a call to SetPlaneResourceUniqueId(), set the VideoFrame's RESOURCE_UPDATER_FLAG metadata to PlaneResource::resource_id. 3. When a VideoFrame is passed in a call to PlaneResourceMatchesUniqueID(), it should also check that the RESOURCE_UPDATER_FLAG field is set to the same value as PlaneResource::resource_id. (and add a comment about why the pointer+timestamp sometimes won't work as a unique identifier). That way, if a VideoFrame is destroyed and re-created with the same pointer, AND the client always sets the timestamp to zero; it's still possible for VideoResourceUpdater to determine that this is a new VideoFrame (because it's FLAG value will be missing).
Patchset #2 (id:60001) has been deleted
liberato@chromium.org changed reviewers: + liberato@chromium.org
drive-by review. please feel free to ignore. thanks -fl https://codereview.chromium.org/1688033005/diff/80001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/1688033005/diff/80001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:135: if (plane_resource.frame_ptr == video_frame && i'm not sure that checking all of these things together is needed. with the new flag, isn't |video_frame| enough to guarantee it? https://codereview.chromium.org/1688033005/diff/80001/media/cast/sender/frame... File media/cast/sender/frame_sender.cc (right): https://codereview.chromium.org/1688033005/diff/80001/media/cast/sender/frame... media/cast/sender/frame_sender.cc:385: picture_lst_at_receiver_bool_ = true; i don't see this used anywhere else, or initialized. what's it for? actually, i don't see any calls to OnReceivedPli, either.
Patchset #2 (id:80001) has been deleted
liberato: sorry missed your comments before deleting the old Patch Set 2. Some files were uploaded by mistake in that old patch. I replied your comments in line. Thanks. On 2016/02/19 23:50:52, liberato wrote: > drive-by review. please feel free to ignore. > > thanks > -fl > > https://codereview.chromium.org/1688033005/diff/80001/cc/resources/video_reso... > File cc/resources/video_resource_updater.cc (right): > > https://codereview.chromium.org/1688033005/diff/80001/cc/resources/video_reso... > cc/resources/video_resource_updater.cc:135: if (plane_resource.frame_ptr == > video_frame && > i'm not sure that checking all of these things together is needed. with the new > flag, isn't |video_frame| enough to guarantee it? The new flag only indicates that the VideoFrame is in the resources. We still need all those checkings to know whether it is this resource. > > https://codereview.chromium.org/1688033005/diff/80001/media/cast/sender/frame... > File media/cast/sender/frame_sender.cc (right): > > https://codereview.chromium.org/1688033005/diff/80001/media/cast/sender/frame... > media/cast/sender/frame_sender.cc:385: picture_lst_at_receiver_bool_ = true; > i don't see this used anywhere else, or initialized. what's it for? actually, > i don't see any calls to OnReceivedPli, either. This file was uploaded by mistake. It has nothing to do with this issue.
PTAL https://codereview.chromium.org/1688033005/diff/40001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/1688033005/diff/40001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:120: task_runner->PostTask(FROM_HERE, task); On 2016/02/19 02:47:46, miu wrote: > This will introduce an unknown time delay before the VideoFrame is invalidated. > It'd be rare, but not impossible for a new VideoFrame to be created with the > same pointer in the meantime...and even harder bug to catch! ;-) > > Instead of this approach, how about this idea: > > 1. Add a new VideoFrameMetadata field called RESOURCE_UPDATER_FLAG. > > 2. When a VideoFrame is passed in a call to SetPlaneResourceUniqueId(), set the > VideoFrame's RESOURCE_UPDATER_FLAG metadata to PlaneResource::resource_id. > > 3. When a VideoFrame is passed in a call to PlaneResourceMatchesUniqueID(), it > should also check that the RESOURCE_UPDATER_FLAG field is set to the same value > as PlaneResource::resource_id. (and add a comment about why the > pointer+timestamp sometimes won't work as a unique identifier). > > That way, if a VideoFrame is destroyed and re-created with the same pointer, AND > the client always sets the timestamp to zero; it's still possible for > VideoResourceUpdater to determine that this is a new VideoFrame (because it's > FLAG value will be missing). Done. Since each VideoFrame can have three resource_ids, instead of setting VideoFrame's RESOURCE_UPDATER_FLAG metadata to PlaneResource::resource_id, setting it to true in SetPlaneResourceUniqueId().
https://codereview.chromium.org/1688033005/diff/100001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/1688033005/diff/100001/cc/resources/video_res... cc/resources/video_resource_updater.cc:138: bool video_frame_in_resource; There's a helper method in VideoFrameMetadata to avoid having to do all this. You can just: return plane_resource.frame_ptr == video_frame && video_frame->metadata()->IsTrue( media::VideoFrameMetadata::RESOURCE_UPDATER_FLAG) && plane_resource.plane_index == plane_index && plane_resource.timestamp == video_frame->timestamp(); https://codereview.chromium.org/1688033005/diff/100001/cc/resources/video_res... File cc/resources/video_resource_updater.h (right): https://codereview.chromium.org/1688033005/diff/100001/cc/resources/video_res... cc/resources/video_resource_updater.h:114: static void SetPlaneResourceUniqueId(media::VideoFrame* video_frame, Please add a comment like: // Side effect: Sets RESOURCE_UPDATER_FLAG boolean in the video frame's metadata. https://codereview.chromium.org/1688033005/diff/100001/media/base/video_frame... File media/base/video_frame_metadata.h (right): https://codereview.chromium.org/1688033005/diff/100001/media/base/video_frame... media/base/video_frame_metadata.h:97: // This field indicates if the VideoFrame content has uploaded as This is a nice comment, but it belongs in VideoResourceUpdater::SetPlaneResourceUniqueId() (where you set the boolan) instead. These details are specific to cc::VideoResourceUpdater, so they don't belong here. Here, you should probably just say: "Boolean used by cc::VideoResourceUpdater to flag that this video frame's content has been cached." https://codereview.chromium.org/1688033005/diff/100001/media/base/video_frame... media/base/video_frame_metadata.h:104: RESOURCE_UPDATER_FLAG, Please move this up (to before RESOURCE_UTILIZATION) to maintain alphabetical order.
Patchset #3 (id:120001) has been deleted
PTAL https://codereview.chromium.org/1688033005/diff/100001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/1688033005/diff/100001/cc/resources/video_res... cc/resources/video_resource_updater.cc:138: bool video_frame_in_resource; On 2016/02/22 21:17:37, miu wrote: > There's a helper method in VideoFrameMetadata to avoid having to do all this. > You can just: > > return plane_resource.frame_ptr == video_frame && > video_frame->metadata()->IsTrue( > media::VideoFrameMetadata::RESOURCE_UPDATER_FLAG) && > plane_resource.plane_index == plane_index && > plane_resource.timestamp == video_frame->timestamp(); > Done. https://codereview.chromium.org/1688033005/diff/100001/cc/resources/video_res... File cc/resources/video_resource_updater.h (right): https://codereview.chromium.org/1688033005/diff/100001/cc/resources/video_res... cc/resources/video_resource_updater.h:114: static void SetPlaneResourceUniqueId(media::VideoFrame* video_frame, On 2016/02/22 21:17:37, miu wrote: > Please add a comment like: > > // Side effect: Sets RESOURCE_UPDATER_FLAG boolean in the video frame's > metadata. Done. https://codereview.chromium.org/1688033005/diff/100001/media/base/video_frame... File media/base/video_frame_metadata.h (right): https://codereview.chromium.org/1688033005/diff/100001/media/base/video_frame... media/base/video_frame_metadata.h:97: // This field indicates if the VideoFrame content has uploaded as On 2016/02/22 21:17:37, miu wrote: > This is a nice comment, but it belongs in > VideoResourceUpdater::SetPlaneResourceUniqueId() (where you set the boolan) > instead. These details are specific to cc::VideoResourceUpdater, so they don't > belong here. > > Here, you should probably just say: "Boolean used by cc::VideoResourceUpdater to > flag that this video frame's content has been cached." Done. https://codereview.chromium.org/1688033005/diff/100001/media/base/video_frame... media/base/video_frame_metadata.h:104: RESOURCE_UPDATER_FLAG, On 2016/02/22 21:17:37, miu wrote: > Please move this up (to before RESOURCE_UTILIZATION) to maintain alphabetical > order. Done.
lgtm
On 2016/02/23 03:40:55, miu wrote: > lgtm Please rewrite the change description to match the new approach. :)
Description was changed from ========== Add VideoFrame destructor callback to invalid cashed resources in VideoResourceUpdater. BUG=581480 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Add a new VideoFrameMetadata field called RESOURCE_UPDATER_FLAG to indicate that the VideoFrame's content has been cached as resource by cc::VideoResourceUpdater. BUG=581480 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
The CQ bit was checked by xjz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688033005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688033005/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
xjz@chromium.org changed reviewers: + xhwang@chromium.org
xhwang: needs owner's approval on video_frame_metadata.h. PTAL.
https://chromiumcodereview.appspot.com/1688033005/diff/140001/cc/resources/vi... File cc/resources/video_resource_updater.h (right): https://chromiumcodereview.appspot.com/1688033005/diff/140001/cc/resources/vi... cc/resources/video_resource_updater.h:118: // by the client. One question: Is it possible to require the timestamp to always be set (and hopefully they are unique?) so we don't need an extra flag to fix this?
xjz@chromium.org changed reviewers: + brianderson@chromium.org
brianderson: PTAL. Need owner's approval on VideoResourceUpdater change.
https://chromiumcodereview.appspot.com/1688033005/diff/140001/media/base/vide... File media/base/video_frame_metadata.h (right): https://chromiumcodereview.appspot.com/1688033005/diff/140001/media/base/vide... media/base/video_frame_metadata.h:81: RESOURCE_UPDATER_FLAG, Also, media/base knows nothing about "cc" and "ResourceUpdater", can we have a more meaningful name for this flag?
Removed the new field in VideoFrameMetada. Instead, used base::SupportsUserData to set the userdata in VideoFrame as miu suggested. PTAL. https://codereview.chromium.org/1688033005/diff/140001/cc/resources/video_res... File cc/resources/video_resource_updater.h (right): https://codereview.chromium.org/1688033005/diff/140001/cc/resources/video_res... cc/resources/video_resource_updater.h:118: // by the client. On 2016/02/23 17:48:51, xhwang wrote: > One question: Is it possible to require the timestamp to always be set (and > hopefully they are unique?) so we don't need an extra flag to fix this? It is difficult to check whether the timestamp is correctly set (unique for each frame) here. If not, the incorrectly reusing of the old resource may happen. And once it happens, it is difficult to debug. It makes sense to set and check the status in VideoResourceUpdater. https://codereview.chromium.org/1688033005/diff/140001/media/base/video_frame... File media/base/video_frame_metadata.h (right): https://codereview.chromium.org/1688033005/diff/140001/media/base/video_frame... media/base/video_frame_metadata.h:81: RESOURCE_UPDATER_FLAG, On 2016/02/23 17:58:02, xhwang wrote: > Also, media/base knows nothing about "cc" and "ResourceUpdater", can we have a > more meaningful name for this flag? Removed this flag here. Instead, use base::SupportsUserData to set the userdata in VideoFrame. media/base does not need to take care of it.
danakj@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/1688033005/diff/160001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/1688033005/diff/160001/cc/resources/video_res... cc/resources/video_resource_updater.cc:141: plane_resource.timestamp == video_frame->timestamp() && Can you instead just return false if the timestamps are 0?
xjz@chromium.org changed reviewers: - brianderson@chromium.org
https://codereview.chromium.org/1688033005/diff/160001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/1688033005/diff/160001/cc/resources/video_res... cc/resources/video_resource_updater.cc:141: plane_resource.timestamp == video_frame->timestamp() && On 2016/02/23 23:55:09, danakj wrote: > Can you instead just return false if the timestamps are 0? The actual timestamp could be 0. It is difficult to check whether the timestamp is correctly set (unique for each frame) here.
https://codereview.chromium.org/1688033005/diff/160001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/1688033005/diff/160001/cc/resources/video_res... cc/resources/video_resource_updater.cc:141: plane_resource.timestamp == video_frame->timestamp() && On 2016/02/24 00:01:28, xjz wrote: > On 2016/02/23 23:55:09, danakj wrote: > > Can you instead just return false if the timestamps are 0? > > The actual timestamp could be 0. It is difficult to check whether the timestamp > is correctly set (unique for each frame) here. Oh :/ I don't really like cc setting things on a VideoFrame for this here, it feels very much like a layering violation. Can the video decoders set some incrementing id on VideoFrames that they generate or something, and it could use that, since there is this new need to tell when a VideoFrame is repeated? That feels like something the VideoFrame should provide then, rather than hacking in this general thing where cc sets a flag only it knows about.
https://codereview.chromium.org/1688033005/diff/160001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/1688033005/diff/160001/cc/resources/video_res... cc/resources/video_resource_updater.cc:141: plane_resource.timestamp == video_frame->timestamp() && On 2016/02/24 00:07:08, danakj wrote: > On 2016/02/24 00:01:28, xjz wrote: > > On 2016/02/23 23:55:09, danakj wrote: > > > Can you instead just return false if the timestamps are 0? > > > > The actual timestamp could be 0. It is difficult to check whether the > timestamp > > is correctly set (unique for each frame) here. > > Oh :/ I don't really like cc setting things on a VideoFrame for this here, it > feels very much like a layering violation. > > Can the video decoders set some incrementing id on VideoFrames that they > generate or something, and it could use that, since there is this new need to > tell when a VideoFrame is repeated? That feels like something the VideoFrame > should provide then, rather than hacking in this general thing where cc sets a > flag only it knows about. This sounds equivalent to asking decoders correctly set the timestamps.
On Wed, Feb 24, 2016 at 1:57 PM, <xjz@chromium.org> wrote: > > > https://codereview.chromium.org/1688033005/diff/160001/cc/resources/video_res... > File cc/resources/video_resource_updater.cc (right): > > > https://codereview.chromium.org/1688033005/diff/160001/cc/resources/video_res... > cc/resources/video_resource_updater.cc:141: plane_resource.timestamp == > video_frame->timestamp() && > On 2016/02/24 00:07:08, danakj wrote: > > On 2016/02/24 00:01:28, xjz wrote: > > > On 2016/02/23 23:55:09, danakj wrote: > > > > Can you instead just return false if the timestamps are 0? > > > > > > The actual timestamp could be 0. It is difficult to check whether > the > > timestamp > > > is correctly set (unique for each frame) here. > > > > Oh :/ I don't really like cc setting things on a VideoFrame for this > here, it > > feels very much like a layering violation. > > > > Can the video decoders set some incrementing id on VideoFrames that > they > > generate or something, and it could use that, since there is this new > need to > > tell when a VideoFrame is repeated? That feels like something the > VideoFrame > > should provide then, rather than hacking in this general thing where > cc sets a > > flag only it knows about. > > This sounds equivalent to asking decoders correctly set the timestamps. > Sure.. is that a problem? If it is, maybe having an monotonically increasing number is easier. > > https://codereview.chromium.org/1688033005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #5 (id:180001) has been deleted
Description was changed from ========== Add a new VideoFrameMetadata field called RESOURCE_UPDATER_FLAG to indicate that the VideoFrame's content has been cached as resource by cc::VideoResourceUpdater. BUG=581480 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Monitor VideoResourceUpdater reusing destructed resource in Debug mode. VideoResourceUpdater software planes use VideoFrame raw pointer and video timestamp as the unique ID for each VideoFrame. However, if the VideoFrame is destructed and the pointer is reused by a new VideoFrame, and if the timestamp is not correctly set, the resource maybe incorrectly reused. This CL marks the resource as destructed when the original VideoFrame is destructed, and monitors if the destructed resource is re-used. The CL only takes effect in Debug mode. BUG=581480 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
On 2016/02/29 22:38:25, danakj wrote: > On Wed, Feb 24, 2016 at 1:57 PM, <mailto:xjz@chromium.org> wrote: > > > > > > > > https://codereview.chromium.org/1688033005/diff/160001/cc/resources/video_res... > > File cc/resources/video_resource_updater.cc (right): > > > > > > > https://codereview.chromium.org/1688033005/diff/160001/cc/resources/video_res... > > cc/resources/video_resource_updater.cc:141: plane_resource.timestamp == > > video_frame->timestamp() && > > On 2016/02/24 00:07:08, danakj wrote: > > > On 2016/02/24 00:01:28, xjz wrote: > > > > On 2016/02/23 23:55:09, danakj wrote: > > > > > Can you instead just return false if the timestamps are 0? > > > > > > > > The actual timestamp could be 0. It is difficult to check whether > > the > > > timestamp > > > > is correctly set (unique for each frame) here. > > > > > > Oh :/ I don't really like cc setting things on a VideoFrame for this > > here, it > > > feels very much like a layering violation. > > > > > > Can the video decoders set some incrementing id on VideoFrames that > > they > > > generate or something, and it could use that, since there is this new > > need to > > > tell when a VideoFrame is repeated? That feels like something the > > VideoFrame > > > should provide then, rather than hacking in this general thing where > > cc sets a > > > flag only it knows about. > > > > This sounds equivalent to asking decoders correctly set the timestamps. > > > > Sure.. is that a problem? If it is, maybe having an monotonically > increasing number is easier. > > > > > > https://codereview.chromium.org/1688033005/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Changed the approach. The VideoResourceUpdater requires that the timestamp (or an ID) is correctly set (unique for each VideoFrame). However, it is hard to detect if it is not. So the new approach just adds a monitor in debug mode to detect if any reuse of destructed resource happens. Please see the updated description.
PTAL Please see the updated description for the change.
lgtm. Note to other reviewers: While this looks like quite a bit of extra code that only compiles/runs in Debug mode, there really doesn't seem to be a simpler way to detect clients that don't set the VideoFrame timestamp correctly here. When this bug bit us, it cost us a huge amount of developer time to diagnose and root-cause, so IMHO, the extra code is worth the protection it affords us. https://codereview.chromium.org/1688033005/diff/200001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/1688033005/diff/200001/cc/resources/video_res... cc/resources/video_resource_updater.cc:159: DCHECK(plane_resource.destructed == false) nit: DCHECK(!plane_resource.destructed) https://codereview.chromium.org/1688033005/diff/200001/cc/resources/video_res... cc/resources/video_resource_updater.cc:747: if (!updater.get()) nit: if (!updater) https://codereview.chromium.org/1688033005/diff/200001/cc/resources/video_res... File cc/resources/video_resource_updater.h (right): https://codereview.chromium.org/1688033005/diff/200001/cc/resources/video_res... cc/resources/video_resource_updater.h:104: // to detect the reuse of the destructed resource (in Debug mode only). Instead of "It is used to detect the reuse of the destructed resource" this should be: "It is used to detect clients that are not setting the VideoFrame's timestamp field correctly, as required. The memory allocator can and will re-use the same pointer for new VideoFrame instances, so a destruction observer is used to detect that."
lgtm even though you don't need my approval any more :)
PTAL https://codereview.chromium.org/1688033005/diff/200001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/1688033005/diff/200001/cc/resources/video_res... cc/resources/video_resource_updater.cc:159: DCHECK(plane_resource.destructed == false) On 2016/03/09 01:59:49, miu wrote: > nit: DCHECK(!plane_resource.destructed) Done. https://codereview.chromium.org/1688033005/diff/200001/cc/resources/video_res... cc/resources/video_resource_updater.cc:747: if (!updater.get()) On 2016/03/09 01:59:49, miu wrote: > nit: if (!updater) Done. https://codereview.chromium.org/1688033005/diff/200001/cc/resources/video_res... File cc/resources/video_resource_updater.h (right): https://codereview.chromium.org/1688033005/diff/200001/cc/resources/video_res... cc/resources/video_resource_updater.h:104: // to detect the reuse of the destructed resource (in Debug mode only). On 2016/03/09 01:59:49, miu wrote: > Instead of "It is used to detect the reuse of the destructed resource" this > should be: "It is used to detect clients that are not setting the VideoFrame's > timestamp field correctly, as required. The memory allocator can and will re-use > the same pointer for new VideoFrame instances, so a destruction observer is used > to detect that." Done.
On 2016/03/10 01:51:44, xjz wrote: > PTAL > > https://codereview.chromium.org/1688033005/diff/200001/cc/resources/video_res... > File cc/resources/video_resource_updater.cc (right): > > https://codereview.chromium.org/1688033005/diff/200001/cc/resources/video_res... > cc/resources/video_resource_updater.cc:159: DCHECK(plane_resource.destructed == > false) > On 2016/03/09 01:59:49, miu wrote: > > nit: DCHECK(!plane_resource.destructed) > > Done. > > https://codereview.chromium.org/1688033005/diff/200001/cc/resources/video_res... > cc/resources/video_resource_updater.cc:747: if (!updater.get()) > On 2016/03/09 01:59:49, miu wrote: > > nit: if (!updater) > > Done. > > https://codereview.chromium.org/1688033005/diff/200001/cc/resources/video_res... > File cc/resources/video_resource_updater.h (right): > > https://codereview.chromium.org/1688033005/diff/200001/cc/resources/video_res... > cc/resources/video_resource_updater.h:104: // to detect the reuse of the > destructed resource (in Debug mode only). > On 2016/03/09 01:59:49, miu wrote: > > Instead of "It is used to detect the reuse of the destructed resource" this > > should be: "It is used to detect clients that are not setting the VideoFrame's > > timestamp field correctly, as required. The memory allocator can and will > re-use > > the same pointer for new VideoFrame instances, so a destruction observer is > used > > to detect that." > > Done. danakj: lgty?
https://chromiumcodereview.appspot.com/1688033005/diff/220001/cc/resources/vi... File cc/resources/video_resource_updater.cc (right): https://chromiumcodereview.appspot.com/1688033005/diff/220001/cc/resources/vi... cc/resources/video_resource_updater.cc:159: DCHECK(!plane_resource.destructed) Can you maybe just add a Lock to the struct when dcheck is on, and avoid introducing task runners to this class? Compositor receives task runners from its embedders, and I don't think we should use ThreadTaskRunnerHandle in here, we'd need to plumb those thru and that's maybe unnecessary? https://chromiumcodereview.appspot.com/1688033005/diff/220001/cc/resources/vi... cc/resources/video_resource_updater.cc:530: video_frame->AddDestructionObserver(base::Bind( Since you're doing this at each call to SetPlaneResourceUniqueID, maybe just call this inside there? https://chromiumcodereview.appspot.com/1688033005/diff/220001/cc/resources/vi... cc/resources/video_resource_updater.cc:745: const void* video_frame_ptr, why void? https://chromiumcodereview.appspot.com/1688033005/diff/220001/cc/resources/vi... File cc/resources/video_resource_updater.h (right): https://chromiumcodereview.appspot.com/1688033005/diff/220001/cc/resources/vi... cc/resources/video_resource_updater.h:102: #if !defined(NDEBUG) I think you mean #if DCHECK_IS_ON() https://chromiumcodereview.appspot.com/1688033005/diff/220001/cc/resources/vi... cc/resources/video_resource_updater.h:155: #if !defined(NDEBUG) for all these
PTAL https://codereview.chromium.org/1688033005/diff/220001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/1688033005/diff/220001/cc/resources/video_res... cc/resources/video_resource_updater.cc:159: DCHECK(!plane_resource.destructed) On 2016/03/14 18:52:41, danakj wrote: > Can you maybe just add a Lock to the struct when dcheck is on, and avoid > introducing task runners to this class? > > Compositor receives task runners from its embedders, and I don't think we should > use ThreadTaskRunnerHandle in here, we'd need to plumb those thru and that's > maybe unnecessary? Using Lock needs to add Lock on |all_resource_| anywhere it is used. Does it look good? Maybe better to get the compositor's task runners when DCHECK is on? Where to get it? https://codereview.chromium.org/1688033005/diff/220001/cc/resources/video_res... cc/resources/video_resource_updater.cc:530: video_frame->AddDestructionObserver(base::Bind( On 2016/03/14 18:52:41, danakj wrote: > Since you're doing this at each call to SetPlaneResourceUniqueID, maybe just > call this inside there? In that way, we have to add a parameter to SetPlaneResourceUniqueID() as it is a static function, and the callback MarkOldResource() uses member from VideoResourceUpdater. Since this change only takes effect when DCHECK is on, it might be unnecessary to change the definition of SetPlaneResourceUniqueID()? https://codereview.chromium.org/1688033005/diff/220001/cc/resources/video_res... cc/resources/video_resource_updater.cc:745: const void* video_frame_ptr, On 2016/03/14 18:52:41, danakj wrote: > why void? Because it is not allowed to bine a ref counted ptr to the callback, and the function only uses the raw pointer of the |video_frame| to compare. https://codereview.chromium.org/1688033005/diff/220001/cc/resources/video_res... File cc/resources/video_resource_updater.h (right): https://codereview.chromium.org/1688033005/diff/220001/cc/resources/video_res... cc/resources/video_resource_updater.h:102: #if !defined(NDEBUG) On 2016/03/14 18:52:41, danakj wrote: > I think you mean #if DCHECK_IS_ON() Done. https://codereview.chromium.org/1688033005/diff/220001/cc/resources/video_res... cc/resources/video_resource_updater.h:155: #if !defined(NDEBUG) On 2016/03/14 18:52:41, danakj wrote: > for all these Done.
LGTM w/ 1 more question https://codereview.chromium.org/1688033005/diff/220001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/1688033005/diff/220001/cc/resources/video_res... cc/resources/video_resource_updater.cc:530: video_frame->AddDestructionObserver(base::Bind( On 2016/03/15 21:04:00, xjz wrote: > On 2016/03/14 18:52:41, danakj wrote: > > Since you're doing this at each call to SetPlaneResourceUniqueID, maybe just > > call this inside there? > > In that way, we have to add a parameter to SetPlaneResourceUniqueID() as it is a > static function, and the callback MarkOldResource() uses member from > VideoResourceUpdater. Since this change only takes effect when DCHECK is on, it > might be unnecessary to change the definition of SetPlaneResourceUniqueID()? OK. I feel like there's some refactoring needs building up in here around allocating, finding, reusing resources. We're getting a pretty long and complicated single function here. https://codereview.chromium.org/1688033005/diff/240001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/1688033005/diff/240001/cc/resources/video_res... cc/resources/video_resource_updater.cc:429: static_cast<void*>(video_frame.get()), can you base::Unretained() this pointer to avoid the void* dance? If not please add a comment saying what you're doing with the static cast here.
https://codereview.chromium.org/1688033005/diff/240001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/1688033005/diff/240001/cc/resources/video_res... cc/resources/video_resource_updater.cc:429: static_cast<void*>(video_frame.get()), On 2016/03/17 00:00:34, danakj wrote: > can you base::Unretained() this pointer to avoid the void* dance? > > If not please add a comment saying what you're doing with the static cast here. Done.
The CQ bit was checked by xjz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org, miu@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/1688033005/#ps260001 (title: "Rebase and address danakj's comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1688033005/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1688033005/260001
Message was sent while issue was closed.
Description was changed from ========== Monitor VideoResourceUpdater reusing destructed resource in Debug mode. VideoResourceUpdater software planes use VideoFrame raw pointer and video timestamp as the unique ID for each VideoFrame. However, if the VideoFrame is destructed and the pointer is reused by a new VideoFrame, and if the timestamp is not correctly set, the resource maybe incorrectly reused. This CL marks the resource as destructed when the original VideoFrame is destructed, and monitors if the destructed resource is re-used. The CL only takes effect in Debug mode. BUG=581480 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Monitor VideoResourceUpdater reusing destructed resource in Debug mode. VideoResourceUpdater software planes use VideoFrame raw pointer and video timestamp as the unique ID for each VideoFrame. However, if the VideoFrame is destructed and the pointer is reused by a new VideoFrame, and if the timestamp is not correctly set, the resource maybe incorrectly reused. This CL marks the resource as destructed when the original VideoFrame is destructed, and monitors if the destructed resource is re-used. The CL only takes effect in Debug mode. BUG=581480 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #8 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Monitor VideoResourceUpdater reusing destructed resource in Debug mode. VideoResourceUpdater software planes use VideoFrame raw pointer and video timestamp as the unique ID for each VideoFrame. However, if the VideoFrame is destructed and the pointer is reused by a new VideoFrame, and if the timestamp is not correctly set, the resource maybe incorrectly reused. This CL marks the resource as destructed when the original VideoFrame is destructed, and monitors if the destructed resource is re-used. The CL only takes effect in Debug mode. BUG=581480 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Monitor VideoResourceUpdater reusing destructed resource in Debug mode. VideoResourceUpdater software planes use VideoFrame raw pointer and video timestamp as the unique ID for each VideoFrame. However, if the VideoFrame is destructed and the pointer is reused by a new VideoFrame, and if the timestamp is not correctly set, the resource maybe incorrectly reused. This CL marks the resource as destructed when the original VideoFrame is destructed, and monitors if the destructed resource is re-used. The CL only takes effect in Debug mode. BUG=581480 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/66b273c41ace18613ce65175bbcf47f9bd3ddf94 Cr-Commit-Position: refs/heads/master@{#381739} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/66b273c41ace18613ce65175bbcf47f9bd3ddf94 Cr-Commit-Position: refs/heads/master@{#381739} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
