New issue
Advanced search Search tips

Issue 837243 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature



Sign in to add a comment

Use ScopedClosureRunner in gpu_memory_buffer_video_frame_pool

Project Member Reported by mcasas@chromium.org, Apr 26 2018

Issue description

gpu_memory_buffer_video_frame_pool has a series of methods
CopyRowsToXXXBuffer(), e.g. [1]. Instead of using there the 
pattern:

 if (x)
   // do ...
 run-once-callback

consider doing here ScopedClosureRunner [2]

  base::ScopedClosureRunner(done);
  if (!output)
    return;
  // the rest.

[1] https://cs.chromium.org/chromium/src/media/video/gpu_memory_buffer_video_frame_pool.cc?dr=CSs&l=354
[2] https://cs.chromium.org/chromium/src/base/callback_helpers.h?type=cs&sq=package:chromium&l=78

 
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6c42d28ee68aafd081161a68513a5a7d6d31ca82

commit 6c42d28ee68aafd081161a68513a5a7d6d31ca82
Author: Andres Calderon Jaramillo <andrescj@chromium.org>
Date: Thu Apr 26 19:30:13 2018

Cleanup: use ScopedClosureRunner in gpu_memory_buffer_video_frame_pool.cc.

This CL modifies the CopyRowsTo*Buffer functions to run the callback
using base::ScopedClosureRunner instead of manually running it at the
end.

Bug:  837243 
Change-Id: Iea1a90f7f18a00bfc15722e84e09fe01d91126bb
Reviewed-on: https://chromium-review.googlesource.com/1030832
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Commit-Queue: Andres Calderon Jaramillo <andrescj@chromium.org>
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554117}
[modify] https://crrev.com/6c42d28ee68aafd081161a68513a5a7d6d31ca82/media/video/gpu_memory_buffer_video_frame_pool.cc

Status: Fixed (was: Assigned)
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 27 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/496b0db5d3e827f2bb5abf5cfa7509ce3db1b0d3

commit 496b0db5d3e827f2bb5abf5cfa7509ce3db1b0d3
Author: Christian Fremerey <chfremer@chromium.org>
Date: Fri Apr 27 00:13:49 2018

Revert "Cleanup: use ScopedClosureRunner in gpu_memory_buffer_video_frame_pool.cc."

This reverts commit 6c42d28ee68aafd081161a68513a5a7d6d31ca82.

Reason for revert: Speculative revert for https://bugs.chromium.org/p/chromium/issues/detail?id=837444

Original change's description:
> Cleanup: use ScopedClosureRunner in gpu_memory_buffer_video_frame_pool.cc.
> 
> This CL modifies the CopyRowsTo*Buffer functions to run the callback
> using base::ScopedClosureRunner instead of manually running it at the
> end.
> 
> Bug:  837243 
> Change-Id: Iea1a90f7f18a00bfc15722e84e09fe01d91126bb
> Reviewed-on: https://chromium-review.googlesource.com/1030832
> Commit-Queue: Miguel Casas <mcasas@chromium.org>
> Commit-Queue: Andres Calderon Jaramillo <andrescj@chromium.org>
> Reviewed-by: Miguel Casas <mcasas@chromium.org>
> Reviewed-by: Dan Sanders <sandersd@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#554117}

TBR=mcasas@chromium.org,sandersd@chromium.org,andrescj@chromium.org

Change-Id: I140ecdd60890d67507eb1cc249eb91742608fecc
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  837243 
Reviewed-on: https://chromium-review.googlesource.com/1031273
Reviewed-by: Christian Fremerey <chfremer@chromium.org>
Commit-Queue: Christian Fremerey <chfremer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554207}
[modify] https://crrev.com/496b0db5d3e827f2bb5abf5cfa7509ce3db1b0d3/media/video/gpu_memory_buffer_video_frame_pool.cc

Project Member

Comment 4 by bugdroid1@chromium.org, May 1 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8fdba54d29d86114d6f5105893efabc9a4b22d44

commit 8fdba54d29d86114d6f5105893efabc9a4b22d44
Author: Andres Calderon Jaramillo <andrescj@chromium.org>
Date: Tue May 01 02:08:25 2018

RELAND: use ScopedClosureRunner correctly in gpu_memory_buffer_video_frame_pool.

This CL relands the attached CL which got reverted due to a failing test
(WebRtcStressSourceSwitchBrowserTest.
MANUAL_SurvivesPeerConnectionSrcFeedSwitching on webrtc bots). The fundamental
cause for the failure was that the callback passed to ScopedClosureRunner was
getting called earlier than expected because the ScopedClosureRunner was
temporary.

The solution is to give a name to the ScopedClosureRunner object so that it
lives until the end of the function.

Changes can be seen here: crrev.com/c/1036162/1..2

Original CL description --------------------------------------------------------

This CL modifies the CopyRowsTo*Buffer functions to run the callback
using base::ScopedClosureRunner instead of manually running it at the
end.

Bug:  837243 
Change-Id: I9ce269e32834f74bb594ddbc031ca9518e48ccad
Reviewed-on: https://chromium-review.googlesource.com/1036162
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554972}
[modify] https://crrev.com/8fdba54d29d86114d6f5105893efabc9a4b22d44/media/video/gpu_memory_buffer_video_frame_pool.cc

Sign in to add a comment