[{"id":18577,"web_url":"https://patchwork.libcamera.org/comment/18577/","msgid":"<YQv9X00T3T11ddhX@pendragon.ideasonboard.com>","date":"2021-08-05T15:01:51","subject":"Re: [libcamera-devel] [PATCH] android: yuv: Fix wrong access of\n\tsource buffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Thu, Aug 05, 2021 at 10:28:05PM +0900, Hirokazu Honda wrote:\n> File descriptors of FrameBuffer given in PostProcessor::process()\n> for all the planes point the same buffer. To access the beginning\n> of the second or later plane, it is necessary to add offsets to\n> the beginning of the buffer.\n> Fix the wrong access to the second plane of NV12 in\n> PostProcessorYuv.\n\nThis is an issue that comes from V4L2, where NV12, a multi-planar\nformat, can be stored with the two planes contiguously in the same\ndmabuf, or in separate dmabufs. We have two V4L2 pixel formats for those\n(for instance, for NV12, that's V4L2_PIX_FMT_NV12 and\nV4L2_PIX_FMT_NV12M), and two APIs, the V4L2 single-planar API that only\nsupports the single dmabuf format, and the V4L2 multi-planar API that\nsupports either formats.\n\nThis patch will work with V4L2_PIX_FMT_NV12 but not with\nV4L2_PIX_FMT_NV12M. Let's not force applications to deal with this mess,\nI don't want to expose it through the libcamera API. We should instead\nhandle it in the MappedBuffer class, so that maps()[1] will return the\ncorrect value.\n\nIn order to support both NV12 and NV12M, we'll have to add an offset\nfield to FrameBuffer::Plane, but in the short term, as we only support\nthe single-dmabuf formats in our pipeline handlers, I think we can\nhardcode the calculation below in the MappedBuffer class (I haven't\ntried it though, so there may be something I'm missing).\n\n> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> ---\n>  src/android/yuv/post_processor_yuv.cpp | 7 +++++--\n>  1 file changed, 5 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp\n> index 772e805b..3b801e96 100644\n> --- a/src/android/yuv/post_processor_yuv.cpp\n> +++ b/src/android/yuv/post_processor_yuv.cpp\n> @@ -62,9 +62,12 @@ int PostProcessorYuv::process(const FrameBuffer &source,\n>  \t\treturn -EINVAL;\n>  \t}\n>  \n> -\tint ret = libyuv::NV12Scale(sourceMapped.maps()[0].data(),\n> +\tconst uint8_t *sourceY = sourceMapped.maps()[0].data();\n> +\tconst uint8_t *sourceUV = sourceY + sourceStride_[0] * sourceSize_.height;\n> +\n> +\tint ret = libyuv::NV12Scale(sourceY,\n>  \t\t\t\t    sourceStride_[0],\n> -\t\t\t\t    sourceMapped.maps()[1].data(),\n> +\t\t\t\t    sourceUV,\n>  \t\t\t\t    sourceStride_[1],\n>  \t\t\t\t    sourceSize_.width, sourceSize_.height,\n>  \t\t\t\t    destination->plane(0).data(),","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 6DFA9C3237\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  5 Aug 2021 15:02:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D77E068811;\n\tThu,  5 Aug 2021 17:02:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2B65A6026D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  5 Aug 2021 17:02:04 +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 A1333E04;\n\tThu,  5 Aug 2021 17:02:03 +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=\"c+/nFPL/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628175723;\n\tbh=tcs7e/9uzKUK+GCevwCVP0VcrDpnP78hi70LfsJo+BM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=c+/nFPL/LITgCjOaYoL1MEEoRZegpVAzZ3eTUWWT3Xh+pX/gsm1w83bVznV6Z2eXB\n\t36MS+WCDhTZmrbpDLTkQz5cD6Hh5Pam4Iwvk9XZU9uNiMAfwb3TQr99poqidmHMMDb\n\t1+9OEoA6CnytlhVJgF4UoUyjiC1xEV1M0PfCwaa8=","Date":"Thu, 5 Aug 2021 18:01:51 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Hirokazu Honda <hiroh@chromium.org>","Message-ID":"<YQv9X00T3T11ddhX@pendragon.ideasonboard.com>","References":"<20210805132805.824754-1-hiroh@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210805132805.824754-1-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH] android: yuv: Fix wrong access of\n\tsource buffer","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18578,"web_url":"https://patchwork.libcamera.org/comment/18578/","msgid":"<CAO5uPHPJkmg9TSOEsXBpDseL+6qQ-4yg7ZJwN4WSyV2rL8g+EA@mail.gmail.com>","date":"2021-08-05T15:21:41","subject":"Re: [libcamera-devel] [PATCH] android: yuv: Fix wrong access of\n\tsource buffer","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Fri, Aug 6, 2021 at 12:02 AM Laurent Pinchart\n<laurent.pinchart@ideasonboard.com> wrote:\n>\n> Hi Hiro,\n>\n> Thank you for the patch.\n>\n> On Thu, Aug 05, 2021 at 10:28:05PM +0900, Hirokazu Honda wrote:\n> > File descriptors of FrameBuffer given in PostProcessor::process()\n> > for all the planes point the same buffer. To access the beginning\n> > of the second or later plane, it is necessary to add offsets to\n> > the beginning of the buffer.\n> > Fix the wrong access to the second plane of NV12 in\n> > PostProcessorYuv.\n>\n> This is an issue that comes from V4L2, where NV12, a multi-planar\n> format, can be stored with the two planes contiguously in the same\n> dmabuf, or in separate dmabufs. We have two V4L2 pixel formats for those\n> (for instance, for NV12, that's V4L2_PIX_FMT_NV12 and\n> V4L2_PIX_FMT_NV12M), and two APIs, the V4L2 single-planar API that only\n> supports the single dmabuf format, and the V4L2 multi-planar API that\n> supports either formats.\n>\n> This patch will work with V4L2_PIX_FMT_NV12 but not with\n> V4L2_PIX_FMT_NV12M. Let's not force applications to deal with this mess,\n> I don't want to expose it through the libcamera API. We should instead\n> handle it in the MappedBuffer class, so that maps()[1] will return the\n> correct value.\n>\n> In order to support both NV12 and NV12M, we'll have to add an offset\n> field to FrameBuffer::Plane, but in the short term, as we only support\n> the single-dmabuf formats in our pipeline handlers, I think we can\n> hardcode the calculation below in the MappedBuffer class (I haven't\n> tried it though, so there may be something I'm missing).\n>\n\nYeah, that is what I would like to discuss through this patch.\nI agree with handling this MappedBuffer.\nThe current libcamera implementation doesn't handle multi-planar format.\nI filed https://bugs.libcamera.org/show_bug.cgi?id=9 previously.\nShall I also add strides() to MappedBuffer or FrameBuffer?\n\nRegards,\n-Hiro\n> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> > ---\n> >  src/android/yuv/post_processor_yuv.cpp | 7 +++++--\n> >  1 file changed, 5 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp\n> > index 772e805b..3b801e96 100644\n> > --- a/src/android/yuv/post_processor_yuv.cpp\n> > +++ b/src/android/yuv/post_processor_yuv.cpp\n> > @@ -62,9 +62,12 @@ int PostProcessorYuv::process(const FrameBuffer &source,\n> >               return -EINVAL;\n> >       }\n> >\n> > -     int ret = libyuv::NV12Scale(sourceMapped.maps()[0].data(),\n> > +     const uint8_t *sourceY = sourceMapped.maps()[0].data();\n> > +     const uint8_t *sourceUV = sourceY + sourceStride_[0] * sourceSize_.height;\n> > +\n> > +     int ret = libyuv::NV12Scale(sourceY,\n> >                                   sourceStride_[0],\n> > -                                 sourceMapped.maps()[1].data(),\n> > +                                 sourceUV,\n> >                                   sourceStride_[1],\n> >                                   sourceSize_.width, sourceSize_.height,\n> >                                   destination->plane(0).data(),\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 76214C3235\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  5 Aug 2021 15:21:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E8BA668811;\n\tThu,  5 Aug 2021 17:21:52 +0200 (CEST)","from mail-ed1-x533.google.com (mail-ed1-x533.google.com\n\t[IPv6:2a00:1450:4864:20::533])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A6AFD6026D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  5 Aug 2021 17:21:51 +0200 (CEST)","by mail-ed1-x533.google.com with SMTP id cf5so8956510edb.2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 05 Aug 2021 08:21:51 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"G6+wqT9n\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=CYX/mWlkZbTW11PUScoaLP+HnxkW724cZfWI9x0ZtVQ=;\n\tb=G6+wqT9nAxR+igW7h6y/SetO8wNQqEIjXwDVfBAXTsxKS5nUBS+T60DmwzELWNjjTX\n\tFLXPUBKrWI2/9hQ71a91XtqEzA3JLKZ5MujKaFnZqLj7wNP+cJZRQRRwJD4LZt1PzI9l\n\tq5XobDzejr4GMcokH9qI5B2giArf4j7lt1j0A=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=CYX/mWlkZbTW11PUScoaLP+HnxkW724cZfWI9x0ZtVQ=;\n\tb=Ll4CtlZn37s8czDfItXWFlrPX/NsAjd2XS9mrx6VxyI+gDgVqWifYXMn6J9nVfgdnH\n\tmB/UojMgjz6D9v4qm/jGzm2YR2iTpYKP8yaPG1rlq4R7sg8OEB2NgXhoLOMhRcEOyKeu\n\tMZYZJtw61xd3i2oZMl2/Zt5ZrIEIKdNaAsHiYzQV3R4HVUlYIgAsrrH5KTpAGQf7rmgw\n\tvYv348x3F2YG19CVYyGmzd0kWYU13wc9Dii5yKM3cLMz8k5xdpln9T3I92Ieyk7r4/I6\n\tu4E0Ig52YHLy5fY8vuh34871s+DIlvGuMtnevxW5yMd2mPuxq9V1QfdXpGZk+Mj0Jnby\n\tef2Q==","X-Gm-Message-State":"AOAM531/cMxOwBXK/o9FUNv/620P2Fvv4LQP7V1MzWSm7UU2ySHP0xKt\n\tU1aIhhqrXbKCEPd6nEwdt9junSaouMEUJ9BNy7MxVw==","X-Google-Smtp-Source":"ABdhPJwjx+rCocKk7z6Iigmewt1FGJKUrZV00GjzFizkEP32iCEyxDOy6Pxyl/plzV48xH9Nzsy6gCGACGzguI0DmE8=","X-Received":"by 2002:aa7:c541:: with SMTP id s1mr7410559edr.327.1628176911227;\n\tThu, 05 Aug 2021 08:21:51 -0700 (PDT)","MIME-Version":"1.0","References":"<20210805132805.824754-1-hiroh@chromium.org>\n\t<YQv9X00T3T11ddhX@pendragon.ideasonboard.com>","In-Reply-To":"<YQv9X00T3T11ddhX@pendragon.ideasonboard.com>","From":"Hirokazu Honda <hiroh@chromium.org>","Date":"Fri, 6 Aug 2021 00:21:41 +0900","Message-ID":"<CAO5uPHPJkmg9TSOEsXBpDseL+6qQ-4yg7ZJwN4WSyV2rL8g+EA@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] android: yuv: Fix wrong access of\n\tsource buffer","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18579,"web_url":"https://patchwork.libcamera.org/comment/18579/","msgid":"<YQwKVkFPwjidMAap@pendragon.ideasonboard.com>","date":"2021-08-05T15:57:10","subject":"Re: [libcamera-devel] [PATCH] android: yuv: Fix wrong access of\n\tsource buffer","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Fri, Aug 06, 2021 at 12:21:41AM +0900, Hirokazu Honda wrote:\n> On Fri, Aug 6, 2021 at 12:02 AM Laurent Pinchart wrote:\n> > On Thu, Aug 05, 2021 at 10:28:05PM +0900, Hirokazu Honda wrote:\n> > > File descriptors of FrameBuffer given in PostProcessor::process()\n> > > for all the planes point the same buffer. To access the beginning\n> > > of the second or later plane, it is necessary to add offsets to\n> > > the beginning of the buffer.\n> > > Fix the wrong access to the second plane of NV12 in\n> > > PostProcessorYuv.\n> >\n> > This is an issue that comes from V4L2, where NV12, a multi-planar\n> > format, can be stored with the two planes contiguously in the same\n> > dmabuf, or in separate dmabufs. We have two V4L2 pixel formats for those\n> > (for instance, for NV12, that's V4L2_PIX_FMT_NV12 and\n> > V4L2_PIX_FMT_NV12M), and two APIs, the V4L2 single-planar API that only\n> > supports the single dmabuf format, and the V4L2 multi-planar API that\n> > supports either formats.\n> >\n> > This patch will work with V4L2_PIX_FMT_NV12 but not with\n> > V4L2_PIX_FMT_NV12M. Let's not force applications to deal with this mess,\n> > I don't want to expose it through the libcamera API. We should instead\n> > handle it in the MappedBuffer class, so that maps()[1] will return the\n> > correct value.\n> >\n> > In order to support both NV12 and NV12M, we'll have to add an offset\n> > field to FrameBuffer::Plane, but in the short term, as we only support\n> > the single-dmabuf formats in our pipeline handlers, I think we can\n> > hardcode the calculation below in the MappedBuffer class (I haven't\n> > tried it though, so there may be something I'm missing).\n> \n> Yeah, that is what I would like to discuss through this patch.\n> I agree with handling this MappedBuffer.\n> The current libcamera implementation doesn't handle multi-planar format.\n> I filed https://bugs.libcamera.org/show_bug.cgi?id=9 previously.\n> Shall I also add strides() to MappedBuffer or FrameBuffer?\n\nI don't think so. Strides are a property of the format. They're\ncurrently exposed in StreamConfiguration. We have a single stride there,\nnot a stride per plane, but I don't think it's urgent to rework this.\n\n> > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> > > ---\n> > >  src/android/yuv/post_processor_yuv.cpp | 7 +++++--\n> > >  1 file changed, 5 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp\n> > > index 772e805b..3b801e96 100644\n> > > --- a/src/android/yuv/post_processor_yuv.cpp\n> > > +++ b/src/android/yuv/post_processor_yuv.cpp\n> > > @@ -62,9 +62,12 @@ int PostProcessorYuv::process(const FrameBuffer &source,\n> > >               return -EINVAL;\n> > >       }\n> > >\n> > > -     int ret = libyuv::NV12Scale(sourceMapped.maps()[0].data(),\n> > > +     const uint8_t *sourceY = sourceMapped.maps()[0].data();\n> > > +     const uint8_t *sourceUV = sourceY + sourceStride_[0] * sourceSize_.height;\n> > > +\n> > > +     int ret = libyuv::NV12Scale(sourceY,\n> > >                                   sourceStride_[0],\n> > > -                                 sourceMapped.maps()[1].data(),\n> > > +                                 sourceUV,\n> > >                                   sourceStride_[1],\n> > >                                   sourceSize_.width, sourceSize_.height,\n> > >                                   destination->plane(0).data(),","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 9242EC3237\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  5 Aug 2021 15:57:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DAE2C687CF;\n\tThu,  5 Aug 2021 17:57:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E6E766026D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  5 Aug 2021 17:57:22 +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 60E1DE04;\n\tThu,  5 Aug 2021 17:57:22 +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=\"puKMoT6P\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1628179042;\n\tbh=mAQGMLtwLLQxl0NL9teBfl5jGA6L3wINnlh20tvqA0Y=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=puKMoT6PdbkKHEkKgWovr9WYexn7K31Ze7X5SEyw9tViyF+zzZMQGmtIEkEsmx7Vg\n\t7SDXHtFRvg4t0ok0ruhpJL1eKX0KWYLEbnheaoJ8jDAmIB0jLm2usBhl7G11lPPtoJ\n\tl7LbJbo/zVx+rAMK5aKzvrRi6hXYTeC3iaDsL3qA=","Date":"Thu, 5 Aug 2021 18:57:10 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Hirokazu Honda <hiroh@chromium.org>","Message-ID":"<YQwKVkFPwjidMAap@pendragon.ideasonboard.com>","References":"<20210805132805.824754-1-hiroh@chromium.org>\n\t<YQv9X00T3T11ddhX@pendragon.ideasonboard.com>\n\t<CAO5uPHPJkmg9TSOEsXBpDseL+6qQ-4yg7ZJwN4WSyV2rL8g+EA@mail.gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<CAO5uPHPJkmg9TSOEsXBpDseL+6qQ-4yg7ZJwN4WSyV2rL8g+EA@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH] android: yuv: Fix wrong access of\n\tsource buffer","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":18580,"web_url":"https://patchwork.libcamera.org/comment/18580/","msgid":"<CAO5uPHNNDH4RqaS7OByoSQDOm+cPuk7THHgR9i4eRRQZoNwY1w@mail.gmail.com>","date":"2021-08-05T16:03:06","subject":"Re: [libcamera-devel] [PATCH] android: yuv: Fix wrong access of\n\tsource buffer","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Fri, Aug 6, 2021 at 12:57 AM Laurent Pinchart\n<laurent.pinchart@ideasonboard.com> wrote:\n>\n> Hi Hiro,\n>\n> On Fri, Aug 06, 2021 at 12:21:41AM +0900, Hirokazu Honda wrote:\n> > On Fri, Aug 6, 2021 at 12:02 AM Laurent Pinchart wrote:\n> > > On Thu, Aug 05, 2021 at 10:28:05PM +0900, Hirokazu Honda wrote:\n> > > > File descriptors of FrameBuffer given in PostProcessor::process()\n> > > > for all the planes point the same buffer. To access the beginning\n> > > > of the second or later plane, it is necessary to add offsets to\n> > > > the beginning of the buffer.\n> > > > Fix the wrong access to the second plane of NV12 in\n> > > > PostProcessorYuv.\n> > >\n> > > This is an issue that comes from V4L2, where NV12, a multi-planar\n> > > format, can be stored with the two planes contiguously in the same\n> > > dmabuf, or in separate dmabufs. We have two V4L2 pixel formats for those\n> > > (for instance, for NV12, that's V4L2_PIX_FMT_NV12 and\n> > > V4L2_PIX_FMT_NV12M), and two APIs, the V4L2 single-planar API that only\n> > > supports the single dmabuf format, and the V4L2 multi-planar API that\n> > > supports either formats.\n> > >\n> > > This patch will work with V4L2_PIX_FMT_NV12 but not with\n> > > V4L2_PIX_FMT_NV12M. Let's not force applications to deal with this mess,\n> > > I don't want to expose it through the libcamera API. We should instead\n> > > handle it in the MappedBuffer class, so that maps()[1] will return the\n> > > correct value.\n> > >\n> > > In order to support both NV12 and NV12M, we'll have to add an offset\n> > > field to FrameBuffer::Plane, but in the short term, as we only support\n> > > the single-dmabuf formats in our pipeline handlers, I think we can\n> > > hardcode the calculation below in the MappedBuffer class (I haven't\n> > > tried it though, so there may be something I'm missing).\n> >\n> > Yeah, that is what I would like to discuss through this patch.\n> > I agree with handling this MappedBuffer.\n> > The current libcamera implementation doesn't handle multi-planar format.\n> > I filed https://bugs.libcamera.org/show_bug.cgi?id=9 previously.\n> > Shall I also add strides() to MappedBuffer or FrameBuffer?\n>\n> I don't think so. Strides are a property of the format. They're\n> currently exposed in StreamConfiguration. We have a single stride there,\n> not a stride per plane, but I don't think it's urgent to rework this.\n>\n\nI got it. I will abandon the patch in favor of  the suggested\nMappedBuffer class change.\n\n-Hiro\n> > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> > > > ---\n> > > >  src/android/yuv/post_processor_yuv.cpp | 7 +++++--\n> > > >  1 file changed, 5 insertions(+), 2 deletions(-)\n> > > >\n> > > > diff --git a/src/android/yuv/post_processor_yuv.cpp b/src/android/yuv/post_processor_yuv.cpp\n> > > > index 772e805b..3b801e96 100644\n> > > > --- a/src/android/yuv/post_processor_yuv.cpp\n> > > > +++ b/src/android/yuv/post_processor_yuv.cpp\n> > > > @@ -62,9 +62,12 @@ int PostProcessorYuv::process(const FrameBuffer &source,\n> > > >               return -EINVAL;\n> > > >       }\n> > > >\n> > > > -     int ret = libyuv::NV12Scale(sourceMapped.maps()[0].data(),\n> > > > +     const uint8_t *sourceY = sourceMapped.maps()[0].data();\n> > > > +     const uint8_t *sourceUV = sourceY + sourceStride_[0] * sourceSize_.height;\n> > > > +\n> > > > +     int ret = libyuv::NV12Scale(sourceY,\n> > > >                                   sourceStride_[0],\n> > > > -                                 sourceMapped.maps()[1].data(),\n> > > > +                                 sourceUV,\n> > > >                                   sourceStride_[1],\n> > > >                                   sourceSize_.width, sourceSize_.height,\n> > > >                                   destination->plane(0).data(),\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 393C2C3235\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  5 Aug 2021 16:03:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AABF9687CF;\n\tThu,  5 Aug 2021 18:03:18 +0200 (CEST)","from mail-ej1-x62c.google.com (mail-ej1-x62c.google.com\n\t[IPv6:2a00:1450:4864:20::62c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7371A6026D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  5 Aug 2021 18:03:16 +0200 (CEST)","by mail-ej1-x62c.google.com with SMTP id zb12so5629681ejb.5\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 05 Aug 2021 09:03:16 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"OpRe3SZT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=pnwN1T53FTUydTEosTV7ACtAQ/SQhZ9b560cKTo7OEI=;\n\tb=OpRe3SZTvR+AV7yL1CP85+tYtK+rS7B7IvypKKaOmn8pRNxtqP/8AECNKKczoNwrx/\n\tknx/ScHskyUCN0NrTGm8QK0s+ARD8I1ijxNYO5Em64tmIvCMidj8u8eM3Z16cZJWaV/1\n\t/EGeF7m1XYFiNpIO0deVTLN9vUrLMv9MgTNkA=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=pnwN1T53FTUydTEosTV7ACtAQ/SQhZ9b560cKTo7OEI=;\n\tb=G5ZBlgc4fCbyKjTJfg9ciLJlH0AbeV0GtPKYjyWMtHscoPYV+6hSOr9UIWT2AETRzP\n\t5ftrqefgVhJql2LjtMXEV9YbR1wBkvdEq2gAawNj0xVjgZa/W5e15YucQkcOn0nXaCGR\n\tdLHXuybivv6HxCVw8H9LRHUMjE0V70G+px9xW04X9a38v5nACoeWuWxG5k9r7qFccWrw\n\tNBZ2lkX8MzekY0sCTMxnQ45yOi0NnEY0hYA1h3WiHa1o1PBbC+XGdScFW0WvpcKdurqG\n\tT2i/1c/RPLzzu4TZHfK3p9Ckc/oUW1uA9JxJGVhnX0jayr4H957bx0XT4+1ZEHCqX175\n\t5OdA==","X-Gm-Message-State":"AOAM531KN+WvetuG69Yx+zu5p654awI3Idd6U/5TF5JhnZD0EC7fW+u0\n\tihLLmlTeQ9M/W10L+4WnCvTU1o0r6HlhGi9eBxx5hQ==","X-Google-Smtp-Source":"ABdhPJyt56AvqSwtfVWsDyXm+QVmzppgas3GKWmIQfd1Up5dZVzjyay9q3A20uwW2EqHCOdhay/v+gI/tC6N+AFbKYk=","X-Received":"by 2002:a17:907:76d3:: with SMTP id\n\tkf19mr5524614ejc.221.1628179396032; \n\tThu, 05 Aug 2021 09:03:16 -0700 (PDT)","MIME-Version":"1.0","References":"<20210805132805.824754-1-hiroh@chromium.org>\n\t<YQv9X00T3T11ddhX@pendragon.ideasonboard.com>\n\t<CAO5uPHPJkmg9TSOEsXBpDseL+6qQ-4yg7ZJwN4WSyV2rL8g+EA@mail.gmail.com>\n\t<YQwKVkFPwjidMAap@pendragon.ideasonboard.com>","In-Reply-To":"<YQwKVkFPwjidMAap@pendragon.ideasonboard.com>","From":"Hirokazu Honda <hiroh@chromium.org>","Date":"Fri, 6 Aug 2021 01:03:06 +0900","Message-ID":"<CAO5uPHNNDH4RqaS7OByoSQDOm+cPuk7THHgR9i4eRRQZoNwY1w@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH] android: yuv: Fix wrong access of\n\tsource buffer","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]