Issue 2042123002: Mac: Make resize more smooth

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+92 lines, -38 lines)			Patch
	M	gpu/ipc/service/image_transport_surface_overlay_mac.h	View	1 2	2 chunks	+21 lines, -6 lines	2 comments	Download
	M	gpu/ipc/service/image_transport_surface_overlay_mac.mm	View	1 2	10 chunks	+71 lines, -32 lines	7 comments	Download

Messages

Total messages: 6 (3 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

tapted

Is there maybe a bug, or some other context (e.g. for CL description) to help ...

4 years, 6 months ago (2016-06-17 00:23:38 UTC) #4

Is there maybe a bug, or some other context (e.g. for CL description) to help me
understand the bigger picture?  E.g. is this just resizes when video is playing?
(What's required to trigger an observable "flash"?) Sorry - i'm still wrapping
my head around a lot of this code :)

And is there a downside to keeping around the additional CAContexts and
LayerTreeCoordinators?  (what's the size of the resources they hold on to, or
keep references alive to? Is it per-tab or per window?). Is there always a
constant stream of swaps, or could the stuff in discarded_size_dependent_state_
become indefinitely stale if the page is idle? (is there any need to have a
timer-based trigger to flush out stale context?)

https://codereview.chromium.org/2042123002/diff/40001/gpu/ipc/service/image_t...
File gpu/ipc/service/image_transport_surface_overlay_mac.h (right):

https://codereview.chromium.org/2042123002/diff/40001/gpu/ipc/service/image_t...
gpu/ipc/service/image_transport_surface_overlay_mac.h:99: struct
SizeDependentState {
optional since this is in private: (and deque/unique_ptr don't need complete
types) you could forward declare `struct SizeDependentState`; here and define

strut ImageTransportSurfaceOverlayMac::SizeDependentState {
..
}

in the .mm file. Then you don't have to provide constructor/destructor at all
(although my next comment requires a constructor again, but it can be inlined)

But! Then the constructor can take arguments - I think that will clean up
Resize(..) a little. E.g. construct with arguments

pixel_size, scale_factor, and use_remote_layer_api

https://codereview.chromium.org/2042123002/diff/40001/gpu/ipc/service/image_t...
gpu/ipc/service/image_transport_surface_overlay_mac.h:110: };
nit: DISALLOW_COPY_AND_ASSIGN(..)

(the unique_ptr member requires this property anyway, so it's good to be
explicit)

https://codereview.chromium.org/2042123002/diff/40001/gpu/ipc/service/image_t...
File gpu/ipc/service/image_transport_surface_overlay_mac.mm (right):

https://codereview.chromium.org/2042123002/diff/40001/gpu/ipc/service/image_t...
gpu/ipc/service/image_transport_surface_overlay_mac.mm:99: if
(!size_dependent_state_)
should this always be true? (e.g. can we DCHECK(!size_dependent_state_)

https://codereview.chromium.org/2042123002/diff/40001/gpu/ipc/service/image_t...
gpu/ipc/service/image_transport_surface_overlay_mac.mm:105:
discarded_size_dependent_state_.push_back(std::move(size_dependent_state_));
why not

discarded_size_dependent_state_.clear()
size_dependent_state_.reset()

?

https://codereview.chromium.org/2042123002/diff/40001/gpu/ipc/service/image_t...
gpu/ipc/service/image_transport_surface_overlay_mac.mm:268: break;
optional nit: Maybe just move the `if` into the `while` condition. And (for
readability) perhaps something like

  auto& discards = discarded_size_dependent_state_;
  while (!discards.empty() && discards.front()->swaps_since_discarded_ > 
         kSwapsToKeepDiscardedSizeDependentState)
    discards.pop_front();


(My old-stylee brain wanted to combine these two loops, but it requires knowing
too much about interator-invalidation on std::deque - e.g. the following
probably works, but explaining, e.g., why you need to check against .empty() as
well as .end() makes it feel a little too much like voodoo

  auto& discards = discarded_size_dependent_state_;
  for (auto i = discards.begin(); !discards.empty() && i != discards.end(); ) {
    if (i->swaps_since_discarded_++ == kSwapsToKeepDiscardedSizeDependentState)
      discarded_size_dependent_state_.erase(i++);
    else
      ++i;
  }

https://codereview.chromium.org/2042123002/diff/40001/gpu/ipc/service/image_t...
gpu/ipc/service/image_transport_surface_overlay_mac.mm:398: [CAContext
contextWithCGSConnection:connection_id options:@{}] retain]);
nit: use base::scoped_policy::RETAIN ?

https://codereview.chromium.org/2042123002/diff/40001/gpu/ipc/service/image_t...
gpu/ipc/service/image_transport_surface_overlay_mac.mm:404: [CAContext
contextWithCGSConnection:connection_id options:@{}] retain]);
nit: base::scoped_policy::RETAIN

https://codereview.chromium.org/2042123002/diff/40001/gpu/ipc/service/image_t...
gpu/ipc/service/image_transport_surface_overlay_mac.mm:408: }
I think it would be good to move all this into the SizeDependentState
constructor -- makes it clear what properties it's using from
ImageTransportSurfaceOverlayMac to create the state (I think just
use_remote_layer_api_?), and you get to remove some occurrences of
`size_dependent_state_->`

https://codereview.chromium.org/2042123002/diff/40001/gpu/ipc/service/image_t...
gpu/ipc/service/image_transport_surface_overlay_mac.mm:443:
ImageTransportSurfaceOverlayMac::SizeDependentState::SizeDependentState() =
nit: (if you want to keep the declaration in the header) move before
IOSurfaceInUseQuery, to match header order

ccameron

4 years, 6 months ago (2016-06-17 00:34:45 UTC) #6

On 2016/06/17 00:23:38, tapted wrote:
> Is there maybe a bug, or some other context (e.g. for CL description) to help
me
> understand the bigger picture?  E.g. is this just resizes when video is
playing?
> (What's required to trigger an observable "flash"?) Sorry - i'm still wrapping
> my head around a lot of this code :)

Sorry, I wrote up a whole bunch of context, but it looks like it disappeared.
Sorry -- I'll go re-write it!

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages