[libcamera-devel] [RFC PATCH v2 05/10] ipa: rkisp1: Use offset in mapping IPABuffer

[Kieran Bingham]

Hi Hiro,

On 25/08/2021 07:13, Hirokazu Honda wrote:
> Hi Laurent and Kieran, thank you for reviewing.

<snip>

>>>
>>> We could use emplace() here to avoid the need for a default constructor,
>>> but we wouldn't be able to use operator[]() below, which isn't nice. I
>>> wonder if we could maybe store a std::unique_ptr<MappedFrameBuffer> in
>>> mappedBuffers_, to avoid the default constructor ?
>>>
>>> Kieran, you've designed the MappedFrameBuffer class, what do you think ?
>>
>> Hrm, firstly the error_ flag needs to be fixed up if there's a default
>> constructor - mentioned below in the Constructor code.
>>
>> But otherwise, even though we do have the isValid() check, I don't think
>> we have many users that actually check it.
>>
>> The aim was to make it simple and easy to create the mapping, which is
>> why it happens on construction.
>>
>> If this is causing problems, we can separate construction and mapping...
>> but then I'd expect to have map()/unmap() as well.
>>
>>
>> Looking at existing users of MappedFrameBuffer, I can see that:
>>
>>  IPAVimc::mapBuffers() currently uses emplace, (with a
>> piecewise_construct, to be able to specify the mapping flags as
>> read-only), and only uses find() to get them.
>>
>>
>> So does IPU3. ...
>>
>> And unsurprisingly, so does RPi.
>>
>>
>> However none of those implementations are checking the isValid() flag.
>> So perhaps they need to be updated to do so.
>>
>> And if we mandate that the isValid() should be checked, then it's not
>> much different to separating construction and mapping...
>>
>>
>> The main goal for the current design is to allow other buffer types to
>> be constructed, so perhaps there could be a MappedDRMBuffer or
>> MappedDMABuffer or such.
>>
>> As long as that's kept in mind, I don't mind if the main interface
>> changes to suit needs as we determine them.
>>
>> Would using [], require checking the .isValid() flag at every usage?
>> (Because we might be given a new empty mapping, if the search failed?)
>>
>> That might become less convenient ... ?
>>
>>
> 
> I store std::unique_ptr<MappedFrameBuffer> to map as Laurent suggested.
> So I can remove the default constructor of MappedFrameBuffer.

Do we need to change all the other IPA's to use the same pattern ?

--
Kieran