| Message ID | 20211203113205.2470651-1-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Commit | f31be76ae67f0eae8c417faa53d8e0281368e0bb |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for this patch! On Fri, 3 Dec 2021 at 11:32, Naushir Patuck <naush@raspberrypi.com> wrote: > > Return the available sensor PixelFormats and sizes from generateConfiguration() > if the StreamRole is set to StreamRole::Raw. The existing code returns the > PixelFormats and sizes for all other StreamRole types. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 21 ++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 5b76916e9e98..cbfb58562626 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -595,12 +595,23 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > return nullptr; > } > > - /* Translate the V4L2PixelFormat to PixelFormat. */ > std::map<PixelFormat, std::vector<SizeRange>> deviceFormats; > - for (const auto &format : fmts) { > - PixelFormat pf = format.first.toPixelFormat(); > - if (pf.isValid()) > - deviceFormats[pf] = format.second; > + if (role == StreamRole::Raw) { > + /* Translate the MBUS codes to a PixelFormat. */ > + for (const auto &format : data->sensorFormats_) { > + PixelFormat pf = mbusCodeToPixelFormat(format.first, > + BayerFormat::Packing::CSI2); > + if (pf.isValid()) > + deviceFormats.emplace(std::piecewise_construct, std::forward_as_tuple(pf), > + std::forward_as_tuple(format.second.begin(), format.second.end())); I won't even begin to pretend that I understand this! I guess I wonder just a little bit whether one could write it in a more "obvious" way that the compiler could still optimise, but really I have no clue. So anyway: Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks! David > + } > + } else { > + /* Translate the V4L2PixelFormat to PixelFormat. */ > + for (const auto &format : fmts) { > + PixelFormat pf = format.first.toPixelFormat(); > + if (pf.isValid()) > + deviceFormats[pf] = format.second; > + } > } > > /* Add the stream format based on the device node used for the use case. */ > -- > 2.25.1 >
Actually I found myself looking at this bit of code again as part of my occasional quest to figure out why the gstreamer element and v4l2 compatibility library work so poorly. I was wondering if we should limit the output sizes to the max sensor resolution here. The trouble with advertising 16384x16384 is that applications (cheese, for example) decide that we really want 8K video and unsurprisingly it's all a complete disaster. What do you think - is this the right place to tackle the problem? (Or if not, where else?) Though I've not checked, maybe this would help the v4l2 compatibility layer too? (anyone know?) David On Fri, 3 Dec 2021 at 12:40, David Plowman <david.plowman@raspberrypi.com> wrote: > > Hi Naush > > Thanks for this patch! > > On Fri, 3 Dec 2021 at 11:32, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > Return the available sensor PixelFormats and sizes from generateConfiguration() > > if the StreamRole is set to StreamRole::Raw. The existing code returns the > > PixelFormats and sizes for all other StreamRole types. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 21 ++++++++++++++----- > > 1 file changed, 16 insertions(+), 5 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 5b76916e9e98..cbfb58562626 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -595,12 +595,23 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > > return nullptr; > > } > > > > - /* Translate the V4L2PixelFormat to PixelFormat. */ > > std::map<PixelFormat, std::vector<SizeRange>> deviceFormats; > > - for (const auto &format : fmts) { > > - PixelFormat pf = format.first.toPixelFormat(); > > - if (pf.isValid()) > > - deviceFormats[pf] = format.second; > > + if (role == StreamRole::Raw) { > > + /* Translate the MBUS codes to a PixelFormat. */ > > + for (const auto &format : data->sensorFormats_) { > > + PixelFormat pf = mbusCodeToPixelFormat(format.first, > > + BayerFormat::Packing::CSI2); > > + if (pf.isValid()) > > + deviceFormats.emplace(std::piecewise_construct, std::forward_as_tuple(pf), > > + std::forward_as_tuple(format.second.begin(), format.second.end())); > > I won't even begin to pretend that I understand this! I guess I wonder > just a little bit whether one could write it in a more "obvious" way > that the compiler could still optimise, but really I have no clue. So > anyway: > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > Thanks! > David > > > + } > > + } else { > > + /* Translate the V4L2PixelFormat to PixelFormat. */ > > + for (const auto &format : fmts) { > > + PixelFormat pf = format.first.toPixelFormat(); > > + if (pf.isValid()) > > + deviceFormats[pf] = format.second; > > + } > > } > > > > /* Add the stream format based on the device node used for the use case. */ > > -- > > 2.25.1 > >
Quoting David Plowman (2021-12-03 17:45:53) > Actually I found myself looking at this bit of code again as part of > my occasional quest to figure out why the gstreamer element and v4l2 > compatibility library work so poorly. > > I was wondering if we should limit the output sizes to the max sensor > resolution here. The trouble with advertising 16384x16384 is that > applications (cheese, for example) decide that we really want 8K video > and unsurprisingly it's all a complete disaster. > > What do you think - is this the right place to tackle the problem? (Or > if not, where else?) > > Though I've not checked, maybe this would help the v4l2 compatibility > layer too? (anyone know?) > > David > > On Fri, 3 Dec 2021 at 12:40, David Plowman > <david.plowman@raspberrypi.com> wrote: > > > > Hi Naush > > > > Thanks for this patch! > > > > On Fri, 3 Dec 2021 at 11:32, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > > > Return the available sensor PixelFormats and sizes from generateConfiguration() > > > if the StreamRole is set to StreamRole::Raw. The existing code returns the > > > PixelFormats and sizes for all other StreamRole types. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 21 ++++++++++++++----- > > > 1 file changed, 16 insertions(+), 5 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index 5b76916e9e98..cbfb58562626 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -595,12 +595,23 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > > > return nullptr; > > > } > > > > > > - /* Translate the V4L2PixelFormat to PixelFormat. */ > > > std::map<PixelFormat, std::vector<SizeRange>> deviceFormats; > > > - for (const auto &format : fmts) { > > > - PixelFormat pf = format.first.toPixelFormat(); > > > - if (pf.isValid()) > > > - deviceFormats[pf] = format.second; > > > + if (role == StreamRole::Raw) { > > > + /* Translate the MBUS codes to a PixelFormat. */ > > > + for (const auto &format : data->sensorFormats_) { > > > + PixelFormat pf = mbusCodeToPixelFormat(format.first, > > > + BayerFormat::Packing::CSI2); > > > + if (pf.isValid()) > > > + deviceFormats.emplace(std::piecewise_construct, std::forward_as_tuple(pf), > > > + std::forward_as_tuple(format.second.begin(), format.second.end())); > > > > I won't even begin to pretend that I understand this! I guess I wonder > > just a little bit whether one could write it in a more "obvious" way > > that the compiler could still optimise, but really I have no clue. So > > anyway: > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > > Thanks! > > David > > > > > + } > > > + } else { > > > + /* Translate the V4L2PixelFormat to PixelFormat. */ > > > + for (const auto &format : fmts) { > > > + PixelFormat pf = format.first.toPixelFormat(); > > > + if (pf.isValid()) > > > + deviceFormats[pf] = format.second; > > > + } > > > } > > > > > > /* Add the stream format based on the device node used for the use case. */ > > > -- > > > 2.25.1 > > >
Hi David, On Fri, 3 Dec 2021 at 17:46, David Plowman <david.plowman@raspberrypi.com> wrote: > Actually I found myself looking at this bit of code again as part of > my occasional quest to figure out why the gstreamer element and v4l2 > compatibility library work so poorly. > > I was wondering if we should limit the output sizes to the max sensor > resolution here. The trouble with advertising 16384x16384 is that > applications (cheese, for example) decide that we really want 8K video > and unsurprisingly it's all a complete disaster. > > What do you think - is this the right place to tackle the problem? (Or > if not, where else?) > > Though I've not checked, maybe this would help the v4l2 compatibility > layer too? (anyone know?) > Yes, that does make sense. Once this goes in, I'll look to make that change. Regards, Naush > > David > > On Fri, 3 Dec 2021 at 12:40, David Plowman > <david.plowman@raspberrypi.com> wrote: > > > > Hi Naush > > > > Thanks for this patch! > > > > On Fri, 3 Dec 2021 at 11:32, Naushir Patuck <naush@raspberrypi.com> > wrote: > > > > > > Return the available sensor PixelFormats and sizes from > generateConfiguration() > > > if the StreamRole is set to StreamRole::Raw. The existing code returns > the > > > PixelFormats and sizes for all other StreamRole types. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 21 ++++++++++++++----- > > > 1 file changed, 16 insertions(+), 5 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index 5b76916e9e98..cbfb58562626 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -595,12 +595,23 @@ CameraConfiguration > *PipelineHandlerRPi::generateConfiguration(Camera *camera, > > > return nullptr; > > > } > > > > > > - /* Translate the V4L2PixelFormat to PixelFormat. */ > > > std::map<PixelFormat, std::vector<SizeRange>> > deviceFormats; > > > - for (const auto &format : fmts) { > > > - PixelFormat pf = format.first.toPixelFormat(); > > > - if (pf.isValid()) > > > - deviceFormats[pf] = format.second; > > > + if (role == StreamRole::Raw) { > > > + /* Translate the MBUS codes to a PixelFormat. > */ > > > + for (const auto &format : > data->sensorFormats_) { > > > + PixelFormat pf = > mbusCodeToPixelFormat(format.first, > > > + > BayerFormat::Packing::CSI2); > > > + if (pf.isValid()) > > > + > deviceFormats.emplace(std::piecewise_construct, std::forward_as_tuple(pf), > > > + > std::forward_as_tuple(format.second.begin(), format.second.end())); > > > > I won't even begin to pretend that I understand this! I guess I wonder > > just a little bit whether one could write it in a more "obvious" way > > that the compiler could still optimise, but really I have no clue. So > > anyway: > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > > Thanks! > > David > > > > > + } > > > + } else { > > > + /* Translate the V4L2PixelFormat to > PixelFormat. */ > > > + for (const auto &format : fmts) { > > > + PixelFormat pf = > format.first.toPixelFormat(); > > > + if (pf.isValid()) > > > + deviceFormats[pf] = > format.second; > > > + } > > > } > > > > > > /* Add the stream format based on the device node used > for the use case. */ > > > -- > > > 2.25.1 > > > >
Quoting Naushir Patuck (2021-12-03 11:32:05) > Return the available sensor PixelFormats and sizes from generateConfiguration() > if the StreamRole is set to StreamRole::Raw. The existing code returns the > PixelFormats and sizes for all other StreamRole types. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 21 ++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 5b76916e9e98..cbfb58562626 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -595,12 +595,23 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > return nullptr; > } > > - /* Translate the V4L2PixelFormat to PixelFormat. */ > std::map<PixelFormat, std::vector<SizeRange>> deviceFormats; > - for (const auto &format : fmts) { > - PixelFormat pf = format.first.toPixelFormat(); > - if (pf.isValid()) > - deviceFormats[pf] = format.second; > + if (role == StreamRole::Raw) { > + /* Translate the MBUS codes to a PixelFormat. */ > + for (const auto &format : data->sensorFormats_) { > + PixelFormat pf = mbusCodeToPixelFormat(format.first, > + BayerFormat::Packing::CSI2); > + if (pf.isValid()) > + deviceFormats.emplace(std::piecewise_construct, std::forward_as_tuple(pf), > + std::forward_as_tuple(format.second.begin(), format.second.end())); I can't tell right now if this could be simpler, but I think it's fine as it is if it works for you and produces the right result. I think this functionality could or rather /should/ be done in the CameraSensor class to return these formats to simplify this for each pipeline handler, as they should all do this in the same way if they use a CameraSensor. However, I think that move to CameraSensor and adapting of the other handlers can be done on top. So, Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + } > + } else { > + /* Translate the V4L2PixelFormat to PixelFormat. */ > + for (const auto &format : fmts) { > + PixelFormat pf = format.first.toPixelFormat(); > + if (pf.isValid()) > + deviceFormats[pf] = format.second; > + } > } > > /* Add the stream format based on the device node used for the use case. */ > -- > 2.25.1 >
Hello, On Mon, Dec 06, 2021 at 05:19:56PM +0000, Kieran Bingham wrote: > Quoting Naushir Patuck (2021-12-03 11:32:05) > > Return the available sensor PixelFormats and sizes from generateConfiguration() > > if the StreamRole is set to StreamRole::Raw. The existing code returns the > > PixelFormats and sizes for all other StreamRole types. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 21 ++++++++++++++----- > > 1 file changed, 16 insertions(+), 5 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 5b76916e9e98..cbfb58562626 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -595,12 +595,23 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, > > return nullptr; > > } > > > > - /* Translate the V4L2PixelFormat to PixelFormat. */ > > std::map<PixelFormat, std::vector<SizeRange>> deviceFormats; > > - for (const auto &format : fmts) { > > - PixelFormat pf = format.first.toPixelFormat(); > > - if (pf.isValid()) > > - deviceFormats[pf] = format.second; > > + if (role == StreamRole::Raw) { > > + /* Translate the MBUS codes to a PixelFormat. */ > > + for (const auto &format : data->sensorFormats_) { > > + PixelFormat pf = mbusCodeToPixelFormat(format.first, > > + BayerFormat::Packing::CSI2); > > + if (pf.isValid()) > > + deviceFormats.emplace(std::piecewise_construct, std::forward_as_tuple(pf), > > + std::forward_as_tuple(format.second.begin(), format.second.end())); > > I can't tell right now if this could be simpler, but I think it's fine > as it is if it works for you and produces the right result. That's likely the shortest piece of code possible, even if not the most explicit one (declaring an explicit intermediate std::vector<SizeRange> would be more readable I think). > I think this functionality could or rather /should/ be done in the > CameraSensor class to return these formats to simplify this for each > pipeline handler, as they should all do this in the same way if they use > a CameraSensor. Except that how a media bus code is translated to a pixel format is specific to the video device, it's not an intrinsic property of the camera sensor. > However, I think that move to CameraSensor and adapting of the other > handlers can be done on top. > > So, > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > + } > > + } else { > > + /* Translate the V4L2PixelFormat to PixelFormat. */ > > + for (const auto &format : fmts) { > > + PixelFormat pf = format.first.toPixelFormat(); > > + if (pf.isValid()) > > + deviceFormats[pf] = format.second; > > + } > > } > > > > /* Add the stream format based on the device node used for the use case. */
Hi all, Thank you for your reviews! On Mon, 6 Dec 2021 at 17:49, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hello, > > On Mon, Dec 06, 2021 at 05:19:56PM +0000, Kieran Bingham wrote: > > Quoting Naushir Patuck (2021-12-03 11:32:05) > > > Return the available sensor PixelFormats and sizes from > generateConfiguration() > > > if the StreamRole is set to StreamRole::Raw. The existing code returns > the > > > PixelFormats and sizes for all other StreamRole types. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 21 ++++++++++++++----- > > > 1 file changed, 16 insertions(+), 5 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index 5b76916e9e98..cbfb58562626 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -595,12 +595,23 @@ CameraConfiguration > *PipelineHandlerRPi::generateConfiguration(Camera *camera, > > > return nullptr; > > > } > > > > > > - /* Translate the V4L2PixelFormat to PixelFormat. */ > > > std::map<PixelFormat, std::vector<SizeRange>> > deviceFormats; > > > - for (const auto &format : fmts) { > > > - PixelFormat pf = format.first.toPixelFormat(); > > > - if (pf.isValid()) > > > - deviceFormats[pf] = format.second; > > > + if (role == StreamRole::Raw) { > > > + /* Translate the MBUS codes to a PixelFormat. > */ > > > + for (const auto &format : > data->sensorFormats_) { > > > + PixelFormat pf = > mbusCodeToPixelFormat(format.first, > > > + > BayerFormat::Packing::CSI2); > > > + if (pf.isValid()) > > > + > deviceFormats.emplace(std::piecewise_construct, std::forward_as_tuple(pf), > > > + > std::forward_as_tuple(format.second.begin(), format.second.end())); > > > > I can't tell right now if this could be simpler, but I think it's fine > > as it is if it works for you and produces the right result. > > That's likely the shortest piece of code possible, even if not the most > explicit one (declaring an explicit intermediate std::vector<SizeRange> > would be more readable I think). > This is the only way I could think of constructing deviceFormat without an intermediate temporary std::vector. If others feel this construct is somewhat obfuscated, I am happy to add a temporary for readability. If not, then feel free to merge this :-) Regards, Naush > > > I think this functionality could or rather /should/ be done in the > > CameraSensor class to return these formats to simplify this for each > > pipeline handler, as they should all do this in the same way if they use > > a CameraSensor. > > Except that how a media bus code is translated to a pixel format is > specific to the video device, it's not an intrinsic property of the > camera sensor. > > > However, I think that move to CameraSensor and adapting of the other > > handlers can be done on top. > > > > So, > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > + } > > > + } else { > > > + /* Translate the V4L2PixelFormat to > PixelFormat. */ > > > + for (const auto &format : fmts) { > > > + PixelFormat pf = > format.first.toPixelFormat(); > > > + if (pf.isValid()) > > > + deviceFormats[pf] = > format.second; > > > + } > > > } > > > > > > /* Add the stream format based on the device node used > for the use case. */ > > -- > Regards, > > Laurent Pinchart >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 5b76916e9e98..cbfb58562626 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -595,12 +595,23 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera, return nullptr; } - /* Translate the V4L2PixelFormat to PixelFormat. */ std::map<PixelFormat, std::vector<SizeRange>> deviceFormats; - for (const auto &format : fmts) { - PixelFormat pf = format.first.toPixelFormat(); - if (pf.isValid()) - deviceFormats[pf] = format.second; + if (role == StreamRole::Raw) { + /* Translate the MBUS codes to a PixelFormat. */ + for (const auto &format : data->sensorFormats_) { + PixelFormat pf = mbusCodeToPixelFormat(format.first, + BayerFormat::Packing::CSI2); + if (pf.isValid()) + deviceFormats.emplace(std::piecewise_construct, std::forward_as_tuple(pf), + std::forward_as_tuple(format.second.begin(), format.second.end())); + } + } else { + /* Translate the V4L2PixelFormat to PixelFormat. */ + for (const auto &format : fmts) { + PixelFormat pf = format.first.toPixelFormat(); + if (pf.isValid()) + deviceFormats[pf] = format.second; + } } /* Add the stream format based on the device node used for the use case. */
Return the available sensor PixelFormats and sizes from generateConfiguration() if the StreamRole is set to StreamRole::Raw. The existing code returns the PixelFormats and sizes for all other StreamRole types. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- .../pipeline/raspberrypi/raspberrypi.cpp | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)