Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(407)

Issue 292183011: Make DefaultDeleter for Video{De|En}codeAccelerator (Closed)

Created:
5 years, 9 months ago by dmichael (off chromium)
Modified:
5 years, 9 months ago
CC:
chromium-reviews, hclam+watch_chromium.org, imcheng+watch_chromium.org, fischman+watch_chromium.org, avayvod+watch_chromium.org, jam, mcasas+watch_chromium.org, pwestin+watch_google.com, feature-media-reviews_chromium.org, hguihot+watch_chromium.org, darin-cc_chromium.org, jasonroberts+watch_google.com, piman+watch_chromium.org, miu+watch_chromium.org, wjia+watch_chromium.org, hubbe+watch_chromium.org, mikhal+watch_chromium.org
Visibility:
Public.

Description

Make DefaultDeleter for Video{De|En}codeAccelerator This makes a specialization of base::DefaultDeleter so that scoped_ptr will use Destroy() instead of the destructor. Without this, it seems wrong to hold VideoDecodeAccelerator in a scoped_ptr, given that letting scoped_ptr delete it via the destructor would be an error. Also moves the destructors to protected, to prevent mistakes. BUG= Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273860

Patch Set 1 #

Patch Set 2 : Fix compile for Impl #

Total comments: 6

Patch Set 3 : Review fixes. #

Patch Set 4 : merge #

Patch Set 5 : out-of-line DefaultDeleter::operator() #

Unified diffs Side-by-side diffs Delta from patch set Stats (+94 lines, -29 lines) Patch
M content/common/gpu/media/gpu_video_decode_accelerator.cc View 1 chunk +0 lines, -3 lines 0 comments Download
M content/common/gpu/media/gpu_video_encode_accelerator.cc View 1 chunk +0 lines, -3 lines 0 comments Download
M content/common/gpu/media/video_decode_accelerator_impl.h View 1 2 chunks +21 lines, -1 line 0 comments Download
M content/common/gpu/media/video_decode_accelerator_impl.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M content/common/gpu/media/video_decode_accelerator_unittest.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M content/common/gpu/media/video_encode_accelerator_unittest.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M content/renderer/media/rtc_video_encoder.cc View 1 2 2 chunks +2 lines, -4 lines 0 comments Download
M content/renderer/pepper/ppb_video_decoder_impl.cc View 1 2 1 chunk +1 line, -2 lines 0 comments Download
M media/cast/video_sender/external_video_encoder.cc View 1 2 2 chunks +2 lines, -6 lines 0 comments Download
M media/filters/gpu_video_decoder.cc View 1 2 1 chunk +1 line, -2 lines 0 comments Download
M media/video/video_decode_accelerator.h View 1 2 3 4 2 chunks +20 lines, -2 lines 0 comments Download
M media/video/video_decode_accelerator.cc View 1 2 3 4 1 chunk +12 lines, -0 lines 0 comments Download
M media/video/video_encode_accelerator.h View 1 2 3 4 2 chunks +20 lines, -2 lines 0 comments Download
M media/video/video_encode_accelerator.cc View 1 2 3 4 1 chunk +11 lines, -0 lines 0 comments Download

Messages

Total messages: 28 (0 generated)
dmichael (off chromium)
What do you think of this? I found it confusing that these were owned by ...
5 years, 9 months ago (2014-05-23 22:27:20 UTC) #1
Ami GONE FROM CHROMIUM
I think this CL addresses a real concern; thanks for raising it. An alternative to ...
5 years, 9 months ago (2014-05-27 20:06:01 UTC) #2
dmichael (off chromium)
On 2014/05/27 20:06:01, Ami Fischman wrote: > I think this CL addresses a real concern; ...
5 years, 9 months ago (2014-05-28 22:14:33 UTC) #3
Ami GONE FROM CHROMIUM
It's ok for scoped_ptr to delete the resource. My desire to make destruction explicit stems ...
5 years, 9 months ago (2014-05-28 22:24:11 UTC) #4
dmichael (off chromium)
Thanks for the review! I think I addressed your comments, but not sure about one ...
5 years, 9 months ago (2014-05-28 22:31:25 UTC) #5
dmichael (off chromium)
Oh, I feel silly. You meant you want that operator to _not_ be inlined. I'll ...
5 years, 9 months ago (2014-05-28 22:33:31 UTC) #6
dmichael (off chromium)
Okay, I think it's ready for review now, thanks!
5 years, 9 months ago (2014-05-28 22:53:53 UTC) #7
dmichael (off chromium)
Okay, I think it's ready for review now, thanks!
5 years, 9 months ago (2014-05-28 22:53:56 UTC) #8
Ami GONE FROM CHROMIUM
LGTM Thanks for the improvement!
5 years, 9 months ago (2014-05-28 23:00:47 UTC) #9
dmichael (off chromium)
The CQ bit was checked by dmichael@chromium.org
5 years, 9 months ago (2014-05-28 23:05:06 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmichael@chromium.org/292183011/80001
5 years, 9 months ago (2014-05-28 23:08:33 UTC) #11
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg on tryserver.chromium ...
5 years, 9 months ago (2014-05-29 03:30:44 UTC) #12
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
5 years, 9 months ago (2014-05-29 04:09:43 UTC) #13
commit-bot: I haz the power
Try jobs failed on following builders: android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/189405)
5 years, 9 months ago (2014-05-29 04:09:44 UTC) #14
dmichael (off chromium)
The CQ bit was checked by dmichael@chromium.org
5 years, 9 months ago (2014-05-29 15:49:11 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmichael@chromium.org/292183011/80001
5 years, 9 months ago (2014-05-29 15:51:57 UTC) #16
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg on tryserver.chromium ...
5 years, 9 months ago (2014-05-29 16:14:04 UTC) #17
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
5 years, 9 months ago (2014-05-29 16:59:34 UTC) #18
commit-bot: I haz the power
Try jobs failed on following builders: android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/189565)
5 years, 9 months ago (2014-05-29 16:59:35 UTC) #19
dmichael (off chromium)
The CQ bit was checked by dmichael@chromium.org
5 years, 9 months ago (2014-05-29 20:40:15 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmichael@chromium.org/292183011/80001
5 years, 9 months ago (2014-05-29 20:43:40 UTC) #21
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium ...
5 years, 9 months ago (2014-05-29 21:15:14 UTC) #22
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
5 years, 9 months ago (2014-05-29 21:41:17 UTC) #23
commit-bot: I haz the power
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered_tests/builds/158083)
5 years, 9 months ago (2014-05-29 21:41:17 UTC) #24
dmichael (off chromium)
The CQ bit was checked by dmichael@chromium.org
5 years, 9 months ago (2014-05-30 15:19:16 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmichael@chromium.org/292183011/80001
5 years, 9 months ago (2014-05-30 15:20:10 UTC) #26
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium ...
5 years, 9 months ago (2014-05-30 15:39:36 UTC) #27
commit-bot: I haz the power
5 years, 9 months ago (2014-05-30 16:48:44 UTC) #28
Message was sent while issue was closed.
Change committed as 273860

Powered by Google App Engine
This is Rietveld 408576698