Kieran Bingham authored 4 years ago and Nícolas F. R. A. Prado committed 4 years ago

Report the function which fails an assertion as well as the actual
assertion.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Kieran Bingham authored 4 years ago and Nícolas F. R. A. Prado committed 4 years ago

Request objects are created and owned by the application, but are of
course utilised widely throughout the internals of libcamera.

If the application free's the requests while they are still active
within libcamera a use after free will occur. While this can be trapped
by tools such as valgrind, given the importance of this object and the
relationship of external ownership, it may have some value to provide
Debug build (disabled at Release build) assertions on the condition of
these objects.

Make sure the request fails an assertion immediately if used after free.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

---
I've removed the RFC from this, and I actually even more so believe this
is a good facility to provide on the Request object.

Mostly because the Request object is the only object which is given to
libcamera from the application (yes, created by libcamera, but that's
separate), and is expected to be free'd by the application.

If an application free's requests while they are still in use within
libcamera, the symptoms can be distinctly misleading and lead to rabbit
holes.

Therefore, I think the Request object is the one place where extra
safety checking (in debug builds) is worth while.

Yes, of course if an application free's a request while it's in use with
libcamera - then it's the applications fault, not ours - but because of
the nature of requests, this could be an easy trap to fall into - and I
don't want to find that reported as bugs in libcamera.

Kieran Bingham authored 4 years ago and Nícolas F. R. A. Prado committed 4 years ago

When the camera manager calls stop on a pipeline, it is expected that
the pipeline handler guarantees all requests are returned back to the
application before the camera has stopped.

Ensure that this guarantee is met by providing an accessor on the
pipeline handler to validate that all pending requests are removed.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

RkISP1 pipeline should be fixed to support raw?

Niklas Söderlund authored 4 years ago and Nícolas F. R. A. Prado committed 4 years ago

Add a test which stops a camera while requests are still queued. This
intends to test cleanup paths where requests are dequeued from video
devices in an uncompleted state.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Niklas Söderlund authored 4 years ago and Nícolas F. R. A. Prado committed 4 years ago

Add a compliance tool to ease testing of cameras. In contrast to the
unit-tests under test/ that aims to test the internal components of
libcamera the compliance tool aims to test application use-cases and to
some extent the public API.

This change adds the boilerplate code of a simple framework for the
creation of tests. The tests aim both to demonstrate the tool and to
catch real problems. The tests added are:

 - Test that if one queues exactly N requests to a camera exactly N
   requests are eventually completed.

 - Test that a configured camera can be started and stopped multiple
   times in an attempt to exercise cleanup code paths otherwise not
   often tested with 'cam' for example.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Acked-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Niklas Söderlund authored 4 years ago and Nícolas F. R. A. Prado committed 4 years ago

The libevent library will be needed by both cam and the soon to be added
lc-compliacne tool. Move the dependency lookup to a shared meson file to
avoid having to look it up twice.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

While installing meson using pip3 install --user meson, due to python
path issues, build.ninja can't be located by ninja. meson generates
ninja files fine, but then when you run ninja, it is unable to find
meson's build.ninja python module due to path issues.
It gives the following error on ninja -C build install:

ninja: Entering directory `build'
ninja: error: loading 'build.ninja': No such file or directory

After uninstalling meson using pip3 and installing it again using pip
without --user argument solved the issue.

Add a troubleshooting section to the readme to describe this issue and
suggest possible solutions.

Signed-off-by: Vedant Paranjape <vedantparanjape160201@gmail.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

Hirokazu Honda authored 4 years ago and Laurent Pinchart committed 4 years ago

libcamera doesn't handle crop_rotate_scale_degrees. Therefore,
if it is requested, that is, crop_rotate_scale_degrees is not
CAMERA3_STREAM_ROTATION_0, the configuration should fail.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Hirokazu Honda authored 4 years ago and Laurent Pinchart committed 4 years ago

|rotation| and |crop_rotate_scale_degrees| are important info of
a configuration. They should be logged.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Hirokazu Honda authored 4 years ago and Laurent Pinchart committed 4 years ago

libcamera doesn't handle |crop_rotate_scale_degrees| in
camera3_stream at all. This adds the validation of the requested
|crop_rotate_scale_degrees| in configuration, but still not
handle the specified values.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Hirokazu Honda authored 4 years ago and Laurent Pinchart committed 4 years ago

Android Camera HAL 3 API used in ChromeOS has a ChromeOS own
extension, for example, crop_rotate_scale_degrees in
camera3_stream. As those extensions are not available on Android
platforms, introduce a OS_CHROMEOS macro that can be used to
compile CrOS-specific code conditionally. The macro is defined
if and only if android_platform is 'cros'.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Commit 7d787983 ("android: mm: cros: Handle buffer registration
failure") mistakenly tried to initialize the CameraBuffer::Private
registered member variable instead of registered_. This reults in a
compilation failure. Fix it.

Fixes: 7d787983 ("android: mm: cros: Handle buffer registration failure")
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Hirokazu Honda authored 4 years ago and Laurent Pinchart committed 4 years ago

This adds more validation to camera3_capture_request mainly
about buffer_handle values.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Hirokazu Honda authored 4 years ago and Laurent Pinchart committed 4 years ago

CameraDevice::processCaptureRequest() checks the validity of a
provided camera3_capture_request. This factorizes the code in
order to add more validation to the request later.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Hirokazu Honda authored 4 years ago and Laurent Pinchart committed 4 years ago

This checks if the number of streams is zero on configuration
and then returns -EINVAL.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Hirokazu Honda authored 4 years ago and Laurent Pinchart committed 4 years ago

cros::CameraBufferManager::Register() fails if a buffer handle
is invalid. We should mark CameraBuffer as invalid on the failure
of Register().

While the cros::CameraBufferManager Unlock() and Deregister() functions
should be able to handle buffers that haven't been locked and
registered, this isn't an API guarantee, and errors will be logged.
Avoid this by skipping unlocking and unregistration of buffers that
haven't been locked or registered.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Hirokazu Honda authored 4 years ago and Laurent Pinchart committed 4 years ago

PipelineHandler::queueRequest() is asynchronously invoked in
Camera::queueRequest(). Therefore the return value of
PipelineHandler::queueRequest() is useless. This changes the
function to a void function.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Pull the "getting started" of README.rst in the documentation tree, as
done on libcamera.org, to ensure documentation build coverage of the
whole README.rst.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Phi-Bang Nguyen authored 4 years ago and Laurent Pinchart committed 4 years ago

The ScalerCrop control does not contain the null check which can cause
the camera HAL crash at boot. Fix it.

Fixes: 31a1a628 ("android: camera_device: Register MAX_DIGITAL_ZOOM")
Signed-off-by: Phi-Bang Nguyen <pnguyen@baylibre.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Custom event dispatchers for threads was an API meant to provide a way
to integrate libcamera in the application's event loop. This isn't used
anymore, as libcamera now creates internal threads. Drop the unused
Thread::setEventDispatcher() function, and update the documentation
accordingly.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Sebastian Fricke authored 4 years ago and Laurent Pinchart committed 4 years ago

s/Return the validated limits out though metadata./
  Return the validated limits via metadata./

Signed-off-by: Sebastian Fricke <sebastian.fricke@posteo.net>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Sebastian Fricke authored 4 years ago and Laurent Pinchart committed 4 years ago

The comment states, that we get the device format to pass it to the IPA,
but the variable `sensorFormat` is not used again after it's assignment.
Remove it, together with the comment.

Signed-off-by: Sebastian Fricke <sebastian.fricke@posteo.net>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

For all frame indexes, use the same sequence number as generated
by the Request object.

This allows clear matching of what operations occurred to which request.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

When the camera is being stop()ped, active requests will complete. These
may trigger an application to re-queue those requests to the camera but
that is not permitted, and is an error in the application.

Extend the camera state to include a stopping state which is entered as
soon as a call to stop() is made. At this point, any request queued will
be rejected with a warning, while any pending requests are either
successfully completed or cancelled.

When the pipeline handler has finished stopping, the camera state will
transition to the CameraConfigured state where it can begin to accept
requests again, and be restarted.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

The camera object has a state machine to ensure calls are only made
when in the correct state. It isn't easy to identify where things happen
when assertions fail so add extra information to make this clearer.

The error level of the isAccessAllowed is raised from Debug to Error as
this is important information for applications to know if they have made
a request in an invalid state.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

All requests must have completed before the Camera has fully stopped.

Requests completing when the camera is not running represent an internal
pipeline handler bug.

Trap this event with a fatal error.

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Provide a toString helper to assist in printing Request state
for debug and logging contexts.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Provide a sequence number on Requests which are added by the pipeline
handler.

Each pipeline handler keeps a requestSequence per CameraData and
increments everytime a request is queued on that camera.

The sequence number is associated with the Request and can be utilised
for assisting with debugging, and printing the queueing sequence of in
flight requests.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

The FrameBuffer class is only friends with Request so that the request
can be associated with the buffers.

FrameBuffer already has a helper to setRequest(), so let's use that
directly instead.

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Asynchronous tasks can only be submitted while the IPA is running.

Further more, the shutdown sequence can not be tracked with a simple
running flag. We can also be in the state 'Stopping' where we have not
yet completed all events, but we must not commence anything new.

Refactor the running_ boolean into a stateful enum to track this.

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Fix two simple typos.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Sebastian Fricke <sebastian.fricke@posteo.net>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

Khem Raj authored 4 years ago and Laurent Pinchart committed 4 years ago

A range-based for loop whose range expression is an array of char
pointers and range variable declaration is a const reference to a
std::string creates a temporary string from the char pointer and binds
the range variable reference to it. This creates a const reference to a
temporary, which is valid in C++, and extends the lifetime of the
temporary to the lifetime of the reference.

However, lifetime extension in range-based for loops is considered as a
sign of a potential issue, as a temporary is created for every
iteration, which can be costly, and the usage of a reference in the
range declaration doesn't make it obvious that the code isn't simply
binding a reference to an existing object. gcc 11, with the
-Wrange-loop-construct option, flags this:

uvcvideo.cpp:432:33: error: loop variable 'name' of type 'const string&' {aka 'const std::__cxx11::basic_string<cha
r>&'} binds to a temporary constructed from type 'const char* const' [-Werror=range-loop-construct]
|   432 |         for (const std::string &name : { "idVendor", "idProduct" }) {
|       |                                 ^~~~

To please the compiler, make the range variable a const char *. This may
bring a tiny performance improvement, as the name is only used once, in
a location where the compiler can use

	operator+(const std::string &, const char *)

instead of

	operator+(const std::string &, const std::string &)

Signed-off-by: Khem Raj <raj.khem@gmail.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Use a const char * type instead of auto, and update the commit message
accordingly.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>