Code review comment for lp:~raof/mir/1hz-rendering-always

On Mon, May 19, 2014 at 9:24 PM, Alan Griffiths <email address hidden>
wrote:
> Review: Needs Information
>
> Apologies for meandering thoughts. But this is continuing the *Needs
> discussion*...
>
> Short version: while I'm in agreement with the approach this branch
> proposes I've hung up on a lack of design clarity around
> "FrameDroppingPolicy" which isn't really a policy and doesn't
> encapsulates the framedropping logic.
>
>> > 61 +class FrameDroppingPolicy
>> > ...
>> > 70 + /**
>> > 71 + * \brief Notify that a swap has blocked
>> > 72 + */
>> > 73 + virtual void swap_now_blocking() = 0;
>> > 74 + /**
>> > 75 + * \brief Notify that previous swap is no longer blocking
>> > 76 + */
>> > 77 + virtual void swap_unblocked() = 0;
>> >
>> > I don't immediately understand the role of a "policy" class that
>> > doesn't seem to be responsible for doing anything (it only has
>> these
>> > two member functions). Based on the functions it has is a
>> > SwapBlockageObserver not a FrameDroppingPolicy.
>>
>> It makes frame dropping decisions based upon notifications from the
>> buffer queue.
>>
>> The mechanism it uses for dropping frames is set at creation time,
>> which is why it doesn't appear in the interface.
>
> I don't think you've quite understood this concern. It isn't the role
> of TimeoutFrameDroppingPolicy I'm saying is unclear it is
> FrameDroppingPolicy that I'm doubtful about. The code that uses it
> looks like this:
>
> 711 + /* Can't give the client a buffer yet; they'll just have to wait
> 712 + * until the compositor is done with an old frame, or the policy
> 713 + * says they've waited long enough.
> 714 + */
> 715 + framedrop_policy->swap_now_blocking();
>
> and
>
> 724 + framedrop_policy->swap_unblocked();
> 725 give_buffer_to_client(buffer, std::move(lock));
>
> Neither fragment looks to me like a call to be handled by a "policy".
> (They look a bit more like state transitions - but if that that model
> is correct "enter_swap_blocked()/exit_swap_blocked() is probably
> clearer.)

It is a state transition, yes, with the wrinkle that it's recursive;
you can enter_swap_blocked() while currently in a swap_blocked state.
If we go this way, enter_swap_blocked()/exit_swap_blocked() would be a
reasonable rename.

The policy needs to know the state, and when the state changes, so it
does indeed look like a state observer.

>
>
> Actually, BufferQueue still has:
>
> /* Last resort, drop oldest buffer from the ready queue */
> if (frame_dropping_enabled && !ready_to_composite_queue.empty())
> {
> auto const buffer = pop(ready_to_composite_queue);
> give_buffer_to_client(buffer, std::move(lock));
>
> So we have both framedrop_policy and frame_dropping_enabled in
> BufferQueue. So we're not covering all the policy around
> framedropping in TimeoutFrameDroppingPolicy.

Hm, this is true. How about if we added virtual void
client_requests_framedropping(bool); to FrameDroppingPolicy. Then
FrameDroppingPolicy *would* be responsible for all the framedropping
decisions?

> …
> class SwapBuffersPolicy
> {
> public:
> virtual void handle_client_acquire(
> std::vector<graphics::Buffer*>& free_buffers,
> std::deque<graphics::Buffer*>& ready_to_composite_queue,
> std::vector<std::shared_ptr<graphics::Buffer>>& buffers,
> mc::BufferQueue::Callback complete) = 0;
>
> virtual void handle_buffer_available_for_client(
> mg::Buffer* buffer,
> std::vector<graphics::Buffer*>& free_buffers) = 0;
>
> virtual void handle_force_requests_to_complete(
> std::vector<graphics::Buffer*>& free_buffers,
> std::deque<graphics::Buffer*>& ready_to_composite_queue) = 0;
> };

My concern with this is that we're extensively tying something that
we'll want to have in the exported-to-shell-API (“how do you want to
handle it when clients get blocked in swap_buffers”) with a the
deeply internal implementation detail of BufferQueue.

A shell won't want to deal with all of handling client swap_buffers
requests; “When should I framedrop” is a well-defined fairly
self-contained question that should have a fairly self-contained answer.