| Message ID | 20191018154201.13540-3-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Fri, Oct 18, 2019 at 04:42:01PM +0100, Kieran Bingham wrote: > When we dequeue a buffer using the MPLANE API, we currently set our > buffer->bytesused field to zero due to the zero entry in the V4L2 buffer > bytesused field. > > The bytesused field is only really tracked for debug purposes currently, > so store the total bytese used from each plane in our local statistics > to identify actual data flow. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > This patch comes with a pinch of salt, we could also add a bytesused > field to the Plane class and intead update all the debug prints to > report the bytesused on a per-plane basis. My initial opinion is that handling this in the Buffer and Plane classes would be better. I think the V4L2-related classes should stay as close as possible to the V4L2 API, otherwise it could soon become quite confusing. Niklas is planning to rework the buffer API this week, he should take this into consideration. > --- > src/libcamera/v4l2_videodevice.cpp | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index 37f61b90ac46..71f141e7b511 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -1124,13 +1124,20 @@ Buffer *V4L2VideoDevice::dequeueBuffer() > fdEvent_->setEnabled(false); > > buffer->index_ = buf.index; > - buffer->bytesused_ = buf.bytesused; > buffer->timestamp_ = buf.timestamp.tv_sec * 1000000000ULL > + buf.timestamp.tv_usec * 1000ULL; > buffer->sequence_ = buf.sequence; > buffer->status_ = buf.flags & V4L2_BUF_FLAG_ERROR > ? Buffer::BufferError : Buffer::BufferSuccess; > > + if (multiPlanar_) { > + buffer->bytesused_ = 0; > + for (unsigned int p = 0; p < buf.length; ++p) > + buffer->bytesused_ += planes[p].bytesused; > + } else { > + buffer->bytesused_ = buf.bytesused; > + } > + > return buffer; > } >
Hi Laurent, On 20/10/2019 06:20, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Fri, Oct 18, 2019 at 04:42:01PM +0100, Kieran Bingham wrote: >> When we dequeue a buffer using the MPLANE API, we currently set our >> buffer->bytesused field to zero due to the zero entry in the V4L2 buffer >> bytesused field. >> >> The bytesused field is only really tracked for debug purposes currently, >> so store the total bytese used from each plane in our local statistics >> to identify actual data flow. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >> --- >> >> This patch comes with a pinch of salt, we could also add a bytesused >> field to the Plane class and intead update all the debug prints to >> report the bytesused on a per-plane basis. > > My initial opinion is that handling this in the Buffer and Plane classes > would be better. I think the V4L2-related classes should stay as close > as possible to the V4L2 API, otherwise it could soon become quite > confusing. > > Niklas is planning to rework the buffer API this week, he should take > this into consideration. That's fine, this patch was mostly sent to highlight the issue, and discuss, as noted by the 'pinch of salt'. I'll await the buffer rework. >> --- >> src/libcamera/v4l2_videodevice.cpp | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp >> index 37f61b90ac46..71f141e7b511 100644 >> --- a/src/libcamera/v4l2_videodevice.cpp >> +++ b/src/libcamera/v4l2_videodevice.cpp >> @@ -1124,13 +1124,20 @@ Buffer *V4L2VideoDevice::dequeueBuffer() >> fdEvent_->setEnabled(false); >> >> buffer->index_ = buf.index; >> - buffer->bytesused_ = buf.bytesused; >> buffer->timestamp_ = buf.timestamp.tv_sec * 1000000000ULL >> + buf.timestamp.tv_usec * 1000ULL; >> buffer->sequence_ = buf.sequence; >> buffer->status_ = buf.flags & V4L2_BUF_FLAG_ERROR >> ? Buffer::BufferError : Buffer::BufferSuccess; >> >> + if (multiPlanar_) { >> + buffer->bytesused_ = 0; >> + for (unsigned int p = 0; p < buf.length; ++p) >> + buffer->bytesused_ += planes[p].bytesused; >> + } else { >> + buffer->bytesused_ = buf.bytesused; >> + } >> + >> return buffer; >> } >> >
On 2019-10-21 11:36:51 +0100, Kieran Bingham wrote: > Hi Laurent, > > On 20/10/2019 06:20, Laurent Pinchart wrote: > > Hi Kieran, > > > > Thank you for the patch. > > > > On Fri, Oct 18, 2019 at 04:42:01PM +0100, Kieran Bingham wrote: > >> When we dequeue a buffer using the MPLANE API, we currently set our > >> buffer->bytesused field to zero due to the zero entry in the V4L2 buffer > >> bytesused field. > >> > >> The bytesused field is only really tracked for debug purposes currently, > >> so store the total bytese used from each plane in our local statistics > >> to identify actual data flow. > >> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> > >> --- > >> > >> This patch comes with a pinch of salt, we could also add a bytesused > >> field to the Plane class and intead update all the debug prints to > >> report the bytesused on a per-plane basis. > > > > My initial opinion is that handling this in the Buffer and Plane classes > > would be better. I think the V4L2-related classes should stay as close > > as possible to the V4L2 API, otherwise it could soon become quite > > confusing. > > > > Niklas is planning to rework the buffer API this week, he should take > > this into consideration. > > That's fine, this patch was mostly sent to highlight the issue, and > discuss, as noted by the 'pinch of salt'. > > > I'll await the buffer rework. This is addressed in the buffer rework series. > > > >> --- > >> src/libcamera/v4l2_videodevice.cpp | 9 ++++++++- > >> 1 file changed, 8 insertions(+), 1 deletion(-) > >> > >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > >> index 37f61b90ac46..71f141e7b511 100644 > >> --- a/src/libcamera/v4l2_videodevice.cpp > >> +++ b/src/libcamera/v4l2_videodevice.cpp > >> @@ -1124,13 +1124,20 @@ Buffer *V4L2VideoDevice::dequeueBuffer() > >> fdEvent_->setEnabled(false); > >> > >> buffer->index_ = buf.index; > >> - buffer->bytesused_ = buf.bytesused; > >> buffer->timestamp_ = buf.timestamp.tv_sec * 1000000000ULL > >> + buf.timestamp.tv_usec * 1000ULL; > >> buffer->sequence_ = buf.sequence; > >> buffer->status_ = buf.flags & V4L2_BUF_FLAG_ERROR > >> ? Buffer::BufferError : Buffer::BufferSuccess; > >> > >> + if (multiPlanar_) { > >> + buffer->bytesused_ = 0; > >> + for (unsigned int p = 0; p < buf.length; ++p) > >> + buffer->bytesused_ += planes[p].bytesused; > >> + } else { > >> + buffer->bytesused_ = buf.bytesused; > >> + } > >> + > >> return buffer; > >> } > >> > > > > -- > Regards > -- > Kieran > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index 37f61b90ac46..71f141e7b511 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -1124,13 +1124,20 @@ Buffer *V4L2VideoDevice::dequeueBuffer() fdEvent_->setEnabled(false); buffer->index_ = buf.index; - buffer->bytesused_ = buf.bytesused; buffer->timestamp_ = buf.timestamp.tv_sec * 1000000000ULL + buf.timestamp.tv_usec * 1000ULL; buffer->sequence_ = buf.sequence; buffer->status_ = buf.flags & V4L2_BUF_FLAG_ERROR ? Buffer::BufferError : Buffer::BufferSuccess; + if (multiPlanar_) { + buffer->bytesused_ = 0; + for (unsigned int p = 0; p < buf.length; ++p) + buffer->bytesused_ += planes[p].bytesused; + } else { + buffer->bytesused_ = buf.bytesused; + } + return buffer; }
When we dequeue a buffer using the MPLANE API, we currently set our buffer->bytesused field to zero due to the zero entry in the V4L2 buffer bytesused field. The bytesused field is only really tracked for debug purposes currently, so store the total bytese used from each plane in our local statistics to identify actual data flow. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- This patch comes with a pinch of salt, we could also add a bytesused field to the Plane class and intead update all the debug prints to report the bytesused on a per-plane basis. --- src/libcamera/v4l2_videodevice.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)