| Message ID | 20190228200410.3022-3-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Thu, Feb 28, 2019 at 09:04:02PM +0100, Jacopo Mondi wrote: > Inspect all image sizes provided by the sensor and select the > biggest one to be returned as default stream configuration instead of > returning the currently applied one. > > Hardcode the stream pixel format to the one produced by the CIO2 unit, > to be changed to the one provided by the ImgU. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 44 ++++++++++++++++++++-------- > 1 file changed, 32 insertions(+), 12 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index d3f1d9a95f81..4f1ab72debf8 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -71,6 +71,8 @@ public: > bool match(DeviceEnumerator *enumerator); > > private: > + static constexpr unsigned int IPU3_BUF_NUM = 4; IPU3_BUF_COUNT or IPU3_BUFFER_COUNT to match the name of the bufferCount field ? > + > IPU3CameraData *cameraData(const Camera *camera) > { > return static_cast<IPU3CameraData *>( > @@ -102,27 +104,45 @@ std::map<Stream *, StreamConfiguration> > PipelineHandlerIPU3::streamConfiguration(Camera *camera, > std::vector<Stream *> &streams) > { > + std::map<unsigned int, std::vector<SizeRange>> formats; How about movinf this line down to declare and initialize the variable at the same time ? > std::map<Stream *, StreamConfiguration> configs; > IPU3CameraData *data = cameraData(camera); > V4L2Subdevice *sensor = data->cio2.sensor; > - V4L2SubdeviceFormat format = {}; > + StreamConfiguration *config = &configs[&data->stream_]; > + > + config->pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10; > + config->bufferCount = IPU3_BUF_NUM; > > /* > - * FIXME: As of now, return the image format reported by the sensor. > - * In future good defaults should be provided for each stream. > + * Use the largest image size the sensor provides or > + * use a default one. > */ > - if (sensor->getFormat(0, &format)) { > - LOG(IPU3, Error) << "Failed to create stream configurations"; > - return configs; > + formats = sensor->formats(0); > + if (formats.empty()) { > + config->width = 1920; > + config->height = 1080; > + LOG(IPU3, Info) > + << "Use default stream sizes " << config->width > + << "x" << config->height; It doesn't make much sense to fall back to a default value here, all sensor drivers must report the sizes they support. If you're worried they may not, let's add a check when creating the camera and error out. You could then cache the formats supported by the sensor in the CIO2Device structure. > } > > - StreamConfiguration config = {}; > - config.width = format.width; > - config.height = format.height; > - config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10; > - config.bufferCount = 4; > + auto it = formats.begin(); > + while (it != formats.end()) { Why do you dislike for loops ? :-) for (auto it = formats.begin(); it != formats.end(); ++it) { And I'd even make it a const auto it and a const SizeRange & below as you don't modify it. > + for (SizeRange &range : it->second) { > + if (range.maxWidth <= config->width || > + range.maxHeight <= config->height) > + continue; > + > + config->width = range.maxWidth; > + config->height = range.maxHeight; > + } What happens if the sensor supports two sizes with different aspect ratios, let's say 4:3 and 16:9, where one has a smaller width by larger height ? Shouldn't we compare the areas instead of the individual dimensions ? > + > + ++it; > + } > > - configs[&data->stream_] = config; > + LOG(IPU3, Info) << "Stream format set to = (" << config->width << "x" > + << config->height << ") - 0x" << std::hex I know how much you dislike the streams API, but 0x%08x would be nice. > + << config->pixelFormat; Wouldn't Debug make more sense than Info here ? > > return configs; > }
Hi Laurent, On Sat, Mar 02, 2019 at 11:23:52PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Thu, Feb 28, 2019 at 09:04:02PM +0100, Jacopo Mondi wrote: > > Inspect all image sizes provided by the sensor and select the > > biggest one to be returned as default stream configuration instead of > > returning the currently applied one. > > > > Hardcode the stream pixel format to the one produced by the CIO2 unit, > > to be changed to the one provided by the ImgU. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 44 ++++++++++++++++++++-------- > > 1 file changed, 32 insertions(+), 12 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index d3f1d9a95f81..4f1ab72debf8 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -71,6 +71,8 @@ public: > > bool match(DeviceEnumerator *enumerator); > > > > private: > > + static constexpr unsigned int IPU3_BUF_NUM = 4; > > IPU3_BUF_COUNT or IPU3_BUFFER_COUNT to match the name of the bufferCount > field ? I'll use IPU3_BUFFER_COUNT > > > + > > IPU3CameraData *cameraData(const Camera *camera) > > { > > return static_cast<IPU3CameraData *>( > > @@ -102,27 +104,45 @@ std::map<Stream *, StreamConfiguration> > > PipelineHandlerIPU3::streamConfiguration(Camera *camera, > > std::vector<Stream *> &streams) > > { > > + std::map<unsigned int, std::vector<SizeRange>> formats; > > How about movinf this line down to declare and initialize the variable > at the same time ? > Will do. > > std::map<Stream *, StreamConfiguration> configs; > > IPU3CameraData *data = cameraData(camera); > > V4L2Subdevice *sensor = data->cio2.sensor; > > - V4L2SubdeviceFormat format = {}; > > + StreamConfiguration *config = &configs[&data->stream_]; > > + > > + config->pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10; > > + config->bufferCount = IPU3_BUF_NUM; > > > > /* > > - * FIXME: As of now, return the image format reported by the sensor. > > - * In future good defaults should be provided for each stream. > > + * Use the largest image size the sensor provides or > > + * use a default one. > > */ > > - if (sensor->getFormat(0, &format)) { > > - LOG(IPU3, Error) << "Failed to create stream configurations"; > > - return configs; > > + formats = sensor->formats(0); > > + if (formats.empty()) { > > + config->width = 1920; > > + config->height = 1080; > > + LOG(IPU3, Info) > > + << "Use default stream sizes " << config->width > > + << "x" << config->height; > > It doesn't make much sense to fall back to a default value here, all > sensor drivers must report the sizes they support. If you're worried > they may not, let's add a check when creating the camera and error out. > You could then cache the formats supported by the sensor in the > CIO2Device structure. > Yes, I agree, defaults are bad. I'll drop this part and assume the sensor provides a list of formats. > > } > > > > - StreamConfiguration config = {}; > > - config.width = format.width; > > - config.height = format.height; > > - config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10; > > - config.bufferCount = 4; > > + auto it = formats.begin(); > > + while (it != formats.end()) { > > Why do you dislike for loops ? :-) > > for (auto it = formats.begin(); it != formats.end(); ++it) { > > And I'd even make it a const auto it and a const SizeRange & below as > you don't modify it. I don't know, I always associate iterators to while loops. Bad habit, I'll change this. > > > + for (SizeRange &range : it->second) { > > + if (range.maxWidth <= config->width || > > + range.maxHeight <= config->height) > > + continue; > > + > > + config->width = range.maxWidth; > > + config->height = range.maxHeight; > > + } > > What happens if the sensor supports two sizes with different aspect > ratios, let's say 4:3 and 16:9, where one has a smaller width by larger > height ? Shouldn't we compare the areas instead of the individual > dimensions ? > Should I add an area comparator to Geometry maybe? > > + > > + ++it; > > + } > > > > - configs[&data->stream_] = config; > > + LOG(IPU3, Info) << "Stream format set to = (" << config->width << "x" > > + << config->height << ") - 0x" << std::hex > > I know how much you dislike the streams API, but 0x%08x would be nice. > > > + << config->pixelFormat; > > Wouldn't Debug make more sense than Info here ? As we agreed offline, most of the Info printouts in this series will be turned into Debug ones. Thanks j > > > > > return configs; > > } > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Sat, Mar 09, 2019 at 09:39:49PM +0100, Jacopo Mondi wrote: > On Sat, Mar 02, 2019 at 11:23:52PM +0200, Laurent Pinchart wrote: > > On Thu, Feb 28, 2019 at 09:04:02PM +0100, Jacopo Mondi wrote: > >> Inspect all image sizes provided by the sensor and select the > >> biggest one to be returned as default stream configuration instead of > >> returning the currently applied one. > >> > >> Hardcode the stream pixel format to the one produced by the CIO2 unit, > >> to be changed to the one provided by the ImgU. > >> > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >> --- > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 44 ++++++++++++++++++++-------- > >> 1 file changed, 32 insertions(+), 12 deletions(-) > >> > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> index d3f1d9a95f81..4f1ab72debf8 100644 > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> @@ -71,6 +71,8 @@ public: > >> bool match(DeviceEnumerator *enumerator); > >> > >> private: > >> + static constexpr unsigned int IPU3_BUF_NUM = 4; > > > > IPU3_BUF_COUNT or IPU3_BUFFER_COUNT to match the name of the bufferCount > > field ? > > I'll use IPU3_BUFFER_COUNT > > >> + > >> IPU3CameraData *cameraData(const Camera *camera) > >> { > >> return static_cast<IPU3CameraData *>( > >> @@ -102,27 +104,45 @@ std::map<Stream *, StreamConfiguration> > >> PipelineHandlerIPU3::streamConfiguration(Camera *camera, > >> std::vector<Stream *> &streams) > >> { > >> + std::map<unsigned int, std::vector<SizeRange>> formats; > > > > How about movinf this line down to declare and initialize the variable > > at the same time ? > > Will do. > > >> std::map<Stream *, StreamConfiguration> configs; > >> IPU3CameraData *data = cameraData(camera); > >> V4L2Subdevice *sensor = data->cio2.sensor; > >> - V4L2SubdeviceFormat format = {}; > >> + StreamConfiguration *config = &configs[&data->stream_]; > >> + > >> + config->pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10; > >> + config->bufferCount = IPU3_BUF_NUM; > >> > >> /* > >> - * FIXME: As of now, return the image format reported by the sensor. > >> - * In future good defaults should be provided for each stream. > >> + * Use the largest image size the sensor provides or > >> + * use a default one. > >> */ > >> - if (sensor->getFormat(0, &format)) { > >> - LOG(IPU3, Error) << "Failed to create stream configurations"; > >> - return configs; > >> + formats = sensor->formats(0); > >> + if (formats.empty()) { > >> + config->width = 1920; > >> + config->height = 1080; > >> + LOG(IPU3, Info) > >> + << "Use default stream sizes " << config->width > >> + << "x" << config->height; > > > > It doesn't make much sense to fall back to a default value here, all > > sensor drivers must report the sizes they support. If you're worried > > they may not, let's add a check when creating the camera and error out. > > You could then cache the formats supported by the sensor in the > > CIO2Device structure. > > Yes, I agree, defaults are bad. I'll drop this part and assume the > sensor provides a list of formats. Retrieving the size at open() time and caching it would be a good idea I think, but that can be done outside of this patch if you prefer (please add a \todo in that case). > >> } > >> > >> - StreamConfiguration config = {}; > >> - config.width = format.width; > >> - config.height = format.height; > >> - config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10; > >> - config.bufferCount = 4; > >> + auto it = formats.begin(); > >> + while (it != formats.end()) { > > > > Why do you dislike for loops ? :-) > > > > for (auto it = formats.begin(); it != formats.end(); ++it) { > > > > And I'd even make it a const auto it and a const SizeRange & below as > > you don't modify it. > > I don't know, I always associate iterators to while loops. Bad habit, > I'll change this. > > >> + for (SizeRange &range : it->second) { > >> + if (range.maxWidth <= config->width || > >> + range.maxHeight <= config->height) > >> + continue; > >> + > >> + config->width = range.maxWidth; > >> + config->height = range.maxHeight; > >> + } > > > > What happens if the sensor supports two sizes with different aspect > > ratios, let's say 4:3 and 16:9, where one has a smaller width by larger > > height ? Shouldn't we compare the areas instead of the individual > > dimensions ? > > Should I add an area comparator to Geometry maybe? I think that's a good idea, it will allow us to standardize comparisons in the library. > >> + > >> + ++it; > >> + } > >> > >> - configs[&data->stream_] = config; > >> + LOG(IPU3, Info) << "Stream format set to = (" << config->width << "x" > >> + << config->height << ") - 0x" << std::hex > > > > I know how much you dislike the streams API, but 0x%08x would be nice. > > > >> + << config->pixelFormat; > > > > Wouldn't Debug make more sense than Info here ? > > As we agreed offline, most of the Info printouts in this series will > be turned into Debug ones. > > >> > >> return configs; > >> } > >
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index d3f1d9a95f81..4f1ab72debf8 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -71,6 +71,8 @@ public: bool match(DeviceEnumerator *enumerator); private: + static constexpr unsigned int IPU3_BUF_NUM = 4; + IPU3CameraData *cameraData(const Camera *camera) { return static_cast<IPU3CameraData *>( @@ -102,27 +104,45 @@ std::map<Stream *, StreamConfiguration> PipelineHandlerIPU3::streamConfiguration(Camera *camera, std::vector<Stream *> &streams) { + std::map<unsigned int, std::vector<SizeRange>> formats; std::map<Stream *, StreamConfiguration> configs; IPU3CameraData *data = cameraData(camera); V4L2Subdevice *sensor = data->cio2.sensor; - V4L2SubdeviceFormat format = {}; + StreamConfiguration *config = &configs[&data->stream_]; + + config->pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10; + config->bufferCount = IPU3_BUF_NUM; /* - * FIXME: As of now, return the image format reported by the sensor. - * In future good defaults should be provided for each stream. + * Use the largest image size the sensor provides or + * use a default one. */ - if (sensor->getFormat(0, &format)) { - LOG(IPU3, Error) << "Failed to create stream configurations"; - return configs; + formats = sensor->formats(0); + if (formats.empty()) { + config->width = 1920; + config->height = 1080; + LOG(IPU3, Info) + << "Use default stream sizes " << config->width + << "x" << config->height; } - StreamConfiguration config = {}; - config.width = format.width; - config.height = format.height; - config.pixelFormat = V4L2_PIX_FMT_IPU3_SGRBG10; - config.bufferCount = 4; + auto it = formats.begin(); + while (it != formats.end()) { + for (SizeRange &range : it->second) { + if (range.maxWidth <= config->width || + range.maxHeight <= config->height) + continue; + + config->width = range.maxWidth; + config->height = range.maxHeight; + } + + ++it; + } - configs[&data->stream_] = config; + LOG(IPU3, Info) << "Stream format set to = (" << config->width << "x" + << config->height << ") - 0x" << std::hex + << config->pixelFormat; return configs; }
Inspect all image sizes provided by the sensor and select the biggest one to be returned as default stream configuration instead of returning the currently applied one. Hardcode the stream pixel format to the one produced by the CIO2 unit, to be changed to the one provided by the ImgU. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 44 ++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 12 deletions(-)