[{"id":19950,"web_url":"https://patchwork.libcamera.org/comment/19950/","msgid":"","date":"2021-09-28T21:58:27","subject":"Re: [libcamera-devel] [PATCH] libcamera: Fix crash caused by\n\treading uninitialised delayed controls","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nOn Tue, Sep 28, 2021 at 04:36:34PM +0100, David Plowman wrote:\n> This fixes bug https://bugs.libcamera.org/show_bug.cgi?id=74.\n\nThis should be moved before the Signed-off-by line, with\n\nBug: https://bugs.libcamera.org/show_bug.cgi?id=74.\n\n> The cause is that we read out delayed values using a frame's sequence\n> number (DelayedControls::get). But we fill the values up\n> (DelayedControls::applyControls) incrementing writeCount by only one\n> even if the sequence number has jumped by several since last\n> time. This is exactly what happens when frames are being dropped.\n> \n> So the fix is to increment writeCount by \"as much as the sequence\n> number has jumped since last time\", which means that we just follow\n> the sequence number directly.\n> \n> Signed-off-by: David Plowman \n> ---\n> src/libcamera/delayed_controls.cpp | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> index 90ce7e0b..9667187e 100644\n> --- a/src/libcamera/delayed_controls.cpp\n> +++ b/src/libcamera/delayed_controls.cpp\n> @@ -279,7 +279,7 @@ void DelayedControls::applyControls(uint32_t sequence)\n> \t\t}\n> \t}\n> \n> -\twriteCount_++;\n> +\twriteCount_ = sequence - firstSequence_ + 1;\n\nI'm always confused when I read this file, I think one day I'll dive\ndeep and do something about it :-) Until then, this looks good to me.\n\nReviewed-by: Laurent Pinchart \n\nA second pair of eyes would be nice (and testing on IPU3 too).\n\n> \n> \twhile (writeCount_ > queueCount_) {\n> \t\tLOG(DelayedControls, Debug)","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 A0240BDC71\n\tfor ;\n\tTue, 28 Sep 2021 21:58:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DEFE469191;\n\tTue, 28 Sep 2021 23:58:30 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 83A4869188\n\tfor ;\n\tTue, 28 Sep 2021 23:58:29 +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 F3F533F1;\n\tTue, 28 Sep 2021 23:58:28 +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=\"nXuIQ0XJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632866309;\n\tbh=dHosX1aXvZPg6CGhbbLdLgjPMj/7t5qW8g2Oyqzi+Vg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nXuIQ0XJsdVLtvwi2q20k1j+EncmP/9gFYl1C2uoYVWcrH2lHPXWx4z3cHJ3Qj70e\n\t9HLk+40LA0K2+A8GkxqVfEmu48QzX5Eq666O5CCJOOhJVgGdXMZz3g8E8MSNsV4VVJ\n\t5q9BEmLBPmHnFWaZFStPVi9syXAk48gsdx9XiZz0=","Date":"Wed, 29 Sep 2021 00:58:27 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<20210928153634.5864-1-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210928153634.5864-1-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH] libcamera: Fix crash caused by\n\treading uninitialised delayed controls","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":19969,"web_url":"https://patchwork.libcamera.org/comment/19969/","msgid":"","date":"2021-09-29T07:46:02","subject":"Re: [libcamera-devel] [PATCH] libcamera: Fix crash caused by\n\treading uninitialised delayed controls","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nThank you for your patch.\n\nOn Tue, 28 Sept 2021 at 16:36, David Plowman \nwrote:\n\n> This fixes bug https://bugs.libcamera.org/show_bug.cgi?id=74.\n>\n> The cause is that we read out delayed values using a frame's sequence\n> number (DelayedControls::get). But we fill the values up\n> (DelayedControls::applyControls) incrementing writeCount by only one\n> even if the sequence number has jumped by several since last\n> time. This is exactly what happens when frames are being dropped.\n>\n> So the fix is to increment writeCount by \"as much as the sequence\n> number has jumped since last time\", which means that we just follow\n> the sequence number directly.\n>\n> Signed-off-by: David Plowman \n>\n\nReviewed-by: Naushir Patuck \nTested-by: Naushir Patuck \n\n\n> ---\n> src/libcamera/delayed_controls.cpp | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/src/libcamera/delayed_controls.cpp\n> b/src/libcamera/delayed_controls.cpp\n> index 90ce7e0b..9667187e 100644\n> --- a/src/libcamera/delayed_controls.cpp\n> +++ b/src/libcamera/delayed_controls.cpp\n> @@ -279,7 +279,7 @@ void DelayedControls::applyControls(uint32_t sequence)\n> }\n> }\n>\n> - writeCount_++;\n> + writeCount_ = sequence - firstSequence_ + 1;\n>\n> while (writeCount_ > queueCount_) {\n> LOG(DelayedControls, Debug)\n> --\n> 2.20.1\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 6D9B3C3243\n\tfor ;\n\tWed, 29 Sep 2021 07:46:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9A39569191;\n\tWed, 29 Sep 2021 09:46:20 +0200 (CEST)","from mail-lf1-x133.google.com (mail-lf1-x133.google.com\n\t[IPv6:2a00:1450:4864:20::133])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0F4296918A\n\tfor ;\n\tWed, 29 Sep 2021 09:46:19 +0200 (CEST)","by mail-lf1-x133.google.com with SMTP id j5so2144006lfg.8\n\tfor ;\n\tWed, 29 Sep 2021 00:46:19 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"E4NPaEJn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=gQT9Ii3aB6mXB5LD78oJD6ioE2OrOV9wGyfs2JcHJh8=;\n\tb=E4NPaEJnc69p/8IoGKJmN/fh2g+OGiVLZ8gtRLwQuQSpe7dPxuOgInXYqduMtMXmjt\n\taLxUeN72jk3JFKMGbgO5fhD+gNXUl+7/XqcaLTr9hlEZU/tXVbmpOkV7LVQ60upTnOUo\n\tE111HsBrewTCKco+HVJ0jIPggOvvkm4FzaUgtXjyVLiPQlyrHsOLLhzd+WjcIR66wwcS\n\tfd2jtWknncohUAn7ftsBVtBMlncPUjVznWRS/iJJlTEeeaCax7DkoRC5GN8xFTaBpKYr\n\thpp4ZTPt6kYg/4raH6v6mQCCvrYalTOVn2+14QI++9GLf6127fv+ED3L06IMLGn3j5ro\n\t2QIg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=gQT9Ii3aB6mXB5LD78oJD6ioE2OrOV9wGyfs2JcHJh8=;\n\tb=6DJ6KSpZ32JOafCcycswzGj7tGEt0Q61TfxTtbsA9zgBjKh1EmNN7gW73+HGKWQl5l\n\tX8FQQIOHiBOwejEqkvOM1LwTBx+3fzwGAkBZVrgscd/2eA2K+tJpqwvu3ezsfI6W/1/3\n\tT+kYTt/leWpsP2P20dp9xAaEwgkkr08jfRSIR1nvsspDfMb9xcnZfC3IgVn1UfZEitin\n\tm+CwtrEX+3seSPWQoBA2VJrRy3T77hSh2OPRbp6Xikxw3+PXOhu163DwyymFnrIzxjFQ\n\tRR1xeW019wSOixt6oIovTt/M0KEmPHlckaowQTyTpWV8QYE4y6RL1VN/Xzs0ZWGeDLJS\n\tDjyw==","X-Gm-Message-State":"AOAM530nimD+40NqVYaFd3iiOVkfyBGlK4+Pk95nJcOwVP74m8wRNjSv\n\t7GM7g4U9Y73aGfCFyKO4caXA0EBdzN4P74aRPZnAGQ==","X-Google-Smtp-Source":"ABdhPJwDNFxcrhabJp2bqSK7s8kF9L+F7jeP2lrduXEp9Eug0ZpZVkCqN/rqkhJn93h7OmvIYeVS2ZM12RJ9/ITmelw=","X-Received":"by 2002:a05:651c:1b8:: with SMTP id\n\tc24mr4909688ljn.520.1632901578227; \n\tWed, 29 Sep 2021 00:46:18 -0700 (PDT)","MIME-Version":"1.0","References":"<20210928153634.5864-1-david.plowman@raspberrypi.com>","In-Reply-To":"<20210928153634.5864-1-david.plowman@raspberrypi.com>","From":"Naushir Patuck ","Date":"Wed, 29 Sep 2021 08:46:02 +0100","Message-ID":"","To":"David Plowman ","Content-Type":"multipart/alternative; boundary=\"0000000000008615b505cd1d864c\"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: Fix crash caused by\n\treading uninitialised delayed controls","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19981,"web_url":"https://patchwork.libcamera.org/comment/19981/","msgid":"<20210929095200.oulf73ku75qdviki@uno.localdomain>","date":"2021-09-29T09:52:00","subject":"Re: [libcamera-devel] [PATCH] libcamera: Fix crash caused by\n\treading uninitialised delayed controls","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hello\n\nOn Wed, Sep 29, 2021 at 12:58:27AM +0300, Laurent Pinchart wrote:\n> Hi David,\n>\n> Thank you for the patch.\n>\n> On Tue, Sep 28, 2021 at 04:36:34PM +0100, David Plowman wrote:\n> > This fixes bug https://bugs.libcamera.org/show_bug.cgi?id=74.\n>\n> This should be moved before the Signed-off-by line, with\n>\n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=74.\n>\n> > The cause is that we read out delayed values using a frame's sequence\n> > number (DelayedControls::get). But we fill the values up\n> > (DelayedControls::applyControls) incrementing writeCount by only one\n> > even if the sequence number has jumped by several since last\n> > time. This is exactly what happens when frames are being dropped.\n> >\n> > So the fix is to increment writeCount by \"as much as the sequence\n> > number has jumped since last time\", which means that we just follow\n> > the sequence number directly.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > src/libcamera/delayed_controls.cpp | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> > index 90ce7e0b..9667187e 100644\n> > --- a/src/libcamera/delayed_controls.cpp\n> > +++ b/src/libcamera/delayed_controls.cpp\n> > @@ -279,7 +279,7 @@ void DelayedControls::applyControls(uint32_t sequence)\n> > \t\t}\n> > \t}\n> >\n> > -\twriteCount_++;\n> > +\twriteCount_ = sequence - firstSequence_ + 1;\n>\n> I'm always confused when I read this file, I think one day I'll dive\n> deep and do something about it :-) Until then, this looks good to me.\n>\n> Reviewed-by: Laurent Pinchart \n>\n> A second pair of eyes would be nice (and testing on IPU3 too).\n\nI've run a few CTS runs and it doesn't seem to introduce regressions.\n\nHowever I'm wondering how this chaneg plays with queueCount\n\n>\n> >\n> > \twhile (writeCount_ > queueCount_) {\n ^\n this\n\nWhat I wonder about is: if a frame is dropped, should we advance the\ncount and 'burn' the controls that were meant for it ? Because in this\ncase it seems to me that if the number of dropped frames exceeds the\nnumber of controls available in the queue, we end up pushing {} but we\nadvance queueCount by one only.\n\nIf it's intended to burn all the controls meant for dropped frames we\nshould probably increment queueCount by the same amount writeCount_\nhas been incremented with.\n\n\n> > \t\tLOG(DelayedControls, Debug)\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 4D284BDC71\n\tfor ;\n\tWed, 29 Sep 2021 09:51:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC54B691AC;\n\tWed, 29 Sep 2021 11:51:14 +0200 (CEST)","from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C58996919D\n\tfor ;\n\tWed, 29 Sep 2021 11:51:13 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 022B12000C;\n\tWed, 29 Sep 2021 09:51:12 +0000 (UTC)"],"Date":"Wed, 29 Sep 2021 11:52:00 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210929095200.oulf73ku75qdviki@uno.localdomain>","References":"<20210928153634.5864-1-david.plowman@raspberrypi.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: Fix crash caused by\n\treading uninitialised delayed controls","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":19982,"web_url":"https://patchwork.libcamera.org/comment/19982/","msgid":"","date":"2021-09-29T10:17:23","subject":"Re: [libcamera-devel] [PATCH] libcamera: Fix crash caused by\n\treading uninitialised delayed controls","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nThanks for the question!\n\nOn Wed, 29 Sept 2021 at 10:51, Jacopo Mondi wrote:\n>\n> Hello\n>\n> On Wed, Sep 29, 2021 at 12:58:27AM +0300, Laurent Pinchart wrote:\n> > Hi David,\n> >\n> > Thank you for the patch.\n> >\n> > On Tue, Sep 28, 2021 at 04:36:34PM +0100, David Plowman wrote:\n> > > This fixes bug https://bugs.libcamera.org/show_bug.cgi?id=74.\n> >\n> > This should be moved before the Signed-off-by line, with\n> >\n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=74.\n> >\n> > > The cause is that we read out delayed values using a frame's sequence\n> > > number (DelayedControls::get). But we fill the values up\n> > > (DelayedControls::applyControls) incrementing writeCount by only one\n> > > even if the sequence number has jumped by several since last\n> > > time. This is exactly what happens when frames are being dropped.\n> > >\n> > > So the fix is to increment writeCount by \"as much as the sequence\n> > > number has jumped since last time\", which means that we just follow\n> > > the sequence number directly.\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > src/libcamera/delayed_controls.cpp | 2 +-\n> > > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> > > index 90ce7e0b..9667187e 100644\n> > > --- a/src/libcamera/delayed_controls.cpp\n> > > +++ b/src/libcamera/delayed_controls.cpp\n> > > @@ -279,7 +279,7 @@ void DelayedControls::applyControls(uint32_t sequence)\n> > > }\n> > > }\n> > >\n> > > - writeCount_++;\n> > > + writeCount_ = sequence - firstSequence_ + 1;\n> >\n> > I'm always confused when I read this file, I think one day I'll dive\n> > deep and do something about it :-) Until then, this looks good to me.\n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> > A second pair of eyes would be nice (and testing on IPU3 too).\n>\n> I've run a few CTS runs and it doesn't seem to introduce regressions.\n>\n> However I'm wondering how this chaneg plays with queueCount\n>\n> >\n> > >\n> > > while (writeCount_ > queueCount_) {\n> ^\n> this\n>\n> What I wonder about is: if a frame is dropped, should we advance the\n> count and 'burn' the controls that were meant for it ? Because in this\n> case it seems to me that if the number of dropped frames exceeds the\n> number of controls available in the queue, we end up pushing {} but we\n> advance queueCount by one only.\n\nThat loop will run, incrementing queueCount (in \"push\") by 1 each\ntime, until it has caught up with writeCount - which I believe to be\nthe correct behaviour. Each call will copy the previous control\nentries to the next one, so anyone who asks for values for a frame we\ndropped will simply get the most recent values that we did see.\n\nEven if the number of dropped frames exceeds the length of the queue\nthe behaviour seems fine - the entire queue will fill up with the last\nseen values.\n\n>\n> If it's intended to burn all the controls meant for dropped frames we\n> should probably increment queueCount by the same amount writeCount_\n> has been incremented with.\n\nNot sure what you mean by \"burn\" the controls? The client code can ask\nfor values from sequence numbers that were never actually seen (if it\nwants), and will get the last known values back. Does that make sense?\n\nBest regards\nDavid\n\n>\n>\n> > > LOG(DelayedControls, Debug)\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 1658EC3243\n\tfor ;\n\tWed, 29 Sep 2021 10:17:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 664AA691AA;\n\tWed, 29 Sep 2021 12:17:36 +0200 (CEST)","from mail-wr1-x42e.google.com (mail-wr1-x42e.google.com\n\t[IPv6:2a00:1450:4864:20::42e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AE3EB69185\n\tfor ;\n\tWed, 29 Sep 2021 12:17:34 +0200 (CEST)","by mail-wr1-x42e.google.com with SMTP id k7so3235597wrd.13\n\tfor ;\n\tWed, 29 Sep 2021 03:17:34 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"ia3+I9Lp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=/1i5KBFgcs0hl+jp90o3OMjWMZEUUrQiI+FHqnXKNnQ=;\n\tb=ia3+I9LpOKZsfu3/0MtxLzMbshYqG432Rh/QK+gcQ4FkDx6C4q08NkFWPCyb24SfAX\n\tLZ03aczx3N/K+ummtpZC8ONhXiWjzhRNjndrS7fPb1CE1OfaCWQOTVN3ee9T+RywZAF2\n\t6FXzdAOn47WLGK8zg4ZY0tuHpd8qZlxzCFAAoeOdGO7R2geyys5YE7D+y8lrYv+gezHL\n\tSuJuQkja1vBvTe0Xdi6KrOtzeBo2F6//iPaoSH/xnZxtYkzYUF5xkf8DjhW7mi25Letj\n\tAOVJ9TMH9I814BwLCAg8ncIDZdAvpymSy9m2Ed0Xyqm9ptyPkehGhYdNTXkNTW9kYh6Q\n\tx7Cw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=/1i5KBFgcs0hl+jp90o3OMjWMZEUUrQiI+FHqnXKNnQ=;\n\tb=WWxkN6Kd91XmE272lycLGdSKHX/B6Eko4+nsZ6TaSaXLMGuP2TDEpbU23hV7u2nEau\n\tEoTTuF5F7B1HpI4Q/iR2KGfM4fxQZkEgudg6dzCmfZmJOtwsJ3Jenhk3mKp/SfuXMQWr\n\tSUvxTf89R27UAJEroOHJXxn7rXa+Qw5NBw7ckZdHydiB5di30StOT4fyljE+6VgP9SW8\n\tIug8CTGMN8f/ImS5ALi/hIQeeor0hJaMgEFrhkdPnnzE+Gtkz/ejee9sDOua04aF5OEK\n\twY6xtsR0ktxhtPaF+FyiphvbSPDMf2Rma2SZtyir4rToXNCvQ1NjVO6+zGR1A4K4ufHG\n\tCrNA==","X-Gm-Message-State":"AOAM5301XFWn8UUJ73fMJQwe+egBd49nS7JLVAouV+tF3cwF1I9Ocft0\n\tUZiJHYo1wQspyvZ4HViqkuJdebolEwqK1/n2mnOjYg==","X-Google-Smtp-Source":"ABdhPJwSZuMMrbu0NG3aNiEsCisP0elVS/sC5sl9Z89Chil9hrisAe1/mY/SxCDGHMGe6jqLnGDF5Zl/XM3He2eGHbk=","X-Received":"by 2002:a5d:4212:: with SMTP id\n\tn18mr5659896wrq.162.1632910654197; \n\tWed, 29 Sep 2021 03:17:34 -0700 (PDT)","MIME-Version":"1.0","References":"<20210928153634.5864-1-david.plowman@raspberrypi.com>\n\t\n\t<20210929095200.oulf73ku75qdviki@uno.localdomain>","In-Reply-To":"<20210929095200.oulf73ku75qdviki@uno.localdomain>","From":"David Plowman ","Date":"Wed, 29 Sep 2021 11:17:23 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: Fix crash caused by\n\treading uninitialised delayed controls","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19983,"web_url":"https://patchwork.libcamera.org/comment/19983/","msgid":"<20210929103131.m3ccfov75wwmo6v7@uno.localdomain>","date":"2021-09-29T10:31:31","subject":"Re: [libcamera-devel] [PATCH] libcamera: Fix crash caused by\n\treading uninitialised delayed controls","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David\n\nOn Wed, Sep 29, 2021 at 11:17:23AM +0100, David Plowman wrote:\n> Hi Jacopo\n>\n> Thanks for the question!\n>\n> On Wed, 29 Sept 2021 at 10:51, Jacopo Mondi wrote:\n> >\n> > Hello\n> >\n> > On Wed, Sep 29, 2021 at 12:58:27AM +0300, Laurent Pinchart wrote:\n> > > Hi David,\n> > >\n> > > Thank you for the patch.\n> > >\n> > > On Tue, Sep 28, 2021 at 04:36:34PM +0100, David Plowman wrote:\n> > > > This fixes bug https://bugs.libcamera.org/show_bug.cgi?id=74.\n> > >\n> > > This should be moved before the Signed-off-by line, with\n> > >\n> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=74.\n> > >\n> > > > The cause is that we read out delayed values using a frame's sequence\n> > > > number (DelayedControls::get). But we fill the values up\n> > > > (DelayedControls::applyControls) incrementing writeCount by only one\n> > > > even if the sequence number has jumped by several since last\n> > > > time. This is exactly what happens when frames are being dropped.\n> > > >\n> > > > So the fix is to increment writeCount by \"as much as the sequence\n> > > > number has jumped since last time\", which means that we just follow\n> > > > the sequence number directly.\n> > > >\n> > > > Signed-off-by: David Plowman \n> > > > ---\n> > > > src/libcamera/delayed_controls.cpp | 2 +-\n> > > > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> > > > index 90ce7e0b..9667187e 100644\n> > > > --- a/src/libcamera/delayed_controls.cpp\n> > > > +++ b/src/libcamera/delayed_controls.cpp\n> > > > @@ -279,7 +279,7 @@ void DelayedControls::applyControls(uint32_t sequence)\n> > > > }\n> > > > }\n> > > >\n> > > > - writeCount_++;\n> > > > + writeCount_ = sequence - firstSequence_ + 1;\n> > >\n> > > I'm always confused when I read this file, I think one day I'll dive\n> > > deep and do something about it :-) Until then, this looks good to me.\n> > >\n> > > Reviewed-by: Laurent Pinchart \n> > >\n> > > A second pair of eyes would be nice (and testing on IPU3 too).\n> >\n> > I've run a few CTS runs and it doesn't seem to introduce regressions.\n> >\n> > However I'm wondering how this chaneg plays with queueCount\n> >\n> > >\n> > > >\n> > > > while (writeCount_ > queueCount_) {\n> > ^\n> > this\n> >\n> > What I wonder about is: if a frame is dropped, should we advance the\n> > count and 'burn' the controls that were meant for it ? Because in this\n> > case it seems to me that if the number of dropped frames exceeds the\n> > number of controls available in the queue, we end up pushing {} but we\n> > advance queueCount by one only.\n>\n> That loop will run, incrementing queueCount (in \"push\") by 1 each\n> time, until it has caught up with writeCount - which I believe to be\n> the correct behaviour. Each call will copy the previous control\n> entries to the next one, so anyone who asks for values for a frame we\n> dropped will simply get the most recent values that we did see.\n>\n> Even if the number of dropped frames exceeds the length of the queue\n> the behaviour seems fine - the entire queue will fill up with the last\n> seen values.\n>\n> >\n> > If it's intended to burn all the controls meant for dropped frames we\n> > should probably increment queueCount by the same amount writeCount_\n> > has been incremented with.\n>\n> Not sure what you mean by \"burn\" the controls? The client code can ask\n> for values from sequence numbers that were never actually seen (if it\n> wants), and will get the last known values back. Does that make sense?\n\nLet's assume that we have 3 controls in the values_ queue waiting to\nbe applied to the 'next' frames\n\nwriteCount = x\nqueueCount = x + 3\nvalues_ = [ C(x+1), C(x+2), C(x+3) ]\n\nWe drop 4 frames hence\n\nwriteCount = x + 4\n\nand we have to advance queueCount by pushing {} and copying the last\nvalues\n\nvalues_ = [ C(x+1), C(x+2), C(x+3), C(x+3) ]\n\nWhat I meant by burnt is that if I'm not mistaken C(x+1) and C(x+2)\nare lost and I wonder if we shouldn't instead\n\nvalues_ = [ C(x+1), C(x+2), C(x+3), C(x+1), C(x+2), C(x+3) ]\n\nwhen frames are dropped.\n\nOr have I got it all wrong ?\n\n\n>\n> Best regards\n> David\n>\n> >\n> >\n> > > > LOG(DelayedControls, Debug)\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 B87E4BDC71\n\tfor ;\n\tWed, 29 Sep 2021 10:30:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 26F6A691AA;\n\tWed, 29 Sep 2021 12:30:46 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 23D1069185\n\tfor ;\n\tWed, 29 Sep 2021 12:30:45 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 5D31D100010;\n\tWed, 29 Sep 2021 10:30:44 +0000 (UTC)"],"Date":"Wed, 29 Sep 2021 12:31:31 +0200","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20210929103131.m3ccfov75wwmo6v7@uno.localdomain>","References":"<20210928153634.5864-1-david.plowman@raspberrypi.com>\n\t\n\t<20210929095200.oulf73ku75qdviki@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: Fix crash caused by\n\treading uninitialised delayed controls","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19984,"web_url":"https://patchwork.libcamera.org/comment/19984/","msgid":"","date":"2021-09-29T10:51:24","subject":"Re: [libcamera-devel] [PATCH] libcamera: Fix crash caused by\n\treading uninitialised delayed controls","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nThanks for clarifying!\n\nOn Wed, 29 Sept 2021 at 11:30, Jacopo Mondi wrote:\n>\n> Hi David\n>\n> On Wed, Sep 29, 2021 at 11:17:23AM +0100, David Plowman wrote:\n> > Hi Jacopo\n> >\n> > Thanks for the question!\n> >\n> > On Wed, 29 Sept 2021 at 10:51, Jacopo Mondi wrote:\n> > >\n> > > Hello\n> > >\n> > > On Wed, Sep 29, 2021 at 12:58:27AM +0300, Laurent Pinchart wrote:\n> > > > Hi David,\n> > > >\n> > > > Thank you for the patch.\n> > > >\n> > > > On Tue, Sep 28, 2021 at 04:36:34PM +0100, David Plowman wrote:\n> > > > > This fixes bug https://bugs.libcamera.org/show_bug.cgi?id=74.\n> > > >\n> > > > This should be moved before the Signed-off-by line, with\n> > > >\n> > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=74.\n> > > >\n> > > > > The cause is that we read out delayed values using a frame's sequence\n> > > > > number (DelayedControls::get). But we fill the values up\n> > > > > (DelayedControls::applyControls) incrementing writeCount by only one\n> > > > > even if the sequence number has jumped by several since last\n> > > > > time. This is exactly what happens when frames are being dropped.\n> > > > >\n> > > > > So the fix is to increment writeCount by \"as much as the sequence\n> > > > > number has jumped since last time\", which means that we just follow\n> > > > > the sequence number directly.\n> > > > >\n> > > > > Signed-off-by: David Plowman \n> > > > > ---\n> > > > > src/libcamera/delayed_controls.cpp | 2 +-\n> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> > > > > index 90ce7e0b..9667187e 100644\n> > > > > --- a/src/libcamera/delayed_controls.cpp\n> > > > > +++ b/src/libcamera/delayed_controls.cpp\n> > > > > @@ -279,7 +279,7 @@ void DelayedControls::applyControls(uint32_t sequence)\n> > > > > }\n> > > > > }\n> > > > >\n> > > > > - writeCount_++;\n> > > > > + writeCount_ = sequence - firstSequence_ + 1;\n> > > >\n> > > > I'm always confused when I read this file, I think one day I'll dive\n> > > > deep and do something about it :-) Until then, this looks good to me.\n> > > >\n> > > > Reviewed-by: Laurent Pinchart \n> > > >\n> > > > A second pair of eyes would be nice (and testing on IPU3 too).\n> > >\n> > > I've run a few CTS runs and it doesn't seem to introduce regressions.\n> > >\n> > > However I'm wondering how this chaneg plays with queueCount\n> > >\n> > > >\n> > > > >\n> > > > > while (writeCount_ > queueCount_) {\n> > > ^\n> > > this\n> > >\n> > > What I wonder about is: if a frame is dropped, should we advance the\n> > > count and 'burn' the controls that were meant for it ? Because in this\n> > > case it seems to me that if the number of dropped frames exceeds the\n> > > number of controls available in the queue, we end up pushing {} but we\n> > > advance queueCount by one only.\n> >\n> > That loop will run, incrementing queueCount (in \"push\") by 1 each\n> > time, until it has caught up with writeCount - which I believe to be\n> > the correct behaviour. Each call will copy the previous control\n> > entries to the next one, so anyone who asks for values for a frame we\n> > dropped will simply get the most recent values that we did see.\n> >\n> > Even if the number of dropped frames exceeds the length of the queue\n> > the behaviour seems fine - the entire queue will fill up with the last\n> > seen values.\n> >\n> > >\n> > > If it's intended to burn all the controls meant for dropped frames we\n> > > should probably increment queueCount by the same amount writeCount_\n> > > has been incremented with.\n> >\n> > Not sure what you mean by \"burn\" the controls? The client code can ask\n> > for values from sequence numbers that were never actually seen (if it\n> > wants), and will get the last known values back. Does that make sense?\n>\n> Let's assume that we have 3 controls in the values_ queue waiting to\n> be applied to the 'next' frames\n>\n> writeCount = x\n> queueCount = x + 3\n> values_ = [ C(x+1), C(x+2), C(x+3) ]\n>\n> We drop 4 frames hence\n>\n> writeCount = x + 4\n>\n> and we have to advance queueCount by pushing {} and copying the last\n> values\n>\n> values_ = [ C(x+1), C(x+2), C(x+3), C(x+3) ]\n\nYes, this is what we get.\n\n>\n> What I meant by burnt is that if I'm not mistaken C(x+1) and C(x+2)\n> are lost and I wonder if we shouldn't instead\n\nI wouldn't describe them as \"lost\". The client code can still access\nthem with get(x+1), get(x+3) etc.\n\n>\n> values_ = [ C(x+1), C(x+2), C(x+3), C(x+1), C(x+2), C(x+3) ]\n\nI think the trouble with this is that DelayedControls are more of an\narray than a queue. Don't think of it in terms of accessing \"the most\nrecent values\" or \"the values from n frames ago\". We access them with\nan array index, which happens to be the actual frame sequence number\n(more or less). (The fact that only the 16 most recent items are\nstored is kind of an \"implementation detail\".)\n\nSo with those values, the code would, for frame x+4, call get(x+4) and\ncome back with C(x+1) - not the right values.\n\n>\n> when frames are dropped.\n>\n> Or have I got it all wrong ?\n\nHave I explained it any better? :)\n\nDavid\n\n>\n>\n> >\n> > Best regards\n> > David\n> >\n> > >\n> > >\n> > > > > LOG(DelayedControls, Debug)\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 D6CBFC3243\n\tfor ;\n\tWed, 29 Sep 2021 10:51:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 58A0E691AA;\n\tWed, 29 Sep 2021 12:51:37 +0200 (CEST)","from mail-wr1-x42c.google.com (mail-wr1-x42c.google.com\n\t[IPv6:2a00:1450:4864:20::42c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9BFEF69185\n\tfor ;\n\tWed, 29 Sep 2021 12:51:35 +0200 (CEST)","by mail-wr1-x42c.google.com with SMTP id t18so3574149wrb.0\n\tfor ;\n\tWed, 29 Sep 2021 03:51:35 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"ZzqIWcEf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=NAuFFnDisdOslGeXz+ZKN8E+b0UACMkC0DVO73MF2hg=;\n\tb=ZzqIWcEfRnS6WZV4Bw8rhZxd3vWIoGE287BmmHWJwhPz4DGU7jxc4ScOEUkAGqwq/j\n\t+43QEGtC6NGoZ9p4npth7qSiLKUcIU2OBKaXQOfTJwNi6qxpbVNdAe7cWo3u8IABtKH3\n\tH1yu1GlS2i+8wSSDqrWQ/bwEicvQAXmBe04fotWTXWuGoRKc4CnqVRX0m+3qiTCz/KXK\n\tlLqEIekpgDLPZq+KMvDA+rGhWK821LCl7JCeU36LpkzQahV2ZAm/wMysxloTbA09tZuG\n\tU3iliZ0DG9ri1uF0nBGJuc5Qp2xC3e6fQEZCnlmpra8fBYLSAWHqrAAgAr9Bf6ns2hD2\n\tAddQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=NAuFFnDisdOslGeXz+ZKN8E+b0UACMkC0DVO73MF2hg=;\n\tb=MojGD/rLo+viU/U5lXdxf02mqDjDsSUBy7DMj1c7wKJBqksaa1DxfEyZK7poRgN8WJ\n\t9s1F0XdVsjC8AMM4X+nlBMPmcCvXsJQCH2yTGRIFzPLGypcUGTDdFGhg5kerlRy93Eiq\n\t8/jx+GPYK9A15NNdFt/Cmnpj382yPk4Fxl0xSeldySeFLe9InGNw6JYI5AhxLrgGZQVi\n\taTi+80/mqT8PGK4OsTqVmdAvQot2RgcFK17MH2zssTHRHDxqL9ti0qkDbd+MR4LA2zPp\n\tqjgSWe7PqOr1q+0RpHnf5M1z8/hhuxZ0XGZwRtZLerVVyJC3iHy65vuSCWN/n636EtdY\n\tHo8A==","X-Gm-Message-State":"AOAM533Wd7XKkjLS0DqszfTijDtwfdCC9ogunL6UIajoScG8bABIzcof\n\tV4RqUvMIlARODJ6AevaViLtpVKzGxBIj9u0xnlx+/DfOViYJew==","X-Google-Smtp-Source":"ABdhPJwbsEsdClaRa1VPZmxwEUE4NX8j2pR5aSCGdHMg8XF1IXqo3bg2Hp2Hw3UkQi5GxoHymiPgs81yoNgVf8pPUU0=","X-Received":"by 2002:a05:6000:18a4:: with SMTP id\n\tb4mr6113428wri.288.1632912695211; \n\tWed, 29 Sep 2021 03:51:35 -0700 (PDT)","MIME-Version":"1.0","References":"<20210928153634.5864-1-david.plowman@raspberrypi.com>\n\t\n\t<20210929095200.oulf73ku75qdviki@uno.localdomain>\n\t\n\t<20210929103131.m3ccfov75wwmo6v7@uno.localdomain>","In-Reply-To":"<20210929103131.m3ccfov75wwmo6v7@uno.localdomain>","From":"David Plowman ","Date":"Wed, 29 Sep 2021 11:51:24 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: Fix crash caused by\n\treading uninitialised delayed controls","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19986,"web_url":"https://patchwork.libcamera.org/comment/19986/","msgid":"<20210929112313.tysgl6x2oahop4os@uno.localdomain>","date":"2021-09-29T11:23:13","subject":"Re: [libcamera-devel] [PATCH] libcamera: Fix crash caused by\n\treading uninitialised delayed controls","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David,\n\nOn Wed, Sep 29, 2021 at 11:51:24AM +0100, David Plowman wrote:\n> Hi Jacopo\n>\n> Thanks for clarifying!\n>\n> On Wed, 29 Sept 2021 at 11:30, Jacopo Mondi wrote:\n> >\n> > Hi David\n> >\n> > On Wed, Sep 29, 2021 at 11:17:23AM +0100, David Plowman wrote:\n> > > Hi Jacopo\n> > >\n> > > Thanks for the question!\n> > >\n> > > On Wed, 29 Sept 2021 at 10:51, Jacopo Mondi wrote:\n> > > >\n> > > > Hello\n> > > >\n> > > > On Wed, Sep 29, 2021 at 12:58:27AM +0300, Laurent Pinchart wrote:\n> > > > > Hi David,\n> > > > >\n> > > > > Thank you for the patch.\n> > > > >\n> > > > > On Tue, Sep 28, 2021 at 04:36:34PM +0100, David Plowman wrote:\n> > > > > > This fixes bug https://bugs.libcamera.org/show_bug.cgi?id=74.\n> > > > >\n> > > > > This should be moved before the Signed-off-by line, with\n> > > > >\n> > > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=74.\n> > > > >\n> > > > > > The cause is that we read out delayed values using a frame's sequence\n> > > > > > number (DelayedControls::get). But we fill the values up\n> > > > > > (DelayedControls::applyControls) incrementing writeCount by only one\n> > > > > > even if the sequence number has jumped by several since last\n> > > > > > time. This is exactly what happens when frames are being dropped.\n> > > > > >\n> > > > > > So the fix is to increment writeCount by \"as much as the sequence\n> > > > > > number has jumped since last time\", which means that we just follow\n> > > > > > the sequence number directly.\n> > > > > >\n> > > > > > Signed-off-by: David Plowman \n> > > > > > ---\n> > > > > > src/libcamera/delayed_controls.cpp | 2 +-\n> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)\n> > > > > >\n> > > > > > diff --git a/src/libcamera/delayed_controls.cpp b/src/libcamera/delayed_controls.cpp\n> > > > > > index 90ce7e0b..9667187e 100644\n> > > > > > --- a/src/libcamera/delayed_controls.cpp\n> > > > > > +++ b/src/libcamera/delayed_controls.cpp\n> > > > > > @@ -279,7 +279,7 @@ void DelayedControls::applyControls(uint32_t sequence)\n> > > > > > }\n> > > > > > }\n> > > > > >\n> > > > > > - writeCount_++;\n> > > > > > + writeCount_ = sequence - firstSequence_ + 1;\n> > > > >\n> > > > > I'm always confused when I read this file, I think one day I'll dive\n> > > > > deep and do something about it :-) Until then, this looks good to me.\n> > > > >\n> > > > > Reviewed-by: Laurent Pinchart \n> > > > >\n> > > > > A second pair of eyes would be nice (and testing on IPU3 too).\n> > > >\n> > > > I've run a few CTS runs and it doesn't seem to introduce regressions.\n> > > >\n> > > > However I'm wondering how this chaneg plays with queueCount\n> > > >\n> > > > >\n> > > > > >\n> > > > > > while (writeCount_ > queueCount_) {\n> > > > ^\n> > > > this\n> > > >\n> > > > What I wonder about is: if a frame is dropped, should we advance the\n> > > > count and 'burn' the controls that were meant for it ? Because in this\n> > > > case it seems to me that if the number of dropped frames exceeds the\n> > > > number of controls available in the queue, we end up pushing {} but we\n> > > > advance queueCount by one only.\n> > >\n> > > That loop will run, incrementing queueCount (in \"push\") by 1 each\n> > > time, until it has caught up with writeCount - which I believe to be\n> > > the correct behaviour. Each call will copy the previous control\n> > > entries to the next one, so anyone who asks for values for a frame we\n> > > dropped will simply get the most recent values that we did see.\n> > >\n> > > Even if the number of dropped frames exceeds the length of the queue\n> > > the behaviour seems fine - the entire queue will fill up with the last\n> > > seen values.\n> > >\n> > > >\n> > > > If it's intended to burn all the controls meant for dropped frames we\n> > > > should probably increment queueCount by the same amount writeCount_\n> > > > has been incremented with.\n> > >\n> > > Not sure what you mean by \"burn\" the controls? The client code can ask\n> > > for values from sequence numbers that were never actually seen (if it\n> > > wants), and will get the last known values back. Does that make sense?\n> >\n> > Let's assume that we have 3 controls in the values_ queue waiting to\n> > be applied to the 'next' frames\n> >\n> > writeCount = x\n> > queueCount = x + 3\n> > values_ = [ C(x+1), C(x+2), C(x+3) ]\n> >\n> > We drop 4 frames hence\n> >\n> > writeCount = x + 4\n> >\n> > and we have to advance queueCount by pushing {} and copying the last\n> > values\n> >\n> > values_ = [ C(x+1), C(x+2), C(x+3), C(x+3) ]\n>\n> Yes, this is what we get.\n>\n> >\n> > What I meant by burnt is that if I'm not mistaken C(x+1) and C(x+2)\n> > are lost and I wonder if we shouldn't instead\n>\n> I wouldn't describe them as \"lost\". The client code can still access\n> them with get(x+1), get(x+3) etc.\n>\n> >\n> > values_ = [ C(x+1), C(x+2), C(x+3), C(x+1), C(x+2), C(x+3) ]\n>\n> I think the trouble with this is that DelayedControls are more of an\n> array than a queue. Don't think of it in terms of accessing \"the most\n> recent values\" or \"the values from n frames ago\". We access them with\n> an array index, which happens to be the actual frame sequence number\n> (more or less). (The fact that only the 16 most recent items are\n> stored is kind of an \"implementation detail\".)\n>\n> So with those values, the code would, for frame x+4, call get(x+4) and\n> come back with C(x+1) - not the right values.\n>\n\nMy understanding was that ideally if we drop a frame, the setting that\nwere applied to it should be re-applied to the next one, and the whole\n'queue' is shifted by one place. That's not the case as I understand\nit. Thanks for the explanation then!\n\nTested-by: Jacopo Mondi \nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n\n> >\n> > when frames are dropped.\n> >\n> > Or have I got it all wrong ?\n>\n> Have I explained it any better? :)\n>\n> David\n>\n> >\n> >\n> > >\n> > > Best regards\n> > > David\n> > >\n> > > >\n> > > >\n> > > > > > LOG(DelayedControls, Debug)\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 A5222C3243\n\tfor ;\n\tWed, 29 Sep 2021 11:22:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E718D691AC;\n\tWed, 29 Sep 2021 13:22:27 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2B7A269185\n\tfor ;\n\tWed, 29 Sep 2021 13:22:27 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 6F9E11BF20B;\n\tWed, 29 Sep 2021 11:22:26 +0000 (UTC)"],"Date":"Wed, 29 Sep 2021 13:23:13 +0200","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20210929112313.tysgl6x2oahop4os@uno.localdomain>","References":"<20210928153634.5864-1-david.plowman@raspberrypi.com>\n\t\n\t<20210929095200.oulf73ku75qdviki@uno.localdomain>\n\t\n\t<20210929103131.m3ccfov75wwmo6v7@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH] libcamera: Fix crash caused by\n\treading uninitialised delayed controls","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]