Track in-flight requests
This allows for verification of callbacks, and will also be helpful
when implementing flush.
BUG: 31653306
TEST: unit tests pass, test program runs
Change-Id: Id43d6cb3c2b9ca4adc96fc08282f88e0b7b904e1
diff --git a/modules/camera/3_4/camera.cpp b/modules/camera/3_4/camera.cpp
index 04d4c89..d4be03d 100644
--- a/modules/camera/3_4/camera.cpp
+++ b/modules/camera/3_4/camera.cpp
@@ -58,7 +58,8 @@
mBusy(false),
mCallbackOps(NULL),
mStreams(NULL),
- mNumStreams(0)
+ mNumStreams(0),
+ mInFlightTracker(new RequestTracker)
{
memset(&mTemplates, 0, sizeof(mTemplates));
memset(&mDevice, 0, sizeof(mDevice));
@@ -249,6 +250,9 @@
mStreams = newStreams;
mNumStreams = stream_config->num_streams;
+ // Update the request tracker.
+ mInFlightTracker->SetStreamConfiguration(*stream_config);
+
return 0;
err_out:
@@ -256,7 +260,10 @@
destroyStreams(newStreams, stream_config->num_streams);
// Set error if it wasn't specified.
if (!res) {
- res = -EINVAL;
+ res = -EINVAL;
+ } else if (res != -EINVAL) {
+ // Fatal error, clear stream configuration.
+ mInFlightTracker->ClearStreamConfiguration();
}
return res;
}
@@ -437,27 +444,40 @@
return -ENODEV;
}
+ // Add the request to tracking.
+ if (!mInFlightTracker->Add(request)) {
+ ALOGE("%s:%d: Failed to track request for frame %d.",
+ __func__, mId, request->frame_number);
+ return -ENODEV;
+ }
+
// Send the request off to the device for completion.
enqueueRequest(request);
// Request is now in flight. The device will call completeRequest
// asynchronously when it is done filling buffers and metadata.
- // TODO(b/31653306): Track requests in flight to ensure not too many are
- // sent at a time, and so they can be dumped even if the device loses them.
return 0;
}
void Camera::completeRequest(std::shared_ptr<CaptureRequest> request, int err)
{
- // TODO(b/31653306): make sure this is actually a request in flight,
- // and not a random new one or a cancelled one. If so, stop tracking.
- if (err) {
- ALOGE("%s:%d: Error completing request for frame %d.",
- __func__, mId, request->frame_number);
- completeRequestWithError(request);
+ if (!mInFlightTracker->Remove(request)) {
+ ALOGE("%s:%d: Completed request %p is not being tracked.",
+ __func__, mId, request.get());
return;
}
+ // Since |request| has been removed from the tracking, this method
+ // MUST call sendResult (can still return a result in an error state, e.g.
+ // through completeRequestWithError) so the frame doesn't get lost.
+
+ if (err) {
+ ALOGE("%s:%d: Error completing request for frame %d.",
+ __func__, mId, request->frame_number);
+ completeRequestWithError(request);
+ return;
+ }
+
// Notify the framework with the shutter time (extracted from the result).
int64_t timestamp = 0;
// TODO(b/31360070): The general metadata methods should be part of the
diff --git a/modules/camera/3_4/camera.h b/modules/camera/3_4/camera.h
index 255156f..f21d33f 100644
--- a/modules/camera/3_4/camera.h
+++ b/modules/camera/3_4/camera.h
@@ -26,6 +26,7 @@
#include "capture_request.h"
#include "metadata/metadata.h"
+#include "request_tracker.h"
#include "stream.h"
namespace default_camera_hal {
@@ -138,6 +139,8 @@
int mNumStreams;
// Standard camera settings templates
std::unique_ptr<const android::CameraMetadata> mTemplates[CAMERA3_TEMPLATE_COUNT];
+ // Track in flight requests.
+ std::unique_ptr<RequestTracker> mInFlightTracker;
};
} // namespace default_camera_hal
diff --git a/modules/camera/3_4/request_tracker.cpp b/modules/camera/3_4/request_tracker.cpp
index 60715b5..dcf8263 100644
--- a/modules/camera/3_4/request_tracker.cpp
+++ b/modules/camera/3_4/request_tracker.cpp
@@ -53,7 +53,7 @@
return std::move(result);
}
-bool RequestTracker::Add(std::unique_ptr<CaptureRequest> request) {
+bool RequestTracker::Add(std::shared_ptr<CaptureRequest> request) {
if (!CanAddRequest(*request)) {
return false;
}
@@ -64,40 +64,42 @@
}
// Store the request.
- frames_in_flight_[request->frame_number] = std::move(request);
+ frames_in_flight_[request->frame_number] = request;
return true;
}
-bool RequestTracker::Remove(uint32_t frame_number,
- std::unique_ptr<CaptureRequest>* request) {
+bool RequestTracker::Remove(std::shared_ptr<CaptureRequest> request) {
// Get the request.
- std::unique_ptr<CaptureRequest> stored_request =
- std::move(frames_in_flight_[frame_number]);
- if (!stored_request) {
- ALOGE("%s: Frame %u is not in flight.", __func__, 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;
+ } else if (request != frame_number_request->second) {
+ ALOGE(
+ "%s: Request for frame %u cannot be removed: "
+ "does not matched the stored request.",
+ __func__,
+ request->frame_number);
return false;
}
- frames_in_flight_.erase(frame_number);
+
+ frames_in_flight_.erase(frame_number_request);
// Decrement the counts of used streams.
- for (const auto stream : RequestStreams(*stored_request)) {
+ for (const auto stream : RequestStreams(*request)) {
--buffers_in_flight_[stream];
}
- // Return the request if requested.
- if (request) {
- *request = std::move(stored_request);
- }
return true;
}
void RequestTracker::Clear(
- std::set<std::unique_ptr<CaptureRequest>>* requests) {
+ std::set<std::shared_ptr<CaptureRequest>>* requests) {
// If desired, extract all the currently in-flight requests.
if (requests) {
for (auto& frame_number_request : frames_in_flight_) {
- requests->insert(std::move(frame_number_request.second));
+ requests->insert(frame_number_request.second);
}
}
diff --git a/modules/camera/3_4/request_tracker.h b/modules/camera/3_4/request_tracker.h
index 864c2f7..a632a61 100644
--- a/modules/camera/3_4/request_tracker.h
+++ b/modules/camera/3_4/request_tracker.h
@@ -45,14 +45,13 @@
// Tracking methods.
// Track a request.
// False if a request of the same frame number is already being tracked
- virtual bool Add(std::unique_ptr<CaptureRequest> request);
- // Stop tracking and retreive a request by frame number.
- // False if the given frame number is not being tracked.
- virtual bool Remove(uint32_t frame_number,
- std::unique_ptr<CaptureRequest>* request = nullptr);
+ virtual bool Add(std::shared_ptr<CaptureRequest> request);
+ // Stop tracking a request.
+ // False if the given request is not being tracked.
+ virtual bool Remove(std::shared_ptr<CaptureRequest> request = nullptr);
// Empty out all requests being tracked.
virtual void Clear(
- std::set<std::unique_ptr<CaptureRequest>>* requests = nullptr);
+ std::set<std::shared_ptr<CaptureRequest>>* requests = nullptr);
// Accessors to check availability.
// Check that a request isn't already in flight, and won't overflow any
@@ -69,7 +68,7 @@
// Track for each stream, how many buffers are in flight.
std::map<const camera3_stream_t*, size_t> buffers_in_flight_;
// Track the frames in flight.
- std::map<uint32_t, std::unique_ptr<CaptureRequest>> frames_in_flight_;
+ std::map<uint32_t, std::shared_ptr<CaptureRequest>> frames_in_flight_;
DISALLOW_COPY_AND_ASSIGN(RequestTracker);
};
diff --git a/modules/camera/3_4/request_tracker_test.cpp b/modules/camera/3_4/request_tracker_test.cpp
index 9a6313f..bab6955 100644
--- a/modules/camera/3_4/request_tracker_test.cpp
+++ b/modules/camera/3_4/request_tracker_test.cpp
@@ -39,10 +39,10 @@
dut_->SetStreamConfiguration(config);
}
- std::unique_ptr<CaptureRequest> GenerateCaptureRequest(
+ std::shared_ptr<CaptureRequest> GenerateCaptureRequest(
uint32_t frame, std::vector<camera3_stream_t*> streams) {
- std::unique_ptr<CaptureRequest> request =
- std::make_unique<CaptureRequest>();
+ std::shared_ptr<CaptureRequest> request =
+ std::make_shared<CaptureRequest>();
// Set the frame number and buffers.
request->frame_number = frame;
@@ -52,19 +52,19 @@
request->output_buffers.push_back(buffer);
}
- return std::move(request);
+ return request;
}
void AddRequest(uint32_t frame,
std::vector<camera3_stream_t*> streams,
bool expected = true) {
- std::unique_ptr<CaptureRequest> request =
+ std::shared_ptr<CaptureRequest> request =
GenerateCaptureRequest(frame, streams);
EXPECT_EQ(dut_->CanAddRequest(*request), expected);
if (expected) {
EXPECT_FALSE(dut_->InFlight(frame));
}
- EXPECT_EQ(dut_->Add(std::move(request)), expected);
+ EXPECT_EQ(dut_->Add(request), expected);
if (expected) {
EXPECT_TRUE(dut_->InFlight(frame));
}
@@ -73,7 +73,7 @@
camera3_stream_t stream1_;
camera3_stream_t stream2_;
std::vector<camera3_stream_t*> streams_;
- std::unique_ptr<RequestTracker> dut_;
+ std::shared_ptr<RequestTracker> dut_;
};
TEST_F(RequestTrackerTest, AddValid) {
@@ -87,13 +87,13 @@
// Add a request
uint32_t frame = 42;
- std::unique_ptr<CaptureRequest> expected = GenerateCaptureRequest(frame, {});
+ std::shared_ptr<CaptureRequest> expected = GenerateCaptureRequest(frame, {});
// Set the input buffer instead of any outputs.
expected->input_buffer.reset(
new camera3_stream_buffer_t{&stream1_, nullptr, 0, -1, -1});
stream1_.max_buffers = 1;
- EXPECT_TRUE(dut_->Add(std::move(expected)));
+ EXPECT_TRUE(dut_->Add(expected));
EXPECT_TRUE(dut_->InFlight(frame));
// Should have added to the count of buffers for stream 1.
EXPECT_TRUE(dut_->StreamFull(&stream1_));
@@ -154,38 +154,20 @@
// Add a request
uint32_t frame = 42;
- std::unique_ptr<CaptureRequest> request =
+ std::shared_ptr<CaptureRequest> request =
GenerateCaptureRequest(frame, {&stream1_});
- CaptureRequest* expected = request.get();
- EXPECT_TRUE(dut_->Add(std::move(request)));
+ EXPECT_TRUE(dut_->Add(request));
EXPECT_TRUE(dut_->InFlight(frame));
AddRequest(frame + 1, {&stream1_});
EXPECT_FALSE(dut_->Empty());
// Remove it.
- request.reset();
- EXPECT_TRUE(dut_->Remove(frame, &request));
+ EXPECT_TRUE(dut_->Remove(request));
// Should have removed only the desired request.
EXPECT_FALSE(dut_->Empty());
-
- // Should have returned the desired request.
- EXPECT_EQ(request.get(), expected);
}
-TEST_F(RequestTrackerTest, RemoveNoResult) {
- EXPECT_TRUE(dut_->Empty());
-
- // Add a request
- uint32_t frame = 42;
- AddRequest(frame, {&stream1_});
- EXPECT_FALSE(dut_->Empty());
-
- // Remove it, but don't ask for the returned request.
- EXPECT_TRUE(dut_->Remove(frame));
- EXPECT_TRUE(dut_->Empty());
-}
-
-TEST_F(RequestTrackerTest, RemoveInvalid) {
+TEST_F(RequestTrackerTest, RemoveInvalidFrame) {
EXPECT_TRUE(dut_->Empty());
// Add a request
@@ -195,40 +177,58 @@
// Try to remove a different one.
uint32_t bad_frame = frame + 1;
+ std::shared_ptr<CaptureRequest> bad_request =
+ GenerateCaptureRequest(bad_frame, {&stream1_});
EXPECT_FALSE(dut_->InFlight(bad_frame));
- EXPECT_FALSE(dut_->Remove(frame + 1));
+ EXPECT_FALSE(dut_->Remove(bad_request));
EXPECT_FALSE(dut_->Empty());
}
+TEST_F(RequestTrackerTest, RemoveInvalidData) {
+ EXPECT_TRUE(dut_->Empty());
+
+ // Add a request
+ uint32_t frame = 42;
+ AddRequest(frame, {&stream1_});
+ EXPECT_FALSE(dut_->Empty());
+
+ // Try to remove a different one.
+ // Even though this request looks the same, that fact that it is
+ // a pointer to a different object means it should fail.
+ std::shared_ptr<CaptureRequest> bad_request =
+ GenerateCaptureRequest(frame, {&stream1_});
+ EXPECT_TRUE(dut_->InFlight(frame));
+ EXPECT_FALSE(dut_->Remove(bad_request));
+ EXPECT_FALSE(dut_->Empty());
+}
+
+TEST_F(RequestTrackerTest, RemoveNull) {
+ EXPECT_FALSE(dut_->Remove(nullptr));
+}
+
TEST_F(RequestTrackerTest, ClearRequests) {
// Create some requests.
uint32_t frame1 = 42;
uint32_t frame2 = frame1 + 1;
- std::unique_ptr<CaptureRequest> request1 =
+ std::shared_ptr<CaptureRequest> request1 =
GenerateCaptureRequest(frame1, {&stream1_});
- std::unique_ptr<CaptureRequest> request2 =
+ std::shared_ptr<CaptureRequest> request2 =
GenerateCaptureRequest(frame2, {&stream2_});
- std::set<CaptureRequest*> expected;
- expected.insert(request1.get());
- expected.insert(request2.get());
+ std::set<std::shared_ptr<CaptureRequest>> expected;
+ expected.insert(request1);
+ expected.insert(request2);
// Insert them.
- EXPECT_TRUE(dut_->Add(std::move(request1)));
- EXPECT_TRUE(dut_->Add(std::move(request2)));
+ EXPECT_TRUE(dut_->Add(request1));
+ EXPECT_TRUE(dut_->Add(request2));
EXPECT_TRUE(dut_->InFlight(frame1));
EXPECT_TRUE(dut_->InFlight(frame2));
EXPECT_FALSE(dut_->Empty());
- std::set<std::unique_ptr<CaptureRequest>> actual_wrapped;
+ std::set<std::shared_ptr<CaptureRequest>> actual;
// Clear them out.
- dut_->Clear(&actual_wrapped);
+ dut_->Clear(&actual);
EXPECT_TRUE(dut_->Empty());
-
- // Get the hard pointer values for comparison.
- std::set<CaptureRequest*> actual;
- for (const auto& actual_request : actual_wrapped) {
- actual.insert(actual_request.get());
- }
EXPECT_EQ(actual, expected);
}