[{"id":20119,"web_url":"https://patchwork.libcamera.org/comment/20119/","msgid":"","date":"2021-10-11T15:20:31","subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Improve\n\tdebugging when buffer is too small","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"On 10/8/21 1:15 PM, Laurent Pinchart wrote:\n\n> When a dequeued buffer is too small, the condition is logged and an\n> error is returned. The logged message doesn't provide any information\n> about the sizes, making debugging more difficult. Improve it by logging\n> both the bytesused value and the length of each plane.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> Compile-tested only. Eugen, could you give it a try to debug the problem\n> you're facing ?\n\nTested-by: Eugen Hristev \n\nTested this patch and the information is more verbose, making it easy to \ndebug my problem.\n\n> \n> ---\n> src/libcamera/v4l2_videodevice.cpp | 14 ++++++++++----\n> 1 file changed, 10 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index ba5f88cd41ed..bd74103de7af 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1713,19 +1713,25 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> \n> unsigned int bytesused = multiPlanar ? planes[0].bytesused\n> : buf.bytesused;\n> + unsigned int remaining = bytesused;\n> \n> for (auto [i, plane] : utils::enumerate(buffer->planes())) {\n> - if (!bytesused) {\n> + if (!remaining) {\n> LOG(V4L2, Error)\n> - << \"Dequeued buffer is too small\";\n> + << \"Dequeued buffer (\" << bytesused\n> + << \" bytes) too small for plane lengths \"\n> + << utils::join(buffer->planes(), \"/\",\n> + [](const FrameBuffer::Plane &p){\n> + return p.length;\n> + });\n> \n> metadata.status = FrameMetadata::FrameError;\n> return buffer;\n> }\n> \n> metadata.planes()[i].bytesused =\n> - std::min(plane.length, bytesused);\n> - bytesused -= metadata.planes()[i].bytesused;\n> + std::min(plane.length, remaining);\n> + remaining -= metadata.planes()[i].bytesused;\n> }\n> } else if (multiPlanar) {\n> /*\n> \n> base-commit: 962df634bd0afe12e6f38464f5e602cf1460c430\n> --\n> Regards,\n> \n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D074ABDC71\n\tfor ;\n\tMon, 11 Oct 2021 15:20:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2A66568F4C;\n\tMon, 11 Oct 2021 17:20:43 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1633965643;\n\tbh=1AyfJNlutfGfn+h3hDydWzpxRPjWWIhpbqjeNg4SdXE=;\n\th=To:Date:References:In-Reply-To:List-Id:List-Post:From:\n\tList-Subscribe:List-Unsubscribe:List-Archive:Reply-To:List-Help:\n\tSubject:From;\n\tb=Rfb/Oct5CDcrKH+wVH95U1l+ivrNB3D7kbkbIiokrXneUK8Jn+ku4nGrXD0aMFnCO\n\tiyMTHUhs50RbLdWjkZD+IaL304FGcFF0pOziji5OV4n6Gy7bPD9Xf0loY+HTHBdzrA\n\tbL3CrDPfIZuLcFZDYcKKk1m6GtRtYDt11S+OueHpmGfhJzX/D2CevNmLEaogHvMv8e\n\t4ncarwM4/nD/ob0pGL/f+rAkIXTqpQoJHnQFQgLjLEZUSu8HrReAwJdDfxaulmvCCz\n\t0VWQSXbCgli4xbR/tCUqhPJb/+ql2zUUsyqlofo8bWz67ImeFVIfzvwaDt8niifXCw\n\t0EWYvWEd+PsPA==","To":",\n\t","Date":"Mon, 11 Oct 2021 15:20:31 +0000","References":"<20211008101556.22987-1-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20211008101556.22987-1-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Message-ID":"","List-Id":"","List-Post":"","From":"Eugen Hristev via libcamera-devel ","Precedence":"list","X-Mailman-Version":"2.1.29","X-BeenThere":"libcamera-devel@lists.libcamera.org","List-Subscribe":",\n\t","List-Unsubscribe":",\n\t","List-Archive":"","Reply-To":"Eugen.Hristev@microchip.com","List-Help":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Improve\n\tdebugging when buffer is too small","Content-Type":"message/rfc822","Content-Disposition":"inline","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20144,"web_url":"https://patchwork.libcamera.org/comment/20144/","msgid":"<163404367062.2976390.11310721362997315405@Monstersaurus>","date":"2021-10-12T13:01:10","subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Improve\n\tdebugging when buffer is too small","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2021-10-08 11:15:56)\n> When a dequeued buffer is too small, the condition is logged and an\n> error is returned. The logged message doesn't provide any information\n> about the sizes, making debugging more difficult. Improve it by logging\n> both the bytesused value and the length of each plane.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> Compile-tested only. Eugen, could you give it a try to debug the problem\n> you're facing ?\n> \n> ---\n> src/libcamera/v4l2_videodevice.cpp | 14 ++++++++++----\n> 1 file changed, 10 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index ba5f88cd41ed..bd74103de7af 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1713,19 +1713,25 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> \n> unsigned int bytesused = multiPlanar ? planes[0].bytesused\n> : buf.bytesused;\n> + unsigned int remaining = bytesused;\n> \n> for (auto [i, plane] : utils::enumerate(buffer->planes())) {\n> - if (!bytesused) {\n> + if (!remaining) {\n> LOG(V4L2, Error)\n> - << \"Dequeued buffer is too small\";\n> + << \"Dequeued buffer (\" << bytesused\n> + << \" bytes) too small for plane lengths \"\n> + << utils::join(buffer->planes(), \"/\",\n> + [](const FrameBuffer::Plane &p){\n> + return p.length;\n> + });\n> \n> metadata.status = FrameMetadata::FrameError;\n> return buffer;\n> }\n> \n> metadata.planes()[i].bytesused =\n> - std::min(plane.length, bytesused);\n> - bytesused -= metadata.planes()[i].bytesused;\n> + std::min(plane.length, remaining);\n> + remaining -= metadata.planes()[i].bytesused;\n\nShould this prevent underflowing 'remaining' - or otherwise, make\nremaining a signed int, and check against if (remaining <= 0) above\ninstead.\n\nI'd prefer in this context to use the signed value and make sure we\nclearly catch negative remaining..\n\nWith that,\n\n\nReviewed-by: Kieran Bingham \n\n> }\n> } else if (multiPlanar) {\n> /*\n> \n> base-commit: 962df634bd0afe12e6f38464f5e602cf1460c430\n> -- \n> Regards,\n> \n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DF955C323E\n\tfor ;\n\tTue, 12 Oct 2021 13:01:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 68BF868F4F;\n\tTue, 12 Oct 2021 15:01:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4D0C268F4C\n\tfor ;\n\tTue, 12 Oct 2021 15:01:13 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CF34AE7;\n\tTue, 12 Oct 2021 15:01:12 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"wZNNv1FQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634043672;\n\tbh=7v5YAb3Yk6XBszGrNi43U/cq/IXAzShZIwYBNUjjnIU=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=wZNNv1FQ9r/ERnFAZjumt3ibXb//UZG+nTmsgyDWIK2ibN1GdReX6bitz0Cd+VVqf\n\tF+qL0P0oTZPhcPAr16GFLiurQND8wbtqCMMFz2Mkff6l++gaDsr9ZinLNLrD3p+L+3\n\tkUBSGlUix5oRFZmYBlrtLbGMAKfmzK5cFlMJ/rH8=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211008101556.22987-1-laurent.pinchart@ideasonboard.com>","References":"<20211008101556.22987-1-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham ","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 12 Oct 2021 14:01:10 +0100","Message-ID":"<163404367062.2976390.11310721362997315405@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Improve\n\tdebugging when buffer is too small","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20148,"web_url":"https://patchwork.libcamera.org/comment/20148/","msgid":"","date":"2021-10-12T19:50:43","subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Improve\n\tdebugging when buffer is too small","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Tue, Oct 12, 2021 at 02:01:10PM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2021-10-08 11:15:56)\n> > When a dequeued buffer is too small, the condition is logged and an\n> > error is returned. The logged message doesn't provide any information\n> > about the sizes, making debugging more difficult. Improve it by logging\n> > both the bytesused value and the length of each plane.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > Compile-tested only. Eugen, could you give it a try to debug the problem\n> > you're facing ?\n> > \n> > ---\n> > src/libcamera/v4l2_videodevice.cpp | 14 ++++++++++----\n> > 1 file changed, 10 insertions(+), 4 deletions(-)\n> > \n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index ba5f88cd41ed..bd74103de7af 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -1713,19 +1713,25 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > \n> > unsigned int bytesused = multiPlanar ? planes[0].bytesused\n> > : buf.bytesused;\n> > + unsigned int remaining = bytesused;\n> > \n> > for (auto [i, plane] : utils::enumerate(buffer->planes())) {\n> > - if (!bytesused) {\n> > + if (!remaining) {\n> > LOG(V4L2, Error)\n> > - << \"Dequeued buffer is too small\";\n> > + << \"Dequeued buffer (\" << bytesused\n> > + << \" bytes) too small for plane lengths \"\n> > + << utils::join(buffer->planes(), \"/\",\n> > + [](const FrameBuffer::Plane &p){\n> > + return p.length;\n> > + });\n> > \n> > metadata.status = FrameMetadata::FrameError;\n> > return buffer;\n> > }\n> > \n> > metadata.planes()[i].bytesused =\n> > - std::min(plane.length, bytesused);\n> > - bytesused -= metadata.planes()[i].bytesused;\n> > + std::min(plane.length, remaining);\n> > + remaining -= metadata.planes()[i].bytesused;\n> \n> Should this prevent underflowing 'remaining' - or otherwise, make\n> remaining a signed int, and check against if (remaining <= 0) above\n> instead.\n> \n> I'd prefer in this context to use the signed value and make sure we\n> clearly catch negative remaining..\n\nIt's not just about overflows, it's also there to support\nvariable-length payloads. There's a comment above the code that explains\nit:\n\n\t\t/*\n\t\t * If we have a multi-planar buffer with a V4L2\n\t\t * single-planar format, split the V4L2 buffer across\n\t\t * the buffer planes. Only the last plane may have less\n\t\t * bytes used than its length.\n\t\t */\n\nIt's probably not a very common use case as we mostly deal with\nfixed-size payloads, but it doesn't cost much to keep it.\n\nLet's also note that we will still need to calculate\nmetadata.planes()[i].bytesused as std::min(plane.length, remaining) in\neither case, so I don't really see what we would gain by decrementing\nremaining with \n\n\tremaining -= plane.length;\n\ninstead.\n\n> With that,\n> \n> Reviewed-by: Kieran Bingham \n> \n> > }\n> > } else if (multiPlanar) {\n> > /*\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 876C2BDC71\n\tfor ;\n\tTue, 12 Oct 2021 19:51:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E43DE68F4F;\n\tTue, 12 Oct 2021 21:50:59 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7618768F4C\n\tfor ;\n\tTue, 12 Oct 2021 21:50:58 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F073BF1;\n\tTue, 12 Oct 2021 21:50:57 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"oE+dITd1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634068258;\n\tbh=O8NiaZigR1WoRgKsAUdkDYBxF1EeZiUAFa4MlzvaHpw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oE+dITd1DpiMG6MO+ZkUcxAe6TRSvjmkLiddw7nTgZU2xoeC2jdtmzAxjagZIUAV6\n\toSct5Zr9sxcGEXHU+qGdrKEoG9Bn9edRys15zRv4VWf/eO5HrYmvW1fUsKnZA5kv4K\n\t9B1e7Lxzqxn6XmJOb1YVo75wrqBo7HH20MiqGtpU=","Date":"Tue, 12 Oct 2021 22:50:43 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20211008101556.22987-1-laurent.pinchart@ideasonboard.com>\n\t<163404367062.2976390.11310721362997315405@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<163404367062.2976390.11310721362997315405@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Improve\n\tdebugging when buffer is too small","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20187,"web_url":"https://patchwork.libcamera.org/comment/20187/","msgid":"<163420167979.3654017.6851648647582244673@Monstersaurus>","date":"2021-10-14T08:54:39","subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Improve\n\tdebugging when buffer is too small","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2021-10-12 20:50:43)\n> Hi Kieran,\n> \n> On Tue, Oct 12, 2021 at 02:01:10PM +0100, Kieran Bingham wrote:\n> > Quoting Laurent Pinchart (2021-10-08 11:15:56)\n> > > When a dequeued buffer is too small, the condition is logged and an\n> > > error is returned. The logged message doesn't provide any information\n> > > about the sizes, making debugging more difficult. Improve it by logging\n> > > both the bytesused value and the length of each plane.\n> > > \n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > Compile-tested only. Eugen, could you give it a try to debug the problem\n> > > you're facing ?\n> > > \n> > > ---\n> > > src/libcamera/v4l2_videodevice.cpp | 14 ++++++++++----\n> > > 1 file changed, 10 insertions(+), 4 deletions(-)\n> > > \n> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > index ba5f88cd41ed..bd74103de7af 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -1713,19 +1713,25 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > > \n\neven with the banner comment at the top of this conditional, I think we\nshould add something here as it's too easy to think this is a bug...\n(I'll send a separate patch):\n\n\t\t /* Single planar format stored in a multiplanar buffer */\n> > > unsigned int bytesused = multiPlanar ? planes[0].bytesused\n> > > : buf.bytesused;\n> > > + unsigned int remaining = bytesused;\n> > > \n> > > for (auto [i, plane] : utils::enumerate(buffer->planes())) {\n> > > - if (!bytesused) {\n> > > + if (!remaining) {\n> > > LOG(V4L2, Error)\n> > > - << \"Dequeued buffer is too small\";\n> > > + << \"Dequeued buffer (\" << bytesused\n> > > + << \" bytes) too small for plane lengths \"\n> > > + << utils::join(buffer->planes(), \"/\",\n> > > + [](const FrameBuffer::Plane &p){\n> > > + return p.length;\n> > > + });\n> > > \n> > > metadata.status = FrameMetadata::FrameError;\n> > > return buffer;\n> > > }\n> > > \n> > > metadata.planes()[i].bytesused =\n> > > - std::min(plane.length, bytesused);\n> > > - bytesused -= metadata.planes()[i].bytesused;\n> > > + std::min(plane.length, remaining);\n> > > + remaining -= metadata.planes()[i].bytesused;\n> > \n> > Should this prevent underflowing 'remaining' - or otherwise, make\n> > remaining a signed int, and check against if (remaining <= 0) above\n> > instead.\n> > \n> > I'd prefer in this context to use the signed value and make sure we\n> > clearly catch negative remaining..\n> \n> It's not just about overflows, it's also there to support\n> variable-length payloads. There's a comment above the code that explains\n> it:\n> \n> /*\n> * If we have a multi-planar buffer with a V4L2\n> * single-planar format, split the V4L2 buffer across\n> * the buffer planes. Only the last plane may have less\n> * bytes used than its length.\n> */\n> \n> It's probably not a very common use case as we mostly deal with\n> fixed-size payloads, but it doesn't cost much to keep it.\n> \n> Let's also note that we will still need to calculate\n> metadata.planes()[i].bytesused as std::min(plane.length, remaining) in\n> either case, so I don't really see what we would gain by decrementing\n> remaining with \n> \n> remaining -= plane.length;\n> \n> instead.\n\nMy concern is in subtracting values from an external source (kernels can\nhave bugs), from an unsigned int, and guaranteeing that it will never\nunderflow. Which would mean \"if (!remaining)\" would not be able to catch\nthe issue.\n\n\nIf you can guarantee that there is no possibility for a kernel bug here,\nthen fine, but otherwise, I think a signed int is safer code.\n\n\n> > With that,\n> > \n> > Reviewed-by: Kieran Bingham \n> > \n> > > }\n> > > } else if (multiPlanar) {\n> > > /*\n> > > \n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BA543BDC71\n\tfor ;\n\tThu, 14 Oct 2021 08:54:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8689568F50;\n\tThu, 14 Oct 2021 10:54:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2747668F4C\n\tfor ;\n\tThu, 14 Oct 2021 10:54:42 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CDA962F3;\n\tThu, 14 Oct 2021 10:54:41 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"j9Us6qnm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634201681;\n\tbh=7ZNXjH+Y4Vi/EwZ12I4DGPJM/rbML4NWGD++9Djx5Gw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=j9Us6qnmCuYa59LNFiSxKLg8iPw5QUDTWwQtb4njKq1JmSVVJt6uKGJYS/be1pXMy\n\tqF/fv0Rrx0kpXhm3dS08H8dACIis3olvvqbMDaQV+71/Q0WPAtNWdELy0wwEMAs6TA\n\tAKkD+9RZxmyJl6AS27ubHfuTcy4pQ/LEypp1F9qE=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211008101556.22987-1-laurent.pinchart@ideasonboard.com>\n\t<163404367062.2976390.11310721362997315405@Monstersaurus>\n\t","From":"Kieran Bingham ","To":"Laurent Pinchart ","Date":"Thu, 14 Oct 2021 09:54:39 +0100","Message-ID":"<163420167979.3654017.6851648647582244673@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Improve\n\tdebugging when buffer is too small","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20190,"web_url":"https://patchwork.libcamera.org/comment/20190/","msgid":"","date":"2021-10-14T09:48:28","subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Improve\n\tdebugging when buffer is too small","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Oct 14, 2021 at 09:54:39AM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2021-10-12 20:50:43)\n> > On Tue, Oct 12, 2021 at 02:01:10PM +0100, Kieran Bingham wrote:\n> > > Quoting Laurent Pinchart (2021-10-08 11:15:56)\n> > > > When a dequeued buffer is too small, the condition is logged and an\n> > > > error is returned. The logged message doesn't provide any information\n> > > > about the sizes, making debugging more difficult. Improve it by logging\n> > > > both the bytesused value and the length of each plane.\n> > > > \n> > > > Signed-off-by: Laurent Pinchart \n> > > > ---\n> > > > Compile-tested only. Eugen, could you give it a try to debug the problem\n> > > > you're facing ?\n> > > > \n> > > > ---\n> > > > src/libcamera/v4l2_videodevice.cpp | 14 ++++++++++----\n> > > > 1 file changed, 10 insertions(+), 4 deletions(-)\n> > > > \n> > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > > index ba5f88cd41ed..bd74103de7af 100644\n> > > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > > @@ -1713,19 +1713,25 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > > > \n> \n> even with the banner comment at the top of this conditional, I think we\n> should add something here as it's too easy to think this is a bug...\n> (I'll send a separate patch):\n> \n> \t\t /* Single planar format stored in a multiplanar buffer */\n> > > > unsigned int bytesused = multiPlanar ? planes[0].bytesused\n> > > > : buf.bytesused;\n> > > > + unsigned int remaining = bytesused;\n> > > > \n> > > > for (auto [i, plane] : utils::enumerate(buffer->planes())) {\n> > > > - if (!bytesused) {\n> > > > + if (!remaining) {\n> > > > LOG(V4L2, Error)\n> > > > - << \"Dequeued buffer is too small\";\n> > > > + << \"Dequeued buffer (\" << bytesused\n> > > > + << \" bytes) too small for plane lengths \"\n> > > > + << utils::join(buffer->planes(), \"/\",\n> > > > + [](const FrameBuffer::Plane &p){\n> > > > + return p.length;\n> > > > + });\n> > > > \n> > > > metadata.status = FrameMetadata::FrameError;\n> > > > return buffer;\n> > > > }\n> > > > \n> > > > metadata.planes()[i].bytesused =\n> > > > - std::min(plane.length, bytesused);\n> > > > - bytesused -= metadata.planes()[i].bytesused;\n> > > > + std::min(plane.length, remaining);\n> > > > + remaining -= metadata.planes()[i].bytesused;\n> > > \n> > > Should this prevent underflowing 'remaining' - or otherwise, make\n> > > remaining a signed int, and check against if (remaining <= 0) above\n> > > instead.\n> > > \n> > > I'd prefer in this context to use the signed value and make sure we\n> > > clearly catch negative remaining..\n> > \n> > It's not just about overflows, it's also there to support\n> > variable-length payloads. There's a comment above the code that explains\n> > it:\n> > \n> > /*\n> > * If we have a multi-planar buffer with a V4L2\n> > * single-planar format, split the V4L2 buffer across\n> > * the buffer planes. Only the last plane may have less\n> > * bytes used than its length.\n> > */\n> > \n> > It's probably not a very common use case as we mostly deal with\n> > fixed-size payloads, but it doesn't cost much to keep it.\n> > \n> > Let's also note that we will still need to calculate\n> > metadata.planes()[i].bytesused as std::min(plane.length, remaining) in\n> > either case, so I don't really see what we would gain by decrementing\n> > remaining with \n> > \n> > remaining -= plane.length;\n> > \n> > instead.\n> \n> My concern is in subtracting values from an external source (kernels can\n> have bugs), from an unsigned int, and guaranteeing that it will never\n> underflow. Which would mean \"if (!remaining)\" would not be able to catch\n> the issue.\n\n metadata.planes()[i].bytesused =\n std::min(plane.length, remaining);\n\nIf the kernel doesn't give us enough data, remaining will be smaller\nthan plane.length. metadata.planes()[i].bytesused will thus be equal to\nremaining.\n\n remaining -= metadata.planes()[i].bytesused;\n\nAnd here remaining will become zero. I don't see where the issue is.\n\n> If you can guarantee that there is no possibility for a kernel bug here,\n> then fine, but otherwise, I think a signed int is safer code.\n> \n> \n> > > With that,\n> > > \n> > > Reviewed-by: Kieran Bingham \n> > > \n> > > > }\n> > > > } else if (multiPlanar) {\n> > > > /*\n> > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AE151C323E\n\tfor ;\n\tThu, 14 Oct 2021 09:48:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 23DFE68F4C;\n\tThu, 14 Oct 2021 11:48:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8898768F4A\n\tfor ;\n\tThu, 14 Oct 2021 11:48:45 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EB2DF2F3;\n\tThu, 14 Oct 2021 11:48:44 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"XWEYT2XJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634204925;\n\tbh=jVwv75np76ggBorMJt5SDtSXrf7YpWokFZp3k0j37uA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XWEYT2XJzoLNbo3RU7ExZt3ICu7SxRL711ctbX2YnJrSdqJmtq8XCIaz8zwfkGrBL\n\tkVc/xznzd6cVNbCOX0uRCCpyDDz8+x/IyRxLBWBWfi1NV98LlpLGaag9U3sI8DfVRF\n\t3OBk10vlnUgOHfQ7yfe5kheCVsklLtZN5VznoMjM=","Date":"Thu, 14 Oct 2021 12:48:28 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20211008101556.22987-1-laurent.pinchart@ideasonboard.com>\n\t<163404367062.2976390.11310721362997315405@Monstersaurus>\n\t\n\t<163420167979.3654017.6851648647582244673@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<163420167979.3654017.6851648647582244673@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Improve\n\tdebugging when buffer is too small","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20263,"web_url":"https://patchwork.libcamera.org/comment/20263/","msgid":"<163455670384.1018219.5019350698445942926@Monstersaurus>","date":"2021-10-18T11:31:43","subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Improve\n\tdebugging when buffer is too small","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2021-10-14 10:48:28)\n> Hi Kieran,\n> \n> On Thu, Oct 14, 2021 at 09:54:39AM +0100, Kieran Bingham wrote:\n> > Quoting Laurent Pinchart (2021-10-12 20:50:43)\n> > > On Tue, Oct 12, 2021 at 02:01:10PM +0100, Kieran Bingham wrote:\n> > > > Quoting Laurent Pinchart (2021-10-08 11:15:56)\n> > > > > When a dequeued buffer is too small, the condition is logged and an\n> > > > > error is returned. The logged message doesn't provide any information\n> > > > > about the sizes, making debugging more difficult. Improve it by logging\n> > > > > both the bytesused value and the length of each plane.\n> > > > > \n> > > > > Signed-off-by: Laurent Pinchart \n> > > > > ---\n> > > > > Compile-tested only. Eugen, could you give it a try to debug the problem\n> > > > > you're facing ?\n> > > > > \n> > > > > ---\n> > > > > src/libcamera/v4l2_videodevice.cpp | 14 ++++++++++----\n> > > > > 1 file changed, 10 insertions(+), 4 deletions(-)\n> > > > > \n> > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > > > index ba5f88cd41ed..bd74103de7af 100644\n> > > > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > > > @@ -1713,19 +1713,25 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > > > > \n> > \n> > even with the banner comment at the top of this conditional, I think we\n> > should add something here as it's too easy to think this is a bug...\n> > (I'll send a separate patch):\n> > \n> > /* Single planar format stored in a multiplanar buffer */\n> > > > > unsigned int bytesused = multiPlanar ? planes[0].bytesused\n> > > > > : buf.bytesused;\n> > > > > + unsigned int remaining = bytesused;\n> > > > > \n> > > > > for (auto [i, plane] : utils::enumerate(buffer->planes())) {\n> > > > > - if (!bytesused) {\n> > > > > + if (!remaining) {\n> > > > > LOG(V4L2, Error)\n> > > > > - << \"Dequeued buffer is too small\";\n> > > > > + << \"Dequeued buffer (\" << bytesused\n> > > > > + << \" bytes) too small for plane lengths \"\n> > > > > + << utils::join(buffer->planes(), \"/\",\n> > > > > + [](const FrameBuffer::Plane &p){\n> > > > > + return p.length;\n> > > > > + });\n> > > > > \n> > > > > metadata.status = FrameMetadata::FrameError;\n> > > > > return buffer;\n> > > > > }\n> > > > > \n> > > > > metadata.planes()[i].bytesused =\n> > > > > - std::min(plane.length, bytesused);\n> > > > > - bytesused -= metadata.planes()[i].bytesused;\n> > > > > + std::min(plane.length, remaining);\n> > > > > + remaining -= metadata.planes()[i].bytesused;\n> > > > \n> > > > Should this prevent underflowing 'remaining' - or otherwise, make\n> > > > remaining a signed int, and check against if (remaining <= 0) above\n> > > > instead.\n> > > > \n> > > > I'd prefer in this context to use the signed value and make sure we\n> > > > clearly catch negative remaining..\n> > > \n> > > It's not just about overflows, it's also there to support\n> > > variable-length payloads. There's a comment above the code that explains\n> > > it:\n> > > \n> > > /*\n> > > * If we have a multi-planar buffer with a V4L2\n> > > * single-planar format, split the V4L2 buffer across\n> > > * the buffer planes. Only the last plane may have less\n> > > * bytes used than its length.\n> > > */\n> > > \n> > > It's probably not a very common use case as we mostly deal with\n> > > fixed-size payloads, but it doesn't cost much to keep it.\n> > > \n> > > Let's also note that we will still need to calculate\n> > > metadata.planes()[i].bytesused as std::min(plane.length, remaining) in\n> > > either case, so I don't really see what we would gain by decrementing\n> > > remaining with \n> > > \n> > > remaining -= plane.length;\n> > > \n> > > instead.\n> > \n> > My concern is in subtracting values from an external source (kernels can\n> > have bugs), from an unsigned int, and guaranteeing that it will never\n> > underflow. Which would mean \"if (!remaining)\" would not be able to catch\n> > the issue.\n> \n> metadata.planes()[i].bytesused =\n> std::min(plane.length, remaining);\n> \n> If the kernel doesn't give us enough data, remaining will be smaller\n> than plane.length. metadata.planes()[i].bytesused will thus be equal to\n> remaining.\n> \n> remaining -= metadata.planes()[i].bytesused;\n> \n> And here remaining will become zero. I don't see where the issue is.\n\nThe issue was me believing that the .bytesused values were coming from\nthe kernel, and that there could be more than one value coming from\nthere.\n\nNow that I see more context after applying the patch manually, (Is there\nnot a better way to see more context?) I can see that we can only enter\ninto this statement if numV4l2Planes == 1, so there can be only one\nplane from the perspective of the kernel anyway.\n\nI'll keep the onus on you though.\n\nAs long as you can guarantee there is no way for 'remaining' to be\nsubtracted below zero you can have a \n\nReviewed-by: Kieran Bingham \n\n:-)\n\n\n\n\n> \n> > If you can guarantee that there is no possibility for a kernel bug here,\n> > then fine, but otherwise, I think a signed int is safer code.\n> > \n> > \n> > > > With that,\n> > > > \n> > > > Reviewed-by: Kieran Bingham \n> > > > \n> > > > > }\n> > > > > } else if (multiPlanar) {\n> > > > > /*\n> > > > > \n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2A66EC323E\n\tfor ;\n\tMon, 18 Oct 2021 11:31:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 74A7B68F59;\n\tMon, 18 Oct 2021 13:31:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A04E368F56\n\tfor ;\n\tMon, 18 Oct 2021 13:31:46 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 33B4F18F1;\n\tMon, 18 Oct 2021 13:31:46 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"DCbB3Wd8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634556706;\n\tbh=p6rd9XaipBvS5GhI+hMVYh4whLKBCQRupg6PB4vitQc=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=DCbB3Wd87kId55l+aGAPWiWmDfq2WnkfH66tqemsNYtE7Qe+tIXoV88rnyRhDpM9f\n\tbrhHH6wAJOlnBMhskSBlZxqwmn2bTQaz/FLcVRQbzYzpp5hE/aUidcSUpFQH6h1RBz\n\ta7kbTV5mhtQ+UwQ4VsrXLYbyfXdNojVzdGWPy6lI=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211008101556.22987-1-laurent.pinchart@ideasonboard.com>\n\t<163404367062.2976390.11310721362997315405@Monstersaurus>\n\t\n\t<163420167979.3654017.6851648647582244673@Monstersaurus>\n\t","From":"Kieran Bingham ","To":"Laurent Pinchart ","Date":"Mon, 18 Oct 2021 12:31:43 +0100","Message-ID":"<163455670384.1018219.5019350698445942926@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH] libcamera: v4l2_videodevice: Improve\n\tdebugging when buffer is too small","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]