<div dir="ltr"><div dir="ltr">Hi all,<div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 24 Mar 2021 at 20:34, Jean-Michel Hautbois <<a href="mailto:jeanmichel.hautbois@ideasonboard.com">jeanmichel.hautbois@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Kieran, Laurent,<br>
<br>
On 24/03/2021 21:19, Laurent Pinchart wrote:<br>
> Hi Kieran,<br>
> <br>
> Thank you for the patch.<br>
> <br>
> On Wed, Mar 24, 2021 at 08:06:24PM +0000, Kieran Bingham wrote:<br>
>> When the CIO2 returns a cancelled buffer, we will not queue buffers<br>
>> to the IMGU.<br>
>><br>
>> These buffers should be explicitly marked as cancelled to ensure<br>
>> the application knows there is no valid metadata or frame data<br>
>> provided in the buffer.<br>
>><br>
>> Provide a cancel() method on the FrameBuffer to allow explicitly<br>
>> cancelling a buffer.<br>
>><br>
>> Signed-off-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
>> ---<br>
>> include/libcamera/buffer.h | 1 +<br>
>> src/libcamera/pipeline/ipu3/ipu3.cpp | 7 +++++--<br>
>> 2 files changed, 6 insertions(+), 2 deletions(-)<br>
>><br>
>> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h<br>
>> index 302fe3d3e86b..6557fb1e59b5 100644<br>
>> --- a/include/libcamera/buffer.h<br>
>> +++ b/include/libcamera/buffer.h<br>
>> @@ -49,6 +49,7 @@ public:<br>
>> Request *request() const { return request_; }<br>
>> void setRequest(Request *request) { request_ = request; }<br>
>> const FrameMetadata &metadata() const { return metadata_; }<br>
>> + void cancel() { metadata_.status = FrameMetadata::FrameCancelled; }<br>
> <br>
> Missing documentation ? I think we should document the function, but<br>
> also explained at a higher level in the FrameBuffer documentation how<br>
> it's meant to be used.<br>
> <br>
>> <br>
>> unsigned int cookie() const { return cookie_; }<br>
>> void setCookie(unsigned int cookie) { cookie_ = cookie; }<br>
>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
>> index fc40dcb381ea..44bd5a9d042b 100644<br>
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp<br>
>> @@ -1256,8 +1256,11 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer)<br>
>> <br>
>> /* If the buffer is cancelled force a complete of the whole request. */<br>
>> if (buffer->metadata().status == FrameMetadata::FrameCancelled) {<br>
>> - for (auto it : request->buffers())<br>
>> - pipe_->completeBuffer(request, it.second);<br>
>> + for (auto it : request->buffers()) {<br>
>> + FrameBuffer *b = it.second;<br>
>> + b->cancel();<br>
>> + pipe_->completeBuffer(request, b);<br>
>> + }<br>
>> <br>
>> frameInfos_.remove(info);<br>
>> pipe_->completeRequest(request);<br>
> <br>
> Pipeline handlers never set the buffer status explicitly today, as this<br>
> is handled in V4L2VideoDevice. As you've found out, it causes issues we<br>
> requests whose buffers haven't been queued. Adding a<br>
> FrameBuffer::cancel() function makes the API a bit asymmetrical, which<br>
> bothers me a bit. There's also the issue that this function shouldn't be<br>
> visible to applications.<br>
> <br>
> I wonder, would it be better to create a Request::cancel() function<br>
> instead, that will cancel all buffers automatically and complete the<br>
> request ?<br></blockquote><div><br></div><div>Thought I might chime in as I had to deal with exactly this scenario</div><div>for the Raspberry Pi pipeline handler. My handling of incomplete</div><div>requests can be found at [1]. It's not the nicest bit of code, but it does</div><div>the job.</div><div><br></div><div>If there was a Request::cancel() method, that would significantly tidy</div><div>up the function at [1], so it has my vote. I recall mentioning this ages</div><div>back when we first released our pipeline handler, and there was</div><div>agreement that such a function would eventually be needed. That time</div><div>looks to be now :-)</div><div><br></div><div>Thanks,</div><div>Naush</div><div><br></div><div>[1] <a href="https://git.linuxtv.org/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n1485">https://git.linuxtv.org/libcamera.git/tree/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp#n1485</a><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
I like that latest idea better.<br>
The comment in cio2BufferReady() is "If the buffer is cancelled" => but<br>
isn't it indeed the request which is cancelled :-) ?<br>
<br>
_______________________________________________<br>
libcamera-devel mailing list<br>
<a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
<a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>