Mir

Code review comment for lp:~mir-team/mir/touchspot-renderable

Revision history for this message
Kevin DuBois (kdub) wrote :

#1
86 + void begin(std::unordered_set<graphics::Renderable::ID> &&renderables_not_to_decorate) const;

I think its a bit more flexible not to force the use of the rvalue.
void begin(std::unordered_set<graphics::Renderable::ID> renderables_not_to_decorate) const;

begin(std::move(a)) would still use the move constructor

#2
193 + virtual std::shared_ptr<BufferWriter> create_buffer_writer() = 0;
I had to change to make_* over in my MP :)
<side note> I think that the "display stuff" should get split out of Platform into its own interface to get rid of NativePlatform.

#3
167 + virtual void write(std::shared_ptr<Buffer> const& buffer, unsigned char const* data, size_t size) = 0;

A continuation from the superseded discussion, but a char* and a size seems like it puts more burden on the caller of the interface to make sure that raw data in the char* matches what the buffer expects. (An example of this is line 1059 where we throw if the size is incorrect) The ideal is probably to see something like:
write(Buffer&, std::vector<Pixels>)
where the write function figures out how to get the pixels into that buffer (adjust for stride, format differences, etc etc)

#4
711 + auto mga_buffer = std::static_pointer_cast<mga::Buffer>(buffer);
needs removal.
Somewhat related, but write() could use a Buffer& instead of a shared_ptr<Buffer>

#5
716 + int usage = GRALLOC_USAGE_SW_READ_OFTEN | GRALLOC_USAGE_SW_WRITE_OFTEN;
minor point, but we just need SW_WRITE_OFTEN

#6
696 +mga::AndroidBufferWriter::AndroidBufferWriter()
We recreate the gralloc module in the constructor here, could reuse a single instance.

#7
698 + int err;
699 +
could use auto and get rid of these lines

#8
963 +
2701
whitespace introduction

#9
The 64x64x4 still increases mir's memory and installation footprint a fair amount, maybe we could programatically create a smaller image?

« Back to merge proposal