Mir

Code review comment for lp:~mir-team/mir/enable-dynamic-buffer-queue

Revision history for this message
Alberto Aguirre (albaguirre) wrote :

> (1) A useful FIXME comment is missing from mc::BufferQueue::min_buffers() -
> // FIXME: Sometimes required_buffers > nbuffers (LP: #1317403)
>
> (2) An important test has been deleted:
> 1061 -TEST_F(BufferQueueTest, bypass_clients_get_more_than_two_buffers)
> It needs to stay, and to pass(!). At least bypass needed 3+ buffers at some
> peak times last I checked. Otherwise it will intermittently deadlock. Although
> I forget the full reasoning right now.
>
> (3) You have fudged slow_client_framerate_matches_compositor, which is kind of
> cheating as mentioned in
> https://code.launchpad.net/~vanvugt/mir/double/+merge/221431/comments/530360
> Not sure how safe the fudge is or if it risks regression. I would be more sure
> if I knew how to reintroduce the bug it protects against :)

(1) is not applicable anymore

(2) It's no longer an important test. Bypass does not need more than 2 buffers. If you find a deadlock scenario, let's fix the deadlock not increase the number of buffers.

(3) No cheating...you have to account for the thread/context switch time since there's no pipelining - which under valgrind running on armhf seems to be significant.

« Back to merge proposal