Mir

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

Revision history for this message
Alexandros Frantzis (afrantzis) wrote :

Some suggestions from a first pass (a weak Needs Fixing).

61 + for(int i = 0; i < nalloc; i++)

Space between for and '('.

75 + if (alloc_policy.alloc_behavior == BufferAllocBehavior::on_demand &&
76 + allocated_buffers < min_buffers())

If alloc_behavior != on_demand isn't "allocated_buffers < min_buffers()" always false (we have pre-allocated the maximum number of buffers). So, we could just write "if (allocated_buffers < min_buffers())"

85 + bool use_current_buffer = force_use_current_buffer;

How about replacing the current logic (which has become a bit complex) with something more straightforward (IMO) like:

bool current_buffer_is_used_but_not_by(void const* user_id)
{
    return !current_buffer_users.empty() && !is_a_current_buffer_user(user_id);
}

bool have_new_compositor_buffer()
{
    return !ready_to_composite_queue.empty();
}

compositor_acquire(void const* user_id)
{
    bool const use_current_buffer =
        force_use_current_buffer ||
        (current_buffer_is_used_but_not_by(user_id) && !have_new_compositor_buffer());

    if (!use_current_buffer)
    {
       ...
    }
    else if (!is_a_current_buffer_user(user_id))
    {
       current_buffer_users.push_back(user_id);
    }
}

150 + int const alloced_buffers = buffers.size();

"allocated_buffers", no need to save a few characters (also for consistency with other "allocated_buffers" in the code)

review: Needs Fixing

« Back to merge proposal