[{"id":3155,"web_url":"https://patchwork.libcamera.org/comment/3155/","msgid":"<20191127150654.7rohrn5p2kthlsly@uno.localdomain>","date":"2019-11-27T15:06:54","subject":"Re: [libcamera-devel] [PATCH 15/30] libcamera: buffer: Move capture\n\tinformation to BufferInfo","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Wed, Nov 27, 2019 at 12:36:05AM +0100, Niklas Söderlund wrote:\n> Move the metadata information retrieved when dequeuing a V4L2 buffer\n> into a BufferInfo. This is done as a step to migrate to the FrameBuffer\n> interface as the functions added to Buffer around BufferInfo matches the\n> one in FrameBuffer.\n>\n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> ---\n>  include/libcamera/buffer.h               |  2 ++\n>  src/android/camera_device.cpp            |  4 +--\n>  src/cam/buffer_writer.cpp                |  2 +-\n>  src/cam/capture.cpp                      |  8 ++++--\n>  src/libcamera/buffer.cpp                 | 34 +++++++++++++++++-------\n>  src/libcamera/pipeline/ipu3/ipu3.cpp     |  4 +--\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  8 +++---\n>  src/libcamera/request.cpp                |  2 +-\n>  src/libcamera/v4l2_videodevice.cpp       | 23 +++++++++-------\n>  src/qcam/main_window.cpp                 | 13 ++++-----\n>  test/camera/buffer_import.cpp            |  2 +-\n>  test/camera/capture.cpp                  |  2 +-\n>  test/v4l2_videodevice/buffer_sharing.cpp | 12 ++++++---\n>  13 files changed, 72 insertions(+), 44 deletions(-)\n>\n> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> index d6db6138ca11d5fe..2e5376fb8b53a4c5 100644\n> --- a/include/libcamera/buffer.h\n> +++ b/include/libcamera/buffer.h\n> @@ -122,6 +122,7 @@ public:\n>  \tunsigned int bytesused() const { return bytesused_; }\n>  \tuint64_t timestamp() const { return timestamp_; }\n>  \tunsigned int sequence() const { return sequence_; }\n> +\tconst BufferInfo &info() const { return info_; };\n>\n>  \tStatus status() const { return status_; }\n>  \tRequest *request() const { return request_; }\n> @@ -142,6 +143,7 @@ private:\n>  \tunsigned int bytesused_;\n>  \tuint64_t timestamp_;\n>  \tunsigned int sequence_;\n> +\tBufferInfo info_;\n>\n>  \tStatus status_;\n>  \tRequest *request_;\n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 09588c16a6301649..55b29a9a41ab8943 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -803,11 +803,11 @@ void CameraDevice::requestComplete(Request *request)\n>\n>  \tif (status == CAMERA3_BUFFER_STATUS_OK) {\n>  \t\tnotifyShutter(descriptor->frameNumber,\n> -\t\t\t      libcameraBuffer->timestamp());\n> +\t\t\t      libcameraBuffer->info().timestamp());\n>\n>  \t\tcaptureResult.partial_result = 1;\n>  \t\tresultMetadata = getResultMetadata(descriptor->frameNumber,\n> -\t\t\t\t\t\t   libcameraBuffer->timestamp());\n> +\t\t\t\t\t\t   libcameraBuffer->info().timestamp());\n>  \t\tcaptureResult.result = resultMetadata->get();\n>  \t}\n>\n> diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp\n> index 5967efca07254666..b6b40baeee661df6 100644\n> --- a/src/cam/buffer_writer.cpp\n> +++ b/src/cam/buffer_writer.cpp\n> @@ -32,7 +32,7 @@ int BufferWriter::write(Buffer *buffer, const std::string &streamName)\n>  \tif (pos != std::string::npos) {\n>  \t\tstd::stringstream ss;\n>  \t\tss << streamName << \"-\" << std::setw(6)\n> -\t\t   << std::setfill('0') << buffer->sequence();\n> +\t\t   << std::setfill('0') << buffer->info().sequence();\n>  \t\tfilename.replace(pos, 1, ss.str());\n>  \t}\n>\n> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> index 1a4dbe7ce4a15a2d..a4fa88a8d99669bc 100644\n> --- a/src/cam/capture.cpp\n> +++ b/src/cam/capture.cpp\n> @@ -155,8 +155,12 @@ void Capture::requestComplete(Request *request)\n>  \t\tconst std::string &name = streamName_[stream];\n>\n>  \t\tinfo << \" \" << name\n> -\t\t     << \" seq: \" << std::setw(6) << std::setfill('0') << buffer->sequence()\n> -\t\t     << \" bytesused: \" << buffer->bytesused();\n> +\t\t     << \" seq: \" << std::setw(6) << std::setfill('0') << buffer->info().sequence();\n> +\n> +\t\tunsigned int nplane = 0;\n> +\t\tfor (const BufferInfo::Plane &plane : buffer->info().planes())\n> +\t\t\tinfo << \" bytesused(\" << nplane++ << \"): \"\n> +\t\t\t     << plane.bytesused;\n>\n>  \t\tif (writer_)\n>  \t\t\twriter_->write(buffer, name);\n> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> index 7043345c3f3207cd..d5a4815a0bb8c528 100644\n> --- a/src/libcamera/buffer.cpp\n> +++ b/src/libcamera/buffer.cpp\n> @@ -375,15 +375,22 @@ Buffer::Buffer(unsigned int index, const Buffer *metadata)\n>  \t  status_(Buffer::BufferSuccess), request_(nullptr),\n>  \t  stream_(nullptr)\n>  {\n> +\tunsigned int sequence;\n> +\tuint64_t timestamp;\n> +\tunsigned int bytesused;\n> +\n>  \tif (metadata) {\n> -\t\tbytesused_ = metadata->bytesused_;\n> -\t\tsequence_ = metadata->sequence_;\n> -\t\ttimestamp_ = metadata->timestamp_;\n> +\t\tbytesused = metadata->info().planes()[0].bytesused;\n> +\t\tsequence = metadata->info().sequence();\n> +\t\ttimestamp = metadata->info().timestamp();\n>  \t} else {\n> -\t\tbytesused_ = 0;\n> -\t\tsequence_ = 0;\n> -\t\ttimestamp_ = 0;\n> +\t\tbytesused = 0;\n> +\t\tsequence = 0;\n> +\t\ttimestamp = 0;\n>  \t}\n> +\n> +\tinfo_.update(BufferInfo::BufferSuccess, sequence, timestamp,\n> +\t\t     { { bytesused } });\n\nI am missing a bit of context here, but is it fair to assume we have\na single plane ?\n>  }\n>\n>  /**\n> @@ -439,6 +446,16 @@ Buffer::Buffer(unsigned int index, const Buffer *metadata)\n>   * \\return Sequence number of the buffer\n>   */\n>\n> +/**\n> + * \\fn Buffer::info()\n> + * \\brief Retrieve the buffer metadata information\n> + *\n\nmetadata + information is a bit of repetition, but that's fine\n\n> + * The buffer metadata information is update every time the buffer contained\n\nas in the previous patch, did you mean \"buffer content\" ?\n\n> + * are changed, for example when it is dequeued from hardware.\n\nbuffer -> is\n\n> + *\n> + * \\return Metadata of the buffer\n> + */\n> +\n>  /**\n>   * \\fn Buffer::status()\n>   * \\brief Retrieve the buffer status\n> @@ -482,10 +499,7 @@ Buffer::Buffer(unsigned int index, const Buffer *metadata)\n>   */\n>  void Buffer::cancel()\n>  {\n> -\tbytesused_ = 0;\n> -\ttimestamp_ = 0;\n> -\tsequence_ = 0;\n> -\tstatus_ = BufferCancelled;\n> +\tinfo_.update(BufferInfo::BufferCancelled, 0, 0, { {} });\n>  }\n>\n>  /**\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index ad223d9101bdc6ed..8ba08351c950f5e2 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -928,7 +928,7 @@ int PipelineHandlerIPU3::registerCameras()\n>  void IPU3CameraData::imguInputBufferReady(Buffer *buffer)\n>  {\n>  \t/* \\todo Handle buffer failures when state is set to BufferError. */\n> -\tif (buffer->status() == Buffer::BufferCancelled)\n> +\tif (buffer->info().status() == BufferInfo::BufferCancelled)\n>  \t\treturn;\n>\n>  \tcio2_.output_->queueBuffer(buffer);\n> @@ -962,7 +962,7 @@ void IPU3CameraData::imguOutputBufferReady(Buffer *buffer)\n>  void IPU3CameraData::cio2BufferReady(Buffer *buffer)\n>  {\n>  \t/* \\todo Handle buffer failures when state is set to BufferError. */\n> -\tif (buffer->status() == Buffer::BufferCancelled)\n> +\tif (buffer->info().status() == BufferInfo::BufferCancelled)\n>  \t\treturn;\n>\n>  \timgu_->input_->queueBuffer(buffer);\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index e8b6a278e97b0ba0..6ad9b57d8353896c 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -100,10 +100,10 @@ public:\n>  \t\tASSERT(frameOffset(SOE) == 0);\n>\n>  \t\tutils::time_point soe = std::chrono::time_point<utils::clock>()\n> -\t\t\t+ std::chrono::nanoseconds(buffer->timestamp())\n> +\t\t\t+ std::chrono::nanoseconds(buffer->info().timestamp())\n>  \t\t\t+ timeOffset(SOE);\n>\n> -\t\tnotifyStartOfExposure(buffer->sequence(), soe);\n> +\t\tnotifyStartOfExposure(buffer->info().sequence(), soe);\n>  \t}\n>\n>  \tvoid setDelay(unsigned int type, int frame, int msdelay)\n> @@ -1006,8 +1006,8 @@ void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)\n>\n>  \tdata->timeline_.bufferReady(buffer);\n>\n> -\tif (data->frame_ <= buffer->sequence())\n> -\t\tdata->frame_ = buffer->sequence() + 1;\n> +\tif (data->frame_ <= buffer->info().sequence())\n> +\t\tdata->frame_ = buffer->info().sequence() + 1;\n>\n>  \tcompleteBuffer(activeCamera_, request, buffer);\n>  \ttryCompleteRequest(request);\n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index 3b49e52510918eee..7593bf9dfa546401 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -260,7 +260,7 @@ bool Request::completeBuffer(Buffer *buffer)\n>\n>  \tbuffer->request_ = nullptr;\n>\n> -\tif (buffer->status() == Buffer::BufferCancelled)\n> +\tif (buffer->info().status() == BufferInfo::BufferCancelled)\n>  \t\tcancelled_ = true;\n>\n>  \treturn !hasPendingBuffers();\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index cc0a1c9382a2b1ed..8f962c7e9d0c7d01 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1012,10 +1012,12 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)\n>  \t}\n>\n>  \tif (V4L2_TYPE_IS_OUTPUT(buf.type)) {\n> -\t\tbuf.bytesused = buffer->bytesused_;\n> -\t\tbuf.sequence = buffer->sequence_;\n> -\t\tbuf.timestamp.tv_sec = buffer->timestamp_ / 1000000000;\n> -\t\tbuf.timestamp.tv_usec = (buffer->timestamp_ / 1000) % 1000000;\n> +\t\tconst BufferInfo &info = buffer->info();\n> +\n> +\t\tbuf.bytesused = info.planes()[0].bytesused;\n> +\t\tbuf.sequence = info.sequence();\n> +\t\tbuf.timestamp.tv_sec = info.timestamp() / 1000000000;\n> +\t\tbuf.timestamp.tv_usec = (info.timestamp() / 1000) % 1000000;\n>  \t}\n>\n>  \tLOG(V4L2, Debug) << \"Queueing buffer \" << buf.index;\n> @@ -1121,12 +1123,13 @@ Buffer *V4L2VideoDevice::dequeueBuffer()\n>  \t\tfdEvent_->setEnabled(false);\n>\n>  \tbuffer->index_ = buf.index;\n> -\tbuffer->bytesused_ = buf.bytesused;\n> -\tbuffer->timestamp_ = buf.timestamp.tv_sec * 1000000000ULL\n> -\t\t\t   + buf.timestamp.tv_usec * 1000ULL;\n> -\tbuffer->sequence_ = buf.sequence;\n> -\tbuffer->status_ = buf.flags & V4L2_BUF_FLAG_ERROR\n> -\t\t\t? Buffer::BufferError : Buffer::BufferSuccess;\n> +\n> +\tBufferInfo::Status status = buf.flags & V4L2_BUF_FLAG_ERROR\n> +\t\t? BufferInfo::BufferError : BufferInfo::BufferSuccess;\n> +\tuint64_t timestamp = buf.timestamp.tv_sec * 1000000000ULL\n> +\t\t+ buf.timestamp.tv_usec * 1000ULL;\n> +\n> +\tbuffer->info_.update(status, buf.sequence, timestamp, { { buf.bytesused } });\n\nHere it's where I wondered if we should not provide an update()\nversion which takes a struct v4l2_buffer, but it would maybe be a bit\nof a layering violation.\n\n>\n>  \treturn buffer;\n>  }\n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index 4cecf7e351214f3d..771020112f09b1ef 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -258,14 +258,15 @@ void MainWindow::requestComplete(Request *request)\n>  \tframesCaptured_++;\n>\n>  \tBuffer *buffer = buffers.begin()->second;\n> +\tconst BufferInfo &info = buffer->info();\n>\n> -\tdouble fps = buffer->timestamp() - lastBufferTime_;\n> +\tdouble fps = info.timestamp() - lastBufferTime_;\n>  \tfps = lastBufferTime_ && fps ? 1000000000.0 / fps : 0.0;\n> -\tlastBufferTime_ = buffer->timestamp();\n> +\tlastBufferTime_ = info.timestamp();\n>\n> -\tstd::cout << \"seq: \" << std::setw(6) << std::setfill('0') << buffer->sequence()\n> -\t\t  << \" bytesused: \" << buffer->bytesused()\n> -\t\t  << \" timestamp: \" << buffer->timestamp()\n> +\tstd::cout << \"seq: \" << std::setw(6) << std::setfill('0') << info.sequence()\n> +\t\t  << \" bytesused: \" << info.planes()[0].bytesused\n\nAssuming a single plane is fine ?\n\n> +\t\t  << \" timestamp: \" << info.timestamp()\n>  \t\t  << \" fps: \" << std::fixed << std::setprecision(2) << fps\n>  \t\t  << std::endl;\n>\n> @@ -302,7 +303,7 @@ int MainWindow::display(Buffer *buffer)\n>\n>  \tDmabuf &dmabuf = mem->planes().front();\n>  \tunsigned char *raw = static_cast<unsigned char *>(dmabuf.mem());\n> -\tviewfinder_->display(raw, buffer->bytesused());\n> +\tviewfinder_->display(raw, buffer->info().planes()[0].bytesused);\n>\n>  \treturn 0;\n>  }\n> diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp\n> index dae907f9f41841c9..5dbaed9255d3d60c 100644\n> --- a/test/camera/buffer_import.cpp\n> +++ b/test/camera/buffer_import.cpp\n> @@ -275,7 +275,7 @@ public:\n>  protected:\n>  \tvoid bufferComplete(Request *request, Buffer *buffer)\n>  \t{\n> -\t\tif (buffer->status() != Buffer::BufferSuccess)\n> +\t\tif (buffer->info().status() != BufferInfo::BufferSuccess)\n>  \t\t\treturn;\n>\n>  \t\tunsigned int index = buffer->index();\n> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp\n> index ca1ebe419946dd4d..8307ea2629801679 100644\n> --- a/test/camera/capture.cpp\n> +++ b/test/camera/capture.cpp\n> @@ -28,7 +28,7 @@ protected:\n>\n>  \tvoid bufferComplete(Request *request, Buffer *buffer)\n>  \t{\n> -\t\tif (buffer->status() != Buffer::BufferSuccess)\n> +\t\tif (buffer->info().status() != BufferInfo::BufferSuccess)\n>  \t\t\treturn;\n>\n>  \t\tcompleteBuffersCount_++;\n> diff --git a/test/v4l2_videodevice/buffer_sharing.cpp b/test/v4l2_videodevice/buffer_sharing.cpp\n> index d02c391b95933922..6b71caef111693d6 100644\n> --- a/test/v4l2_videodevice/buffer_sharing.cpp\n> +++ b/test/v4l2_videodevice/buffer_sharing.cpp\n> @@ -92,10 +92,12 @@ protected:\n>\n>  \tvoid captureBufferReady(Buffer *buffer)\n>  \t{\n> +\t\tconst BufferInfo &info = buffer->info();\n> +\n>  \t\tstd::cout << \"Received capture buffer  sequence \"\n> -\t\t\t  << buffer->sequence() << std::endl;\n> +\t\t\t  << info.sequence() << std::endl;\n>\n> -\t\tif (buffer->status() != Buffer::BufferSuccess)\n> +\t\tif (info.status() != BufferInfo::BufferSuccess)\n>  \t\t\treturn;\n>\n>  \t\toutput_->queueBuffer(buffer);\n> @@ -104,10 +106,12 @@ protected:\n>\n>  \tvoid outputBufferReady(Buffer *buffer)\n>  \t{\n> +\t\tconst BufferInfo &info = buffer->info();\n> +\n>  \t\tstd::cout << \"Received output buffer sequence \"\n> -\t\t\t  << buffer->sequence() << std::endl;\n> +\t\t\t  << info.sequence() << std::endl;\n>\n> -\t\tif (buffer->status() != Buffer::BufferSuccess)\n> +\t\tif (info.status() != BufferInfo::BufferSuccess)\n>  \t\t\treturn;\n>\n>  \t\tcapture_->queueBuffer(buffer);\n> --\n> 2.24.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5165B60C33\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 27 Nov 2019 16:04:48 +0100 (CET)","from uno.localdomain (93-34-114-233.ip49.fastwebnet.it\n\t[93.34.114.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id BF6F4FF80B;\n\tWed, 27 Nov 2019 15:04:47 +0000 (UTC)"],"X-Originating-IP":"93.34.114.233","Date":"Wed, 27 Nov 2019 16:06:54 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20191127150654.7rohrn5p2kthlsly@uno.localdomain>","References":"<20191126233620.1695316-1-niklas.soderlund@ragnatech.se>\n\t<20191126233620.1695316-16-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"e2dnpo63uo3dbjnt\"","Content-Disposition":"inline","In-Reply-To":"<20191126233620.1695316-16-niklas.soderlund@ragnatech.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 15/30] libcamera: buffer: Move capture\n\tinformation to BufferInfo","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>","X-List-Received-Date":"Wed, 27 Nov 2019 15:04:48 -0000"}},{"id":3236,"web_url":"https://patchwork.libcamera.org/comment/3236/","msgid":"<20191209211933.GF18060@pendragon.ideasonboard.com>","date":"2019-12-09T21:19:33","subject":"Re: [libcamera-devel] [PATCH 15/30] libcamera: buffer: Move capture\n\tinformation to BufferInfo","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Wed, Nov 27, 2019 at 04:06:54PM +0100, Jacopo Mondi wrote:\n> On Wed, Nov 27, 2019 at 12:36:05AM +0100, Niklas Söderlund wrote:\n> > Move the metadata information retrieved when dequeuing a V4L2 buffer\n\n\"metadata\" or \"information\", not \"metadata information\". I would pick\none or the other based on whether you want to rename BufferInfo to\nFrameInfo or FrameMetadata (with a preference for the latter on my\nside).\n\n> > into a BufferInfo. This is done as a step to migrate to the FrameBuffer\n> > interface as the functions added to Buffer around BufferInfo matches the\n\ns/matches/match/\n\n> > one in FrameBuffer.\n\ns/one/ones/\n\n> >\n> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n> > ---\n> >  include/libcamera/buffer.h               |  2 ++\n> >  src/android/camera_device.cpp            |  4 +--\n> >  src/cam/buffer_writer.cpp                |  2 +-\n> >  src/cam/capture.cpp                      |  8 ++++--\n> >  src/libcamera/buffer.cpp                 | 34 +++++++++++++++++-------\n> >  src/libcamera/pipeline/ipu3/ipu3.cpp     |  4 +--\n> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp |  8 +++---\n> >  src/libcamera/request.cpp                |  2 +-\n> >  src/libcamera/v4l2_videodevice.cpp       | 23 +++++++++-------\n> >  src/qcam/main_window.cpp                 | 13 ++++-----\n> >  test/camera/buffer_import.cpp            |  2 +-\n> >  test/camera/capture.cpp                  |  2 +-\n> >  test/v4l2_videodevice/buffer_sharing.cpp | 12 ++++++---\n> >  13 files changed, 72 insertions(+), 44 deletions(-)\n> >\n> > diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> > index d6db6138ca11d5fe..2e5376fb8b53a4c5 100644\n> > --- a/include/libcamera/buffer.h\n> > +++ b/include/libcamera/buffer.h\n> > @@ -122,6 +122,7 @@ public:\n> >  \tunsigned int bytesused() const { return bytesused_; }\n> >  \tuint64_t timestamp() const { return timestamp_; }\n> >  \tunsigned int sequence() const { return sequence_; }\n> > +\tconst BufferInfo &info() const { return info_; };\n> >\n> >  \tStatus status() const { return status_; }\n> >  \tRequest *request() const { return request_; }\n> > @@ -142,6 +143,7 @@ private:\n> >  \tunsigned int bytesused_;\n> >  \tuint64_t timestamp_;\n> >  \tunsigned int sequence_;\n> > +\tBufferInfo info_;\n> >\n> >  \tStatus status_;\n> >  \tRequest *request_;\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 09588c16a6301649..55b29a9a41ab8943 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -803,11 +803,11 @@ void CameraDevice::requestComplete(Request *request)\n> >\n> >  \tif (status == CAMERA3_BUFFER_STATUS_OK) {\n> >  \t\tnotifyShutter(descriptor->frameNumber,\n> > -\t\t\t      libcameraBuffer->timestamp());\n> > +\t\t\t      libcameraBuffer->info().timestamp());\n> >\n> >  \t\tcaptureResult.partial_result = 1;\n> >  \t\tresultMetadata = getResultMetadata(descriptor->frameNumber,\n> > -\t\t\t\t\t\t   libcameraBuffer->timestamp());\n> > +\t\t\t\t\t\t   libcameraBuffer->info().timestamp());\n> >  \t\tcaptureResult.result = resultMetadata->get();\n> >  \t}\n> >\n> > diff --git a/src/cam/buffer_writer.cpp b/src/cam/buffer_writer.cpp\n> > index 5967efca07254666..b6b40baeee661df6 100644\n> > --- a/src/cam/buffer_writer.cpp\n> > +++ b/src/cam/buffer_writer.cpp\n> > @@ -32,7 +32,7 @@ int BufferWriter::write(Buffer *buffer, const std::string &streamName)\n> >  \tif (pos != std::string::npos) {\n> >  \t\tstd::stringstream ss;\n> >  \t\tss << streamName << \"-\" << std::setw(6)\n> > -\t\t   << std::setfill('0') << buffer->sequence();\n> > +\t\t   << std::setfill('0') << buffer->info().sequence();\n> >  \t\tfilename.replace(pos, 1, ss.str());\n> >  \t}\n> >\n> > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> > index 1a4dbe7ce4a15a2d..a4fa88a8d99669bc 100644\n> > --- a/src/cam/capture.cpp\n> > +++ b/src/cam/capture.cpp\n> > @@ -155,8 +155,12 @@ void Capture::requestComplete(Request *request)\n> >  \t\tconst std::string &name = streamName_[stream];\n> >\n> >  \t\tinfo << \" \" << name\n> > -\t\t     << \" seq: \" << std::setw(6) << std::setfill('0') << buffer->sequence()\n> > -\t\t     << \" bytesused: \" << buffer->bytesused();\n> > +\t\t     << \" seq: \" << std::setw(6) << std::setfill('0') << buffer->info().sequence();\n> > +\n> > +\t\tunsigned int nplane = 0;\n> > +\t\tfor (const BufferInfo::Plane &plane : buffer->info().planes())\n> > +\t\t\tinfo << \" bytesused(\" << nplane++ << \"): \"\n> > +\t\t\t     << plane.bytesused;\n\nNitpicking, this makes it quite long. How about\n\n\t\tconst BufferInfo &info = buffer->info();\n\n\t\tinfo << \" \" << name\n\t\t     << \" seq: \" << std::setw(6) << std::setfill('0') << info..sequence()\n\t\t     << \" bytesused: \";\n\n\t\tunsigned int nplane = 0;\n\t\tfor (const BufferInfo::Plane &plane : info.planes()) {\n\t\t\tinfo << plane.bytesused;\n\t\t\tif (++nplane < info.planes().size())\n\t\t\t\tinfo << \"/\";\n\t\t}\n\n> >\n> >  \t\tif (writer_)\n> >  \t\t\twriter_->write(buffer, name);\n> > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> > index 7043345c3f3207cd..d5a4815a0bb8c528 100644\n> > --- a/src/libcamera/buffer.cpp\n> > +++ b/src/libcamera/buffer.cpp\n> > @@ -375,15 +375,22 @@ Buffer::Buffer(unsigned int index, const Buffer *metadata)\n> >  \t  status_(Buffer::BufferSuccess), request_(nullptr),\n> >  \t  stream_(nullptr)\n> >  {\n> > +\tunsigned int sequence;\n> > +\tuint64_t timestamp;\n> > +\tunsigned int bytesused;\n> > +\n> >  \tif (metadata) {\n> > -\t\tbytesused_ = metadata->bytesused_;\n> > -\t\tsequence_ = metadata->sequence_;\n> > -\t\ttimestamp_ = metadata->timestamp_;\n> > +\t\tbytesused = metadata->info().planes()[0].bytesused;\n> > +\t\tsequence = metadata->info().sequence();\n> > +\t\ttimestamp = metadata->info().timestamp();\n> >  \t} else {\n> > -\t\tbytesused_ = 0;\n> > -\t\tsequence_ = 0;\n> > -\t\ttimestamp_ = 0;\n> > +\t\tbytesused = 0;\n> > +\t\tsequence = 0;\n> > +\t\ttimestamp = 0;\n> >  \t}\n> > +\n> > +\tinfo_.update(BufferInfo::BufferSuccess, sequence, timestamp,\n> > +\t\t     { { bytesused } });\n\nThis would be simplified is BufferInfo was turned into a structure with\npublic members.\n\n> \n> I am missing a bit of context here, but is it fair to assume we have\n> a single plane ?\n> >  }\n> >\n> >  /**\n> > @@ -439,6 +446,16 @@ Buffer::Buffer(unsigned int index, const Buffer *metadata)\n> >   * \\return Sequence number of the buffer\n> >   */\n> >\n> > +/**\n> > + * \\fn Buffer::info()\n> > + * \\brief Retrieve the buffer metadata information\n> > + *\n> \n> metadata + information is a bit of repetition, but that's fine\n\nAs commented above, I think we should use one or the other. If we go for\nmetadata, I would name the method metadata(). It probably doesn't matter\nmuch here though, as the Buffer class is removed later in the series.\n\n> > + * The buffer metadata information is update every time the buffer contained\n> \n> as in the previous patch, did you mean \"buffer content\" ?\n> \n> > + * are changed, for example when it is dequeued from hardware.\n> \n> buffer -> is\n> \n> > + *\n> > + * \\return Metadata of the buffer\n> > + */\n> > +\n> >  /**\n> >   * \\fn Buffer::status()\n> >   * \\brief Retrieve the buffer status\n> > @@ -482,10 +499,7 @@ Buffer::Buffer(unsigned int index, const Buffer *metadata)\n> >   */\n> >  void Buffer::cancel()\n> >  {\n> > -\tbytesused_ = 0;\n> > -\ttimestamp_ = 0;\n> > -\tsequence_ = 0;\n> > -\tstatus_ = BufferCancelled;\n> > +\tinfo_.update(BufferInfo::BufferCancelled, 0, 0, { {} });\n> >  }\n> >\n> >  /**\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index ad223d9101bdc6ed..8ba08351c950f5e2 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -928,7 +928,7 @@ int PipelineHandlerIPU3::registerCameras()\n> >  void IPU3CameraData::imguInputBufferReady(Buffer *buffer)\n> >  {\n> >  \t/* \\todo Handle buffer failures when state is set to BufferError. */\n> > -\tif (buffer->status() == Buffer::BufferCancelled)\n> > +\tif (buffer->info().status() == BufferInfo::BufferCancelled)\n> >  \t\treturn;\n> >\n> >  \tcio2_.output_->queueBuffer(buffer);\n> > @@ -962,7 +962,7 @@ void IPU3CameraData::imguOutputBufferReady(Buffer *buffer)\n> >  void IPU3CameraData::cio2BufferReady(Buffer *buffer)\n> >  {\n> >  \t/* \\todo Handle buffer failures when state is set to BufferError. */\n> > -\tif (buffer->status() == Buffer::BufferCancelled)\n> > +\tif (buffer->info().status() == BufferInfo::BufferCancelled)\n> >  \t\treturn;\n> >\n> >  \timgu_->input_->queueBuffer(buffer);\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index e8b6a278e97b0ba0..6ad9b57d8353896c 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -100,10 +100,10 @@ public:\n> >  \t\tASSERT(frameOffset(SOE) == 0);\n> >\n> >  \t\tutils::time_point soe = std::chrono::time_point<utils::clock>()\n> > -\t\t\t+ std::chrono::nanoseconds(buffer->timestamp())\n> > +\t\t\t+ std::chrono::nanoseconds(buffer->info().timestamp())\n> >  \t\t\t+ timeOffset(SOE);\n> >\n> > -\t\tnotifyStartOfExposure(buffer->sequence(), soe);\n> > +\t\tnotifyStartOfExposure(buffer->info().sequence(), soe);\n> >  \t}\n> >\n> >  \tvoid setDelay(unsigned int type, int frame, int msdelay)\n> > @@ -1006,8 +1006,8 @@ void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)\n> >\n> >  \tdata->timeline_.bufferReady(buffer);\n> >\n> > -\tif (data->frame_ <= buffer->sequence())\n> > -\t\tdata->frame_ = buffer->sequence() + 1;\n> > +\tif (data->frame_ <= buffer->info().sequence())\n> > +\t\tdata->frame_ = buffer->info().sequence() + 1;\n> >\n> >  \tcompleteBuffer(activeCamera_, request, buffer);\n> >  \ttryCompleteRequest(request);\n> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> > index 3b49e52510918eee..7593bf9dfa546401 100644\n> > --- a/src/libcamera/request.cpp\n> > +++ b/src/libcamera/request.cpp\n> > @@ -260,7 +260,7 @@ bool Request::completeBuffer(Buffer *buffer)\n> >\n> >  \tbuffer->request_ = nullptr;\n> >\n> > -\tif (buffer->status() == Buffer::BufferCancelled)\n> > +\tif (buffer->info().status() == BufferInfo::BufferCancelled)\n> >  \t\tcancelled_ = true;\n> >\n> >  \treturn !hasPendingBuffers();\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index cc0a1c9382a2b1ed..8f962c7e9d0c7d01 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -1012,10 +1012,12 @@ int V4L2VideoDevice::queueBuffer(Buffer *buffer)\n> >  \t}\n> >\n> >  \tif (V4L2_TYPE_IS_OUTPUT(buf.type)) {\n> > -\t\tbuf.bytesused = buffer->bytesused_;\n> > -\t\tbuf.sequence = buffer->sequence_;\n> > -\t\tbuf.timestamp.tv_sec = buffer->timestamp_ / 1000000000;\n> > -\t\tbuf.timestamp.tv_usec = (buffer->timestamp_ / 1000) % 1000000;\n> > +\t\tconst BufferInfo &info = buffer->info();\n> > +\n> > +\t\tbuf.bytesused = info.planes()[0].bytesused;\n> > +\t\tbuf.sequence = info.sequence();\n> > +\t\tbuf.timestamp.tv_sec = info.timestamp() / 1000000000;\n> > +\t\tbuf.timestamp.tv_usec = (info.timestamp() / 1000) % 1000000;\n> >  \t}\n> >\n> >  \tLOG(V4L2, Debug) << \"Queueing buffer \" << buf.index;\n> > @@ -1121,12 +1123,13 @@ Buffer *V4L2VideoDevice::dequeueBuffer()\n> >  \t\tfdEvent_->setEnabled(false);\n> >\n> >  \tbuffer->index_ = buf.index;\n> > -\tbuffer->bytesused_ = buf.bytesused;\n> > -\tbuffer->timestamp_ = buf.timestamp.tv_sec * 1000000000ULL\n> > -\t\t\t   + buf.timestamp.tv_usec * 1000ULL;\n> > -\tbuffer->sequence_ = buf.sequence;\n> > -\tbuffer->status_ = buf.flags & V4L2_BUF_FLAG_ERROR\n> > -\t\t\t? Buffer::BufferError : Buffer::BufferSuccess;\n> > +\n> > +\tBufferInfo::Status status = buf.flags & V4L2_BUF_FLAG_ERROR\n> > +\t\t? BufferInfo::BufferError : BufferInfo::BufferSuccess;\n> > +\tuint64_t timestamp = buf.timestamp.tv_sec * 1000000000ULL\n> > +\t\t+ buf.timestamp.tv_usec * 1000ULL;\n\nCould you align the ? and + under the = ?\n\n> > +\n> > +\tbuffer->info_.update(status, buf.sequence, timestamp, { { buf.bytesused } });\n> \n> Here it's where I wondered if we should not provide an update()\n> version which takes a struct v4l2_buffer, but it would maybe be a bit\n> of a layering violation.\n\nAs it is needed in a single place I'd prefer keeping it in\nv4l2_videodevice.cpp.\n\n> >\n> >  \treturn buffer;\n> >  }\n> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > index 4cecf7e351214f3d..771020112f09b1ef 100644\n> > --- a/src/qcam/main_window.cpp\n> > +++ b/src/qcam/main_window.cpp\n> > @@ -258,14 +258,15 @@ void MainWindow::requestComplete(Request *request)\n> >  \tframesCaptured_++;\n> >\n> >  \tBuffer *buffer = buffers.begin()->second;\n> > +\tconst BufferInfo &info = buffer->info();\n> >\n> > -\tdouble fps = buffer->timestamp() - lastBufferTime_;\n> > +\tdouble fps = info.timestamp() - lastBufferTime_;\n> >  \tfps = lastBufferTime_ && fps ? 1000000000.0 / fps : 0.0;\n> > -\tlastBufferTime_ = buffer->timestamp();\n> > +\tlastBufferTime_ = info.timestamp();\n> >\n> > -\tstd::cout << \"seq: \" << std::setw(6) << std::setfill('0') << buffer->sequence()\n> > -\t\t  << \" bytesused: \" << buffer->bytesused()\n> > -\t\t  << \" timestamp: \" << buffer->timestamp()\n> > +\tstd::cout << \"seq: \" << std::setw(6) << std::setfill('0') << info.sequence()\n> > +\t\t  << \" bytesused: \" << info.planes()[0].bytesused\n> \n> Assuming a single plane is fine ?\n\nIt doesn't introduce any regression, but will need to be fixed. We need\nto address multiplanar support.\n\n> > +\t\t  << \" timestamp: \" << info.timestamp()\n> >  \t\t  << \" fps: \" << std::fixed << std::setprecision(2) << fps\n> >  \t\t  << std::endl;\n> >\n> > @@ -302,7 +303,7 @@ int MainWindow::display(Buffer *buffer)\n> >\n> >  \tDmabuf &dmabuf = mem->planes().front();\n> >  \tunsigned char *raw = static_cast<unsigned char *>(dmabuf.mem());\n> > -\tviewfinder_->display(raw, buffer->bytesused());\n> > +\tviewfinder_->display(raw, buffer->info().planes()[0].bytesused);\n> >\n> >  \treturn 0;\n> >  }\n> > diff --git a/test/camera/buffer_import.cpp b/test/camera/buffer_import.cpp\n> > index dae907f9f41841c9..5dbaed9255d3d60c 100644\n> > --- a/test/camera/buffer_import.cpp\n> > +++ b/test/camera/buffer_import.cpp\n> > @@ -275,7 +275,7 @@ public:\n> >  protected:\n> >  \tvoid bufferComplete(Request *request, Buffer *buffer)\n> >  \t{\n> > -\t\tif (buffer->status() != Buffer::BufferSuccess)\n> > +\t\tif (buffer->info().status() != BufferInfo::BufferSuccess)\n> >  \t\t\treturn;\n> >\n> >  \t\tunsigned int index = buffer->index();\n> > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp\n> > index ca1ebe419946dd4d..8307ea2629801679 100644\n> > --- a/test/camera/capture.cpp\n> > +++ b/test/camera/capture.cpp\n> > @@ -28,7 +28,7 @@ protected:\n> >\n> >  \tvoid bufferComplete(Request *request, Buffer *buffer)\n> >  \t{\n> > -\t\tif (buffer->status() != Buffer::BufferSuccess)\n> > +\t\tif (buffer->info().status() != BufferInfo::BufferSuccess)\n> >  \t\t\treturn;\n> >\n> >  \t\tcompleteBuffersCount_++;\n> > diff --git a/test/v4l2_videodevice/buffer_sharing.cpp b/test/v4l2_videodevice/buffer_sharing.cpp\n> > index d02c391b95933922..6b71caef111693d6 100644\n> > --- a/test/v4l2_videodevice/buffer_sharing.cpp\n> > +++ b/test/v4l2_videodevice/buffer_sharing.cpp\n> > @@ -92,10 +92,12 @@ protected:\n> >\n> >  \tvoid captureBufferReady(Buffer *buffer)\n> >  \t{\n> > +\t\tconst BufferInfo &info = buffer->info();\n> > +\n> >  \t\tstd::cout << \"Received capture buffer  sequence \"\n> > -\t\t\t  << buffer->sequence() << std::endl;\n> > +\t\t\t  << info.sequence() << std::endl;\n> >\n> > -\t\tif (buffer->status() != Buffer::BufferSuccess)\n> > +\t\tif (info.status() != BufferInfo::BufferSuccess)\n> >  \t\t\treturn;\n> >\n> >  \t\toutput_->queueBuffer(buffer);\n> > @@ -104,10 +106,12 @@ protected:\n> >\n> >  \tvoid outputBufferReady(Buffer *buffer)\n> >  \t{\n> > +\t\tconst BufferInfo &info = buffer->info();\n> > +\n> >  \t\tstd::cout << \"Received output buffer sequence \"\n> > -\t\t\t  << buffer->sequence() << std::endl;\n> > +\t\t\t  << info.sequence() << std::endl;\n> >\n> > -\t\tif (buffer->status() != Buffer::BufferSuccess)\n> > +\t\tif (info.status() != BufferInfo::BufferSuccess)\n> >  \t\t\treturn;\n> >\n> >  \t\tcapture_->queueBuffer(buffer);","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A8FBF60BDB\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  9 Dec 2019 22:19:40 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1317711B7;\n\tMon,  9 Dec 2019 22:19:40 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1575926380;\n\tbh=wHOfOC2muY0o30JsPAp38b0pf5ndUSWgv5lp7PDjpFM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=G3HxwrCCMexsbAI2tBFLoLQWycpOddV4a9N7YUxpjKoS9MHzSAPq2MkdEtlATOwjQ\n\tzlvgfyS5wiqO8i2YpxMe89+65LEmp4pf3OfxnFcc/5D054AwZZWmd/lC0UZepViAPR\n\t1qVZjA6VEBEhEDvYAmTWMB8Xgq5A+JBfaYFs4Qow=","Date":"Mon, 9 Dec 2019 23:19:33 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"Jacopo Mondi <jacopo@jmondi.org>, libcamera-devel@lists.libcamera.org","Message-ID":"<20191209211933.GF18060@pendragon.ideasonboard.com>","References":"<20191126233620.1695316-1-niklas.soderlund@ragnatech.se>\n\t<20191126233620.1695316-16-niklas.soderlund@ragnatech.se>\n\t<20191127150654.7rohrn5p2kthlsly@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20191127150654.7rohrn5p2kthlsly@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 15/30] libcamera: buffer: Move capture\n\tinformation to BufferInfo","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>","X-List-Received-Date":"Mon, 09 Dec 2019 21:19:40 -0000"}}]