[libcamera-devel] [PATCH] gstreamer: Fix usage of default size for fixation
Nicolas Dufresne
nicolas.dufresne at collabora.com
Thu Aug 26 16:34:54 CEST 2021
Le jeudi 26 août 2021 à 15:31 +0100, Kieran Bingham a écrit :
>
> On 26/08/2021 15:04, Nicolas Dufresne wrote:
> > Le jeudi 26 août 2021 à 14:44 +0100, Kieran Bingham a écrit :
> > > On 25/08/2021 16:07, Nicolas Dufresne wrote:
> > > > From: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > > >
> > > > Even thought this is not formaly documented, all pipeline managers sets a
> > >
> > > s/thought/though/
> > >
> > > s/formaly/formally/
> > >
> > > > default value to StreamConfiguration::size. The original fixation code was
> > >
> > > This is documented at by the function Camera::generateConfiguration()
> > >
> > > https://libcamera.org/api-html/classlibcamera_1_1Camera.html#a25c80eb7fc9b1cf32692ce0c7f09991d
> > >
> > > I suspect the real issue (of documentation) is that - we have lots of
> > > documentation, but it's hard to find the parts that are needed at the
> > > time they are needed.
> > >
> > >
> > > > attempting to use it, but as it was truncating the caps to its first structure
> > > > it would never actually find a best match.
> > > >
> > > > In this patch, instead of truncating, we weight various matches using the
> > > > product of the width and height delta. We also split delta from ranges
> > > > appart and prefer fixed size over them as ranges are not reliable.
> > >
> > > s/appart/apart/
> > >
> > >
> > > > This patch also removes the related todo, as it seems that libcamera core won't
> > > > go further then providing this default value and won't be sorting the format and
> > > > size lists.
> > >
> > > The formats and sizes might be sorted numerically ... but there's no
> > > indication of preference there indeed. They don't even really mean that
> > > the frame size is supported by the device (though the pixel format is a
> > > bit more indicative).
> > >
> > >
> > > Maybe the Sizes and Formats shouldn't be used at all by gstreamer, it
> > > might be that they are providing information which is seen as more
> > > expressive than it really is, and gstreamer should do it's own
> > > try/validate negotiation phase to determine the possibilities.
> > >
> > >
> > >
> > > > Signed-off-by: Nicolas Dufresne <nicolas.dufresne at collabora.com>
> > > > ---
> > > > src/gstreamer/gstlibcamera-utils.cpp | 51 +++++++++++++++++++++++++---
> > > > src/gstreamer/gstlibcamerasrc.cpp | 4 ---
> > > > 2 files changed, 47 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/src/gstreamer/gstlibcamera-utils.cpp b/src/gstreamer/gstlibcamera-utils.cpp
> > > > index 61370d5f..007d6a64 100644
> > > > --- a/src/gstreamer/gstlibcamera-utils.cpp
> > > > +++ b/src/gstreamer/gstlibcamera-utils.cpp
> > > > @@ -136,14 +136,57 @@ gst_libcamera_configure_stream_from_caps(StreamConfiguration &stream_cfg,
> > > > GstCaps *caps)
> > > > {
> > > > GstVideoFormat gst_format = pixel_format_to_gst_format(stream_cfg.pixelFormat);
> > > > + guint i;
> > > > + gint best_fixed = -1, best_in_range = -1;
> > > > + GstStructure *s;
> > > > +
> > > > + /*
> > > > + * These are delta weight computed from:
> > > > + * ABS(width - stream_cfg.size.width) * ABS(height - stream_cfg.size.height)
> > > > + */
> > > > + guint best_fixed_delta = G_MAXUINT;
> > > > + guint best_in_range_delta = G_MAXUINT;
> > > >
> > > > /* First fixate the caps using default configuration value. */
> > > > g_assert(gst_caps_is_writable(caps));
> > > > - caps = gst_caps_truncate(caps);
> > > > - GstStructure *s = gst_caps_get_structure(caps, 0);
> > > >
> > > > - gst_structure_fixate_field_nearest_int(s, "width", stream_cfg.size.width);
> > > > - gst_structure_fixate_field_nearest_int(s, "height", stream_cfg.size.height);
> > > > + /* Lookup the structure for a close match to the stream_cfg.size */
> > >
> > > I'm worried that this is placing too much weight on the caps which have
> > > been generated from the StreamFormats.
> > >
> > > https://libcamera.org/api-html/classlibcamera_1_1StreamFormats.html
> > > states:
> > >
> > > > All sizes retrieved from StreamFormats shall be treated as advisory
> > > > and no size shall be considered to be supported until it has been
> > > > verified using CameraConfiguration::validate().
> > >
> > > If a role has been passed to the generateConfiguration() [0], then the
> > > Streams (and therefore their StreamConfiguration) are provided as
> > > suggested default configurations (which should have already been run
> > > through validation).
> >
> > Feel free to edit this part out, or ask for a V2. What "other project" (of
> > course I mean gst here) do is that we add links with "Refer to ...". In that
> > specific case, instead of leaving StreamConfiguration::size and
> > StreamConfiguration::format blank, or duplicating the doc, it could be extended
> > with a link that refers to were these are documented.
>
> I don't think size and format need to reference elsewhere, they /are/
> the size and format of that StreamConfiguration.
Well they have a very special semantic, as before being set and validate, they
meant to be a "suggested default value". I also believe that me being tricked
thinking this was undocumented is enough to justify improving it.
>
> The StreamConfiguration (as a whole) is the object which is returned,
> and it 'is' the default suggested configuration.
>
> The given default StreamConfiguration has a size and a format ...
>
> There's nothing special about 'size' and 'format' of the
> StreamConfiguration - it's the object as a whole that we're discussing here.
>
>
> Perhaps a better way to describe it is
>
> "Pipeline handlers will return a default StreamConfiguration for a given
> StreamRole. That StreamConfiguration comprises of a validated Size and
> Format which can be used to start the device".
>
>
> I think what is confusing is that a StreamConfiguration has this 'extra'
> thing called formats, and sizes. They are /not/ the active
> StreamConfiguration - but they are possible values (hints) that can be
> put back into the StreamConfiguration to try validating.
>
>
>
> > > [0]
> > > https://libcamera.org/api-html/classlibcamera_1_1Camera.html#a25c80eb7fc9b1cf32692ce0c7f09991d
> > >
> > >
> > > However, having discussed this with you last night, I think the code
> > > here is correct, as it matches against the caps of the whole pipeline,
> > > but we might want to have a validation phase either during
> > > gst_libcamera_stream_formats_to_caps() or after it to further
> > > filter/restrict what is actually added as supported formats.
> > >
> > > I've already seen that this is reporting formats to the caps, that are
> > > not actually available on the IPU3, and so its' failing to correctly
> > > configure the pipeline as expected.
> > >
> > > But I think that's a separate issue on top... Or I wonder if we could
> > > handle that better at the libcamera layer ...
> >
> > What I was saying is that you don't want to fix this theoretically, as you'll
> > never see the end of it. At the moment, libcamera provides format/size(fixed)
> > pairs and then unreliable ranges.
> >
> > What new sine I wrote that gst code, is that we generate unreliable fixed sizes
> > from ranges now. I suggest to check IPU code, that might be one of these that
> > endup being picked and failed.
> >
> > I was expected that using the default size would greatly help avoid bad pick,
> > since, can you check if you have managed to hit that default but you had a
> > format miss-match ? If that was the case instead of a "unreliable" generated
> > fixed size, we could make improve it short term by adding preferred format
> > around this.
>
>
> I think the default size was picked - but the width * width bug gave a
> different default, that's identified and fixed now though ...
>
> What does still happen is that the 'arbitrary unvalidated list' is
> exposed to cheese, so it lets me pick one of those sizes.
>
> I select it, and it seems to fail validation - and that causes the
> gstreamer pipeline to stop with:
>
> (cheese:93188): cheese-WARNING **: 15:29:24.060: Internal data stream
> error.: ../../src/gstreamer/gstlibcamerasrc.cpp(323):
> gst_libcamera_src_task_run ():
> /GstCameraBin:camerabin/GstWrapperCameraBinSrc:camera_source/GstBin:bin19/GstLibcameraSrc:libcamerasrc2:
> streaming stopped, reason not-negotiated (-4)
>
>
> But it doesn't resume when changing back to a supported size.
>
> Anyway, I don't think that's an issue caused by this patch, and this
> patch already brings improvements, so I'm going to move to the v2 patch
> that you've posted ;-)
>
>
> > >
> > >
> > > > + for (i = 0; i < gst_caps_get_size(caps); i++) {
> > > > + s = gst_caps_get_structure(caps, i);
> > > > + gint width, height;
> > > > + guint delta;
> > > > +
> > > > + if (gst_structure_has_field_typed(s, "width", G_TYPE_INT) &&
> > > > + gst_structure_has_field_typed(s, "height", G_TYPE_INT)) {
> > > > + gst_structure_get_int(s, "width", &width);
> > > > + gst_structure_get_int(s, "width", &height);
> > >
> > > s/width/height/ here, looks like your setting width * width.
> >
> > Interesting, indeed, will fix.
> >
> > >
> > > In fact, that might explain one of the bugs I hit with this...
> > >
> > > The IPU3 was ending up using an odd default size which was possible
> > > based on the best match of 720x720 now I see this ...
> > >
> > > (I'll test again later).
> > >
> > >
> > > Anyway, with s/width/height/ fixed above here:
> > >
> > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > >
> > >
> > >
> > > > +
> > > > + delta = ABS(width - (gint)stream_cfg.size.width) * ABS(height - (gint)stream_cfg.size.height);
> > > > +
> > > > + if (delta < best_fixed_delta) {
> > > > + best_fixed_delta = delta;
> > > > + best_fixed = i;
> > > > + }
> > > > + } else {
> > > > + gst_structure_fixate_field_nearest_int(s, "width", stream_cfg.size.width);
> > > > + gst_structure_fixate_field_nearest_int(s, "height", stream_cfg.size.height);
> > > > + gst_structure_get_int(s, "width", &width);
> > > > + gst_structure_get_int(s, "width", &height);
> > > > +
> > > > + delta = ABS(width - (gint)stream_cfg.size.width) * ABS(height - (gint)stream_cfg.size.height);
> > > > +
> > > > + if (delta < best_in_range_delta) {
> > > > + best_in_range_delta = delta;
> > > > + best_in_range = i;
> > > > + }
> > > > + }
> > > > + }
> > > > +
> > > > + /* Prefer reliable fixed value over ranges */
> > > > + if (best_fixed >= 0)
> > > > + s = gst_caps_get_structure(caps, best_fixed);
> > > > + else
> > > > + s = gst_caps_get_structure(caps, best_in_range);
> > > >
> > > > if (gst_structure_has_name(s, "video/x-raw")) {
> > > > const gchar *format = gst_video_format_to_string(gst_format);
> > > > diff --git a/src/gstreamer/gstlibcamerasrc.cpp b/src/gstreamer/gstlibcamerasrc.cpp
> > > > index b243aeb3..4c7b36ae 100644
> > > > --- a/src/gstreamer/gstlibcamerasrc.cpp
> > > > +++ b/src/gstreamer/gstlibcamerasrc.cpp
> > > > @@ -25,10 +25,6 @@
> > > > * - Add timestamp support
> > > > * - Use unique names to select the camera devices
> > > > * - Add GstVideoMeta support (strides and offsets)
> > > > - *
> > > > - * \todo libcamera UVC drivers picks the lowest possible resolution first, this
> > > > - * should be fixed so that we get a decent resolution and framerate for the
> > > > - * role by default.
> > > > */
> > > >
> > > > #include "gstlibcamerasrc.h"
> > > >
> >
> >
More information about the libcamera-devel
mailing list