[PATCH 0/1] Add StreamRole into StreamConfiguration
Cheng-Hao Yang
chenghaoyang at google.com
Mon Sep 23 16:14:48 CEST 2024
Thanks Jacopo for looking into the tree of the mtkisp7 branch!
On Mon, Sep 23, 2024 at 8:57 PM Jacopo Mondi <jacopo.mondi at ideasonboard.com>
wrote:
> Hi Harvey
>
> On Thu, Sep 19, 2024 at 09:55:58PM GMT, Cheng-Hao Yang wrote:
> > Hi Jacopo and Laurent,
> >
> > On Thu, Sep 19, 2024 at 5:09 PM Jacopo Mondi <
> jacopo.mondi at ideasonboard.com>
> > wrote:
> >
> > > Hi Harvey
> > >
> > > On Wed, Sep 18, 2024 at 04:33:27PM GMT, Cheng-Hao Yang wrote:
> > > > Sorry Jacopo, my bad to miss the first message:
> > > >
> > > > On Mon, Sep 16, 2024 at 8:45 PM Jacopo Mondi <
> > > jacopo.mondi at ideasonboard.com>
> > > > wrote:
> > > >
> > > > > Hi Harvey
> > > > >
> > > > > On Mon, Sep 16, 2024 at 03:51:00PM GMT, Cheng-Hao Yang wrote:
> > > > > > Hi Jacopo,
> > > > > >
> > > > > > On Mon, Sep 16, 2024 at 3:35 PM Jacopo Mondi <
> > > > > jacopo.mondi at ideasonboard.com>
> > > > > > wrote:
> > > > > >
> > > > > > > Hi Harvey
> > > > > > >
> > > > > > > On Mon, Sep 16, 2024 at 04:51:54AM GMT, Harvey Yang wrote:
> > > > > > > > Hi folks,
> > > > > > > >
> > > > > > > > Currently applications set resolutions, pixelFormat,
> bufferCount,
> > > > > etc,
> > > > > > > > into StreamConfigurations, and Pipeline Handler decides which
> > > streams
> > > > > > > > they're assigned to. However, it doesn't allow application to
> > > assign
> > > > > > > > streams that cannot be distinguished by those arguments into
> > > > > > > > VideoRecording or StillCapture (say YUV/NV12 format), which
> is
> > > > > needed in
> > > > > > > > mtkisp7.
> > > > > > >
> > > > > > > Could you explain in a bit more detail why this "is needed"
> and how
> > > > > > > you plan to use StreamRole as part of the stream configuration
> ?
> > > > > > >
> > > > >
> > > > > Could you explain in a bit more detail why this "is needed" and how
> > > > > you plan to use StreamRole as part of the stream configuration ?
> > > > >
> > > > >
> > > > >
> > > > In mtkisp7 pipeline handler, both preview (or video) and still
> capture
> > > > streams support the same format (NV12) and bufferCount, and the
> > > > maximum resolutions are also the same. Therefore, when calling
> > > > `validate()` and `configure()`, mtkisp7 pipeline handler doesn't know
> > > > if applications/Android adapter wants the stream to go through the
> > > > still captur pipeline or not.
> > >
> > > I still have an hard time understanding why validate() and configure()
> > > needs to know the "role". Is this to assign "pipes" to Streams ?
> >
> >
> > If I understand correctly that "pipes" means the pipelines of ISP/IPA
> algos
> > for preview/video/still capture respectively, yes, it's to assign the
> > StreamConfiguration(s) to the desired pipe(s), which means to call
> > `StreamConfiguration::setStream()`. According to the comments [1], it's
> > supposed to be called in `PipelineHandler::configure()`, like
> > SimplePipelineHandler [2]. However, there are also some pipeline handlers
> > that set them in `CameraConfiguration::validate()` [3] [4].
>
> Indeed most of our documentation suggests that setStream() is meant to
> be called during configure(). Considering that validate() is -always-
> called before configure() by Camera, so where exactly you call it, I
> don't think makes a big difference.
>
> We could also relax the documentation.
>
Yes, that's a great idea to avoid confusion.
>
> >
> > [1]:
> >
> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline_handler.cpp#n303
> > [2]:
> >
> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/simple/simple.cpp#n1270
> > [3]:
> >
> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/ipu3/ipu3.cpp#n347
> > [4]:
> >
> https://git.libcamera.org/libcamera/libcamera.git/tree/src/libcamera/pipeline/rkisp1/rkisp1.cpp#n520
> >
> > Does
> > > the mtkisp7 pipes have different capabilities when it comes to
> > > supported formats and resolutions ?
> > >
> >
> > No, as you can see in mtkisp7's implementation [5], all of them support
> the
> > same formats and resolutions.
> >
> > [5]:
> >
> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=556
> >
> >
> >
> >
> > > >
> > > > Does that make sense :P?
> > >
> > > I guess so, but I wonder if the "role" is the main criteria that
> > > should be taken into account when assiging pipes to streams.
> > >
> > How would this work for applications that operates on platforms where
> > > the pipe selection depends on other criteria like the supported image
> > > formats and stream resolutions ? In example, for rkisp1
> > > the "self" pipelines can do RGB while the "main" can't and that's how
> > > we assign pipes during validate().
> >
> >
> > > To be honest this has not proven to be optimal and I'm not opposed to
> > > add Roles to the pipe selection criteria, but they shouldn't be made
> the
> > > only criteria on which pipes are assigned (unless we support this on
> > > all pipelines).
> > >
> > >
> > I agree that the `role` might not be the main criteria in general cases.
> > It's just that we find difficulties in assigning them properly with the
> > current
> > configurations in mtkisp7.
> >
> >
> >
> > > Also I would not based any future-proof design on Roles, they will
> > > likely be heavily reworked or removed going forward.
> > >
> > > As your main use case is Android, I think it would be doable for you
> > > to
> > > 1) Request a generateConfiguration() and keep track of the
> > > StreamConfiguration order.
> > >
> > > generateConfiguration(Viewfinder, Still, Raw)
> > >
> > > will give you StreamConfiguration for the above rols in order as
> > > you have requested them, if supported.
> > >
> > > 2) Code validate() so that you try to assing pipes based on the
> > > formats/sizes and if formats/sizes are the same define an ordering.
> > > In example the first YUV/WxH stream goes to Viewfinder if an
> > > identical YUV/WxH stream is requested for Still Capture.
> > >
> >
> > This actually assumes the application doesn't change the content of the
> > default CameraConfiguration/StreamConfiguration, which I doubt if it's a
> > good thing. Currently in Android adapter, it re-arranges [6]
> > StreamConfigurations based on the previously-fetched default
> > StreamConfigurations of the StreamRoles, which I think is a normal
> > practice of the current interfaces' logic. Please correct me if I'm
> wrong :)
>
> Good point, as Android HAL re-sorts the StreamConfiguration before
> calling Camera::configure() you can't rely on the order in which you
> have requested roles to Camera::generateConfiguration().
>
> >
> > [6]:
> >
> https://git.libcamera.org/libcamera/libcamera.git/tree/src/android/camera_device.cpp#n708
> >
> > Also, if an application calls
> > `generateConfiguration(camera, {StreamRole::VideoRecording})` and
> > `generateConfiguration(camera, {StreamRole::StillCapture})`
> > respectively, and ends up calling `configure()` with `VideoRecording`'s
> > CameraConfiguration, how would the pipeline handler know that the
> > application intends to get buffers from the stream `VideoRecording`?
> > In your suggestion, it seems that the application needs to call
> > `generateConfiguration(camera, {StreamRole::VideoRecording})` again,
> > which I don't think is enforced in the current libcamera API, and might
> > not be a good practice.
> >
> >
> > > 3) As validate() assigns Steams * to StreamConfiguration (with
> > > ::setStream) it should be easy to keep track to which pipe a
> > > StreamConfiguration has been assigned to at configure() time.
> > >
> > > Could you list what are the platform's pipes capabilities ?
> > >
> > >
> > For example, on `ciri` with mtkisp7 pipeline handler [5], `pixelFormat`
> is
> > NV12,
> > supported resolution range is `320x240` to `2592x1944`, and
> `bufferCount` is
> > 8. It applies to `StillCapture`, `VideoRecording`, and `ViewFinder`.
> >
> > [5]:
> >
> https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/libcamera/mtkisp7/src/libcamera/pipeline/mtkisp7/mtkisp7.cpp;l=556
> >
>
> Looking at the pipeline handler implementation from your tree it seems
> to me that you're using roles in validate() to call setStream(), as we
> already clarified:
>
> switch (cfg.role) {
> case StreamRole::Viewfinder:
> case StreamRole::VideoRecording:
> if (videoCnt >= 2) {
> LOG(MtkISP7, Error)
> << "Support only 2 Preview/Video
> streams";
> return Invalid;
> }
> cfg.setStream(const_cast<Stream
> *>(vidStreams[videoCnt++]));
> break;
> case StreamRole::StillCapture:
> if (stillCnt >= 2) {
> LOG(MtkISP7, Error)
> << "Support only 2 StillCapture
> streams";
> return Invalid;
> }
> cfg.setStream(const_cast<Stream
> *>(stillStreams[stillCnt++]));
> break;
> default:
> LOG(MtkISP7, Error) << "Invalid StreamRole " <<
> cfg.role;
> return Invalid;
> }
>
> This shows that you have 4 streams, 2 for Preview/Video and 2 for
> StillCapture.
>
> Then you use the Stream in configure() for populate two sizes
>
> Size video1 = Size{ 0, 0 };
> Size video2 = Size{ 0, 0 };
> Size still1 = Size{ 0, 0 };
> Size still2 = Size{ 0, 0 };
>
> /* Only cover the video resolution */
> for (auto &cfg : *c) {
> if (cfg.stream() == &video1Stream_)
> video1 = cfg.size;
> else if (cfg.stream() == &video2Stream_)
> video2 = cfg.size;
> else if (cfg.stream() == &still1Stream_)
> still1 = cfg.size;
> else if (cfg.stream() == &still2Stream_)
> still2 = cfg.size;
> else
> return -EINVAL;
> }
>
> From there on, all the API you have implemented relies on the presence
> and order of the 'video1', 'video2, 'still1' and 'still2' parameters.
>
> In example
>
> mcnrManager.configure(camsysYuvSize, video1, video2);
> lpnrManager.configure(sensorFullSize_, still1, still2);
> lpnrTunManager.configure(sensorFullSize_, still1, still2);
> mcnrTunManager.configure(camsysYuvSize, video1, video2);
>
> Now, according to what I've read and what you said, there are no
> constraints on the HW that help identify Stream. To make an example,
> you can't assume "Oh this StreamConfiguration is RGB so it needs to go
> to StreamX".
>
Yes, that's the main concern :)
>
> So I guess what you want is to allow an application to say "One
> viewfinder is WxH, one Still capture is WxH, one viewfinder is WxH"
> (also, all Streams are NV12 if I'm not mistaken).
>
> > As said, I'm not opposed to use Roles for assigning pipes, but I don't
> > > think we should based any new development on Roles as there's an high
> > > chance they can be reworked.
> > >
> >
> > Could you briefly describe how the new APIs would be like?
> >
> > Basically I think either applications need to have an argument to
> indicate
> > which kind of stream the StreamConfiguration needs to be assigned to, or
> > the pipeline handler needs to remember which StreamRole a
> > StreamConfiguration returned was asked as the default configuration for.
>
> It's not really about that, the idea is that the stream's
> characteristics should be used to assign a stream to a pipe, like the
> format and the sizes. In your case that's not really possible as all
> streams are NV12 and all resolutions can go anywhere.
>
> But, as you use roles, that logic should live somewhere, doesn't it ?
>
> Looking at your implementation of the Android HAL I see (sorry, cannot
> point out a commit id as there's quite some reverts in the history)
>
> if (isJpegStream(stream)) {
> continue;
> } else if (isYuvSnapshotStream(stream)) {
> streamConfig.streams = { { stream,
> CameraStream::Type::Direct } };
> streamConfig.config.role =
> StreamRole::StillCapture;
> } else if (isPreviewStream(stream)) {
> streamConfig.streams = { { stream,
> CameraStream::Type::Direct } };
> streamConfig.config.role = StreamRole::Viewfinder;
> } else if (isVideoStream(stream)) {
> streamConfig.streams = { { stream,
> CameraStream::Type::Direct } };
> streamConfig.config.role =
> StreamRole::VideoRecording;
> } else {
> streamConfig.streams = { { stream,
> CameraStream::Type::Direct } };
> streamConfig.config.role = StreamRole::Viewfinder;
> }
>
> So you populate roles based on some criteria here, and if I look at
> the criteria
>
> bool isPreviewStream(camera3_stream_t *stream)
> {
> return (GRALLOC_USAGE_HW_COMPOSER & stream->usage);
> }
>
> bool isVideoStream(camera3_stream_t *stream)
> {
> return (GRALLOC_USAGE_HW_VIDEO_ENCODER & stream->usage);
> }
>
> bool isYuvSnapshotStream(camera3_stream_t *stream)
> {
> return (!isVideoStream(stream) && !isPreviewStream(stream) &&
> (HAL_PIXEL_FORMAT_YCbCr_420_888 == stream->format));
> }
>
> they indeed come from Android's requirements that cannot be expressed
> in libcamera.
>
> To be clear, when asking "what you need this for" this is the level of
> detail that is needed to explain your design choices. Otherwise it's
> up to us to decode what you have done in the mtk support branch.
>
I think that's also related to another confusion that I have regarding the
current libcamera API: We use `StreamRole` as the input to get the
default configuration, while it doesn't stop a pipeline handler to assign
it to a different stream, with a different StreamRole, later in `validate()`
or `configure()`. The pipeline handler might not even return
`Status::Adjusted`, if the requested arguments are not changed.
Maybe libcamera core libraries assume each ISP pipe has different
characteristics, like PixelFormat?
>
>
> > It doesn't have to be `StreamRole`. If there are other types/arguments in
> > the new reworked API, I'd be happy to adapt to the new one(s).
> >
> > Two naive ideas:
> > 1. Keep the new `StreamRole role;` to be a const member variable in
> > StreamConfiguration, which should be set in
> > `PipelineHandler::generateConfiguration()`.
> >
> > 2. If it makes sense, add a private class for `StreamConfiguration` and
> keep
> > the new `StreamRole role;` there, so that applications cannot get the
> info.
> > (I don't think we need to hide the info from the applications though...)
> >
> > WDYT?
>
> Given the above described use case is my opinion valid, I wouldn't be
> opposed to have roles as a public member of the StreamConfiguration to
> allow application to hint how a stream should be used if no other
> criteria is applicable.
>
Yeah I also think that the current patch is the simplest.
>
> >
> >
> > > Cc-Laurent for opinions.
>
> As this is an application-facing API change, I would however like to
> see more opinions.
>
Sure, let's wait for more opinions from others :)
>
> Thanks
> j
>
> > >
> > > >
> > > > BR,
> > > > Harvey
> > >
> >
> >
> > --
> > BR,
> > Harvey Yang
>
--
BR,
Harvey Yang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20240923/2e120182/attachment.htm>
More information about the libcamera-devel
mailing list