[libcamera-devel] [PATCH 2/3] libcamera: buffer: Rename buffer.h to framebuffer.h

[Kieran Bingham]

On 05/07/2021 11:22, Laurent Pinchart wrote:

>>>   rename include/libcamera/{buffer.h => framebuffer.h} (89%)
>>>   rename include/libcamera/internal/{buffer.h => framebuffer.h} (78%)
>>
>> Are you sure you want to name it framebuffer.h for the internal header 
>> also? I get the impression that the patch attempts to eliminates the 
>> exception, filename vs class-name mismatch, but the internal header with 
>> this change, will still be an exception. Why shouldn't we be leaning 
>> towards
>>
>>   rename include/libcamera/internal/{buffer.h => mappedbuffer.h} (78%) ?
> 
> That's a good point. Note however how patch 3/3 adds the
> FrameBuffer::Private class to internal/framebuffer.h. The MappedBuffer
> class is hijacking framebuffer.h a little bit, maybe it should be split
> into a separate header. It has been added to buffer.h out of simplicity,
> and we have other headers that have a main purpose and also carry
> auxiliary classes.

I would have said all the more reason to rename this to mappedbuffer.h
directly here, (as that's all it contains) - and re-introduce
framebuffer.h in 3/3.

But ... I won't object to either route, if this is done on top so

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


> In any case, I think this should be addressed in a separate patch if we
> decide to go that route.

Splitting MappedBuffers out sounds like a sensible thing to do though,
especially given they are considered to be a helper 'on top' of our
Buffer API (yet only used internally here).

>> Other than that, patch looks good to me,
>>
>> Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>

Umnag, As you've already essentially done exactly this for the IPA -
would you be happy to submit a patch to clean this up on top of
Laurent's series, if it doesn't get updated here?

--
Kieran