Implement flushing and improve request tracking.
BUG: https://b/29937783, https://b/32941326
TEST: unit tests pass, fixes 8 failing cts tests
Change-Id: Idc390bdc1e51ba2bdbda91d90acc459a09a5f174
diff --git a/modules/camera/3_4/camera.cpp b/modules/camera/3_4/camera.cpp
index 2dcf1c7..dc9c825 100644
--- a/modules/camera/3_4/camera.cpp
+++ b/modules/camera/3_4/camera.cpp
@@ -163,6 +163,7 @@
}
disconnect();
+ flush();
mBusy = false;
return 0;
}
@@ -393,6 +394,9 @@
int Camera::processCaptureRequest(camera3_capture_request_t *temp_request)
{
int res;
+ // TODO(b/32917568): A capture request submitted or ongoing during a flush
+ // should be returned with an error; for now they are mutually exclusive.
+ android::Mutex::Autolock al(mFlushLock);
ATRACE_CALL();
@@ -460,7 +464,8 @@
void Camera::completeRequest(std::shared_ptr<CaptureRequest> request, int err)
{
if (!mInFlightTracker->Remove(request)) {
- ALOGE("%s:%d: Completed request %p is not being tracked.",
+ ALOGE("%s:%d: Completed request %p is not being tracked. "
+ "It may have been cleared out during a flush.",
__func__, mId, request.get());
return;
}
@@ -498,6 +503,30 @@
sendResult(request);
}
+int Camera::flush()
+{
+ ALOGV("%s:%d: Flushing.", __func__, mId);
+ // TODO(b/32917568): Synchronization. Behave "appropriately"
+ // (i.e. according to camera3.h) if process_capture_request()
+ // is called concurrently with this (in either order).
+ // Since the callback to completeRequest also may happen on a separate
+ // thread, this function should behave nicely concurrently with that too.
+ android::Mutex::Autolock al(mFlushLock);
+
+ std::set<std::shared_ptr<CaptureRequest>> requests;
+ mInFlightTracker->Clear(&requests);
+ for (auto& request : requests) {
+ // TODO(b/31653322): See camera3.h. Should return different error
+ // depending on status of the request.
+ completeRequestWithError(request);
+ }
+
+ ALOGV("%s:%d: Flushed %u requests.", __func__, mId, requests.size());
+
+ // Call down into the device flushing.
+ return flushBuffers();
+}
+
int Camera::preprocessCaptureBuffer(camera3_stream_buffer_t *buffer)
{
int res;
@@ -638,11 +667,9 @@
camdev_to_camera(dev)->dump(fd);
}
-static int flush(const camera3_device_t*)
+static int flush(const camera3_device_t *dev)
{
- // TODO(b/29937783)
- ALOGE("%s: unimplemented.", __func__);
- return -1;
+ return camdev_to_camera(dev)->flush();
}
} // extern "C"
diff --git a/modules/camera/3_4/camera.h b/modules/camera/3_4/camera.h
index f21d33f..e1f7639 100644
--- a/modules/camera/3_4/camera.h
+++ b/modules/camera/3_4/camera.h
@@ -55,7 +55,7 @@
const camera_metadata_t *constructDefaultRequestSettings(int type);
int processCaptureRequest(camera3_capture_request_t *temp_request);
void dump(int fd);
-
+ int flush();
protected:
// Connect to the device: open dev nodes, etc.
@@ -82,6 +82,9 @@
// Enqueue a request to receive data from the camera
virtual int enqueueRequest(
std::shared_ptr<CaptureRequest> request) = 0;
+ // Flush in flight buffers.
+ virtual int flushBuffers() = 0;
+
// Callback for when the device has filled in the requested data.
// Fills in the result struct, validates the data, sends appropriate
@@ -133,6 +136,7 @@
// Lock protecting only static camera characteristics, which may
// be accessed without the camera device open
android::Mutex mStaticInfoLock;
+ android::Mutex mFlushLock;
// Array of handles to streams currently in use by the device
Stream **mStreams;
// Number of streams in mStreams
diff --git a/modules/camera/3_4/request_tracker.cpp b/modules/camera/3_4/request_tracker.cpp
index dcf8263..09f634d 100644
--- a/modules/camera/3_4/request_tracker.cpp
+++ b/modules/camera/3_4/request_tracker.cpp
@@ -70,8 +70,13 @@
}
bool RequestTracker::Remove(std::shared_ptr<CaptureRequest> request) {
+ if (!request) {
+ return false;
+ }
+
// Get the request.
- const auto frame_number_request = frames_in_flight_.find(request->frame_number);
+ const auto frame_number_request =
+ frames_in_flight_.find(request->frame_number);
if (frame_number_request == frames_in_flight_.end()) {
ALOGE("%s: Frame %u is not in flight.", __func__, request->frame_number);
return false;
@@ -105,7 +110,10 @@
// Clear out all tracking.
frames_in_flight_.clear();
- buffers_in_flight_.clear();
+ // Maintain the configuration, but reset counts.
+ for (auto& stream_count : buffers_in_flight_) {
+ stream_count.second = 0;
+ }
}
bool RequestTracker::CanAddRequest(const CaptureRequest& request) const {
diff --git a/modules/camera/3_4/request_tracker_test.cpp b/modules/camera/3_4/request_tracker_test.cpp
index bab6955..a68ff57 100644
--- a/modules/camera/3_4/request_tracker_test.cpp
+++ b/modules/camera/3_4/request_tracker_test.cpp
@@ -230,6 +230,9 @@
dut_->Clear(&actual);
EXPECT_TRUE(dut_->Empty());
EXPECT_EQ(actual, expected);
+
+ // Configuration (max values) should not have been cleared.
+ EXPECT_TRUE(dut_->Add(request1));
}
TEST_F(RequestTrackerTest, ClearRequestsNoResult) {
diff --git a/modules/camera/3_4/v4l2_camera.cpp b/modules/camera/3_4/v4l2_camera.cpp
index 80c2b1f..3383645 100644
--- a/modules/camera/3_4/v4l2_camera.cpp
+++ b/modules/camera/3_4/v4l2_camera.cpp
@@ -121,6 +121,20 @@
// because this camera is no longer in use.
}
+int V4L2Camera::flushBuffers() {
+ HAL_LOG_ENTER();
+
+ int res = device_->StreamOff();
+
+ // This is not strictly necessary, but prevents a buildup of aborted
+ // requests in the in_flight_ map. These should be cleared
+ // whenever the stream is turned off.
+ std::lock_guard<std::mutex> guard(in_flight_lock_);
+ in_flight_.clear();
+
+ return res;
+}
+
int V4L2Camera::initStaticInfo(android::CameraMetadata* out) {
HAL_LOG_ENTER();
@@ -191,12 +205,11 @@
// Assume request validated before calling this function.
// (For now, always exactly 1 output buffer, no inputs).
-
{
std::lock_guard<std::mutex> guard(request_queue_lock_);
request_queue_.push(request);
+ requests_available_.notify_one();
}
- requests_available_.notify_one();
return 0;
}
@@ -235,68 +248,80 @@
return true;
}
- // Actually enqueue the buffer for capture.
- res = device_->EnqueueBuffer(&request->output_buffers[0]);
- if (res) {
- HAL_LOGE("Device failed to enqueue buffer.");
- completeRequest(request, res);
- return true;
- }
-
// Replace the requested settings with a snapshot of
- // the used settings/state immediately after enqueue.
+ // the used settings/state immediately before enqueue.
res = metadata_->FillResultMetadata(&request->settings);
if (res) {
+ // Note: since request is a shared pointer, this may happen if another
+ // thread has already decided to complete the request (e.g. via flushing),
+ // since that locks the metadata (in that case, this failing is fine,
+ // and completeRequest will simply do nothing).
HAL_LOGE("Failed to fill result metadata.");
completeRequest(request, res);
return true;
}
- // Make sure the stream is on (no effect if already on).
- res = device_->StreamOn();
- if (res) {
- HAL_LOGE("Device failed to turn on stream.");
- completeRequest(request, res);
- return true;
- }
-
+ // Actually enqueue the buffer for capture.
{
std::lock_guard<std::mutex> guard(in_flight_lock_);
- in_flight_.push(request);
+
+ uint32_t index;
+ res = device_->EnqueueBuffer(&request->output_buffers[0], &index);
+ if (res) {
+ HAL_LOGE("Device failed to enqueue buffer.");
+ completeRequest(request, res);
+ return true;
+ }
+
+ // Make sure the stream is on (no effect if already on).
+ res = device_->StreamOn();
+ if (res) {
+ HAL_LOGE("Device failed to turn on stream.");
+ // Don't really want to send an error for only the request here,
+ // since this is a full device error.
+ // TODO: Should trigger full flush.
+ return true;
+ }
+
+ // Note: the request should be dequeued/flushed from the device
+ // before removal from in_flight_.
+ in_flight_.emplace(index, request);
+ buffers_in_flight_.notify_one();
}
- buffers_in_flight_.notify_one();
return true;
}
bool V4L2Camera::dequeueRequestBuffers() {
- std::unique_lock<std::mutex> lock(in_flight_lock_);
- while (in_flight_.empty()) {
- buffers_in_flight_.wait(lock);
- }
-
- std::shared_ptr<default_camera_hal::CaptureRequest> request =
- in_flight_.front();
- in_flight_.pop();
-
- lock.unlock();
-
- // Dequeue the buffer. For now, since each request has only 1 buffer,
- // and the in_flight_lock_ is held while enqueueing and dequeuing
- // from the device, just assume that the dequeued buffer corresponds
- // to the dequeued request.
- // TODO(b/31657008): in_flight_ should map buffer handles to requests;
- // this consumer should just dequeue whatever it gets, then match
- // that to a handle.
- v4l2_buffer result_buffer;
- int res = device_->DequeueBuffer(&result_buffer);
+ // Dequeue a buffer.
+ uint32_t result_index;
+ int res = device_->DequeueBuffer(&result_index);
if (res) {
- HAL_LOGE("Device failed to dequeue buffer.");
- completeRequest(request, res);
+ if (res == -EAGAIN) {
+ // EAGAIN just means nothing to dequeue right now.
+ // Wait until something is available before looping again.
+ std::unique_lock<std::mutex> lock(in_flight_lock_);
+ while (in_flight_.empty()) {
+ buffers_in_flight_.wait(lock);
+ }
+ } else {
+ HAL_LOGW("Device failed to dequeue buffer: %d", res);
+ }
return true;
}
- completeRequest(request, 0);
+ // Find the associated request and complete it.
+ std::lock_guard<std::mutex> guard(in_flight_lock_);
+ auto index_request = in_flight_.find(result_index);
+ if (index_request != in_flight_.end()) {
+ completeRequest(index_request->second, 0);
+ in_flight_.erase(index_request);
+ } else {
+ HAL_LOGW(
+ "Dequeued non in-flight buffer index %d. "
+ "This buffer may have been flushed from the HAL but not the device.",
+ index_request->first);
+ }
return true;
}
@@ -411,6 +436,14 @@
// claims it's underdocumented; the implementation lets the HAL overwrite it.
stream->setDataSpace(HAL_DATASPACE_V0_JFIF);
+ std::lock_guard<std::mutex> guard(in_flight_lock_);
+ // The framework should be enforcing this, but doesn't hurt to be safe.
+ if (!in_flight_.empty()) {
+ HAL_LOGE("Can't set device format while frames are in flight.");
+ return -EINVAL;
+ }
+
+ // Ensure the stream is off.
int res = device_->StreamOff();
if (res) {
HAL_LOGE("Device failed to turn off stream for reconfiguration.");
@@ -422,6 +455,7 @@
HAL_LOGE("Failed to set device to correct format for stream.");
return res;
}
+
// Sanity check.
if (*max_buffers < 1) {
HAL_LOGE("Setting format resulted in an invalid maximum of %u buffers.",
diff --git a/modules/camera/3_4/v4l2_camera.h b/modules/camera/3_4/v4l2_camera.h
index 29583b0..3dae649 100644
--- a/modules/camera/3_4/v4l2_camera.h
+++ b/modules/camera/3_4/v4l2_camera.h
@@ -21,6 +21,7 @@
#include <array>
#include <condition_variable>
+#include <map>
#include <queue>
#include <string>
@@ -80,6 +81,8 @@
// Enqueue a request to receive data from the camera.
int enqueueRequest(
std::shared_ptr<default_camera_hal::CaptureRequest> request) override;
+ // Flush in flight buffers.
+ int flushBuffers() override;
// Async request processing helpers.
// Dequeue a request from the waiting queue.
@@ -100,7 +103,9 @@
std::queue<std::shared_ptr<default_camera_hal::CaptureRequest>>
request_queue_;
std::mutex in_flight_lock_;
- std::queue<std::shared_ptr<default_camera_hal::CaptureRequest>> in_flight_;
+ // Maps buffer index : request.
+ std::map<uint32_t, std::shared_ptr<default_camera_hal::CaptureRequest>>
+ in_flight_;
// Threads require holding an Android strong pointer.
android::sp<android::Thread> buffer_enqueuer_;
android::sp<android::Thread> buffer_dequeuer_;
diff --git a/modules/camera/3_4/v4l2_gralloc.cpp b/modules/camera/3_4/v4l2_gralloc.cpp
index 0b093ea..7da3c4e 100644
--- a/modules/camera/3_4/v4l2_gralloc.cpp
+++ b/modules/camera/3_4/v4l2_gralloc.cpp
@@ -321,6 +321,7 @@
// The BufferData entry is always dynamically allocated in lock().
delete entry.second;
}
+ mBufferMap.clear();
// If any unlock failed, return error.
if (failed) {
diff --git a/modules/camera/3_4/v4l2_wrapper.cpp b/modules/camera/3_4/v4l2_wrapper.cpp
index 2f21c94..24a6faf 100644
--- a/modules/camera/3_4/v4l2_wrapper.cpp
+++ b/modules/camera/3_4/v4l2_wrapper.cpp
@@ -68,7 +68,8 @@
return 0;
}
- int fd = TEMP_FAILURE_RETRY(open(device_path_.c_str(), O_RDWR));
+ // Open in nonblocking mode (DQBUF may return EAGAIN).
+ int fd = TEMP_FAILURE_RETRY(open(device_path_.c_str(), O_RDWR | O_NONBLOCK));
if (fd < 0) {
HAL_LOGE("failed to open %s (%s)", device_path_.c_str(), strerror(errno));
return -ENODEV;
@@ -155,6 +156,10 @@
int res = IoctlLocked(VIDIOC_STREAMOFF, &type);
// Calling STREAMOFF releases all queued buffers back to the user.
int gralloc_res = gralloc_->unlockAllBuffers();
+ // No buffers in flight.
+ for (size_t i = 0; i < buffers_.size(); ++i) {
+ buffers_[i] = false;
+ }
if (res < 0) {
HAL_LOGE("STREAMOFF fails: %s", strerror(errno));
return -ENODEV;
@@ -350,9 +355,7 @@
// Continuous/Step-wise: based on the stepwise struct returned by the query.
// Fully listing all possible sizes, with large enough range/small enough
// step size, may produce far too many potential sizes. Instead, find the
- // closest to a set of standard sizes plus largest possible.
- sizes->insert({{{static_cast<int32_t>(size_query.stepwise.max_width),
- static_cast<int32_t>(size_query.stepwise.max_height)}}});
+ // closest to a set of standard sizes.
for (const auto size : kStandardSizes) {
// Find the closest size, rounding up.
uint32_t desired_width = size[0];
@@ -453,11 +456,26 @@
return 0;
}
- // Not in the correct format, set our format.
+ // Not in the correct format, set the new one.
+
+ if (format_) {
+ // If we had an old format, first request 0 buffers to inform the device
+ // we're no longer using any previously "allocated" buffers from the old
+ // format. This seems like it shouldn't be necessary for USERPTR memory,
+ // and/or should happen from turning the stream off, but the driver
+ // complained. May be a driver issue, or may be intended behavior.
+ int res = RequestBuffers(0);
+ if (res) {
+ return res;
+ }
+ }
+
+ // Set the camera to the new format.
v4l2_format new_format;
desired_format.FillFormatRequest(&new_format);
// TODO(b/29334616): When async, this will need to check if the stream
// is on, and if so, lock it off while setting format.
+
if (IoctlLocked(VIDIOC_S_FMT, &new_format) < 0) {
HAL_LOGE("S_FMT failed: %s", strerror(errno));
return -ENODEV;
@@ -472,31 +490,22 @@
// Keep track of our new format.
format_.reset(new StreamFormat(new_format));
- // Format changed, setup new buffers.
- int res = SetupBuffers();
+ // Format changed, request new buffers.
+ int res = RequestBuffers(1);
if (res) {
- HAL_LOGE("Failed to set up buffers for new format.");
+ HAL_LOGE("Requesting buffers for new format failed.");
return res;
}
*result_max_buffers = buffers_.size();
return 0;
}
-int V4L2Wrapper::SetupBuffers() {
- HAL_LOG_ENTER();
-
- if (!format_) {
- HAL_LOGE("Stream format must be set before setting up buffers.");
- return -ENODEV;
- }
-
- // "Request" a buffer (since we're using a userspace buffer, this just
- // tells V4L2 to switch into userspace buffer mode).
+int V4L2Wrapper::RequestBuffers(uint32_t num_requested) {
v4l2_requestbuffers req_buffers;
memset(&req_buffers, 0, sizeof(req_buffers));
req_buffers.type = format_->type();
req_buffers.memory = V4L2_MEMORY_USERPTR;
- req_buffers.count = 1;
+ req_buffers.count = num_requested;
int res = IoctlLocked(VIDIOC_REQBUFS, &req_buffers);
// Calling REQBUFS releases all queued buffers back to the user.
@@ -511,7 +520,7 @@
}
// V4L2 will set req_buffers.count to a number of buffers it can handle.
- if (req_buffers.count < 1) {
+ if (num_requested > 0 && req_buffers.count < 1) {
HAL_LOGE("REQBUFS claims it can't handle any buffers.");
return -ENODEV;
}
@@ -520,7 +529,8 @@
return 0;
}
-int V4L2Wrapper::EnqueueBuffer(const camera3_stream_buffer_t* camera_buffer) {
+int V4L2Wrapper::EnqueueBuffer(const camera3_stream_buffer_t* camera_buffer,
+ uint32_t* enqueued_index) {
if (!format_) {
HAL_LOGE("Stream format must be set before enqueuing buffers.");
return -ENODEV;
@@ -576,36 +586,52 @@
std::lock_guard<std::mutex> guard(buffer_queue_lock_);
buffers_[index] = true;
+ if (enqueued_index) {
+ *enqueued_index = index;
+ }
return 0;
}
-int V4L2Wrapper::DequeueBuffer(v4l2_buffer* buffer) {
+int V4L2Wrapper::DequeueBuffer(uint32_t* dequeued_index) {
if (!format_) {
- HAL_LOGE("Stream format must be set before dequeueing buffers.");
- return -ENODEV;
+ HAL_LOGV(
+ "Format not set, so stream can't be on, "
+ "so no buffers available for dequeueing");
+ return -EAGAIN;
}
- memset(buffer, 0, sizeof(*buffer));
- buffer->type = format_->type();
- buffer->memory = V4L2_MEMORY_USERPTR;
- if (IoctlLocked(VIDIOC_DQBUF, buffer) < 0) {
- HAL_LOGE("DQBUF fails: %s", strerror(errno));
- return -ENODEV;
+ v4l2_buffer buffer;
+ memset(&buffer, 0, sizeof(buffer));
+ buffer.type = format_->type();
+ buffer.memory = V4L2_MEMORY_USERPTR;
+ int res = IoctlLocked(VIDIOC_DQBUF, &buffer);
+ if (res) {
+ if (errno == EAGAIN) {
+ // Expected failure.
+ return -EAGAIN;
+ } else {
+ // Unexpected failure.
+ HAL_LOGE("DQBUF fails: %s", strerror(errno));
+ return -ENODEV;
+ }
}
// Mark the buffer as no longer in flight.
{
std::lock_guard<std::mutex> guard(buffer_queue_lock_);
- buffers_[buffer->index] = false;
+ buffers_[buffer.index] = false;
}
// Now that we're done painting the buffer, we can unlock it.
- int res = gralloc_->unlock(buffer);
+ res = gralloc_->unlock(&buffer);
if (res) {
HAL_LOGE("Gralloc failed to unlock buffer after dequeueing.");
return res;
}
+ if (dequeued_index) {
+ *dequeued_index = buffer.index;
+ }
return 0;
}
diff --git a/modules/camera/3_4/v4l2_wrapper.h b/modules/camera/3_4/v4l2_wrapper.h
index f25f80e..8c10d4b 100644
--- a/modules/camera/3_4/v4l2_wrapper.h
+++ b/modules/camera/3_4/v4l2_wrapper.h
@@ -45,8 +45,9 @@
Connection(std::shared_ptr<V4L2Wrapper> device)
: device_(std::move(device)), connect_result_(device_->Connect()) {}
~Connection() {
- if (connect_result_ == 0)
+ if (connect_result_ == 0) {
device_->Disconnect();
+ }
}
// Check whether the connection succeeded or not.
inline int status() const { return connect_result_; }
@@ -77,8 +78,9 @@
virtual int SetFormat(const default_camera_hal::Stream& stream,
uint32_t* result_max_buffers);
// Manage buffers.
- virtual int EnqueueBuffer(const camera3_stream_buffer_t* camera_buffer);
- virtual int DequeueBuffer(v4l2_buffer* buffer);
+ virtual int EnqueueBuffer(const camera3_stream_buffer_t* camera_buffer,
+ uint32_t* enqueued_index = nullptr);
+ virtual int DequeueBuffer(uint32_t* dequeued_index = nullptr);
private:
// Constructor is private to allow failing on bad input.
@@ -93,8 +95,8 @@
// Perform an ioctl call in a thread-safe fashion.
template <typename T>
int IoctlLocked(int request, T data);
- // Adjust buffers any time a device is connected/reformatted.
- int SetupBuffers();
+ // Request/release userspace buffer mode via VIDIOC_REQBUFS.
+ int RequestBuffers(uint32_t num_buffers);
inline bool connected() { return device_fd_.get() >= 0; }
diff --git a/modules/camera/3_4/v4l2_wrapper_mock.h b/modules/camera/3_4/v4l2_wrapper_mock.h
index c717d32..5f8fc4a 100644
--- a/modules/camera/3_4/v4l2_wrapper_mock.h
+++ b/modules/camera/3_4/v4l2_wrapper_mock.h
@@ -45,9 +45,10 @@
MOCK_METHOD2(SetFormat,
int(const default_camera_hal::Stream& stream,
uint32_t* result_max_buffers));
- MOCK_METHOD1(EnqueueBuffer,
- int(const camera3_stream_buffer_t* camera_buffer));
- MOCK_METHOD1(DequeueBuffer, int(v4l2_buffer* buffer));
+ MOCK_METHOD2(EnqueueBuffer,
+ int(const camera3_stream_buffer_t* camera_buffer,
+ uint32_t* enqueued_index));
+ MOCK_METHOD1(DequeueBuffer, int(uint32_t* dequeued_index));
};
} // namespace v4l2_camera_hal