From c97ac94d0d97daf6396d6e2d6cc7d0ab655b3c6d Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Wed, 24 Jan 2024 16:29:53 -0500 Subject: [PATCH 1/7] Allow the simulated camera thread to wait a moment before acquiring a lock if we're about to get a frame. --- .../src/simcams/simulated.camera.c | 21 +++ .../tests/integration/CMakeLists.txt | 1 + .../integration/simcam-will-not-stall.cpp | 141 ++++++++++++++++++ 3 files changed, 163 insertions(+) create mode 100644 acquire-driver-common/tests/integration/simcam-will-not-stall.cpp diff --git a/acquire-driver-common/src/simcams/simulated.camera.c b/acquire-driver-common/src/simcams/simulated.camera.c index ddab23e..3910d3c 100644 --- a/acquire-driver-common/src/simcams/simulated.camera.c +++ b/acquire-driver-common/src/simcams/simulated.camera.c @@ -63,6 +63,9 @@ struct SimulatedCamera struct clock throttle; int is_running; struct thread thread; + struct lock lock; + int hold_for_capture; + struct condition_variable ready_to_capture; } streamer; struct @@ -227,6 +230,13 @@ simulated_camera_streamer_thread(struct SimulatedCamera* self) struct ImageShape full = { 0 }; uint32_t origin[2] = { 0, 0 }; + ECHO(lock_acquire(&self->streamer.lock)); + while (self->streamer.hold_for_capture) { + condition_variable_wait(&self->streamer.ready_to_capture, + &self->streamer.lock); + } + ECHO(lock_release(&self->streamer.lock)); + ECHO(lock_acquire(&self->im.lock)); ECHO(compute_full_resolution_shape_and_offset(self, &full, origin)); @@ -466,10 +476,18 @@ simcam_get_frame(struct Camera* camera, CHECK(*nbytes >= bytes_of_image(&self->im.shape)); CHECK(self->streamer.is_running); + self->streamer.hold_for_capture = 1; + TRACE("last: %5d current %5d", self->im.last_emitted_frame_id, self->im.frame_id); ECHO(lock_acquire(&self->im.lock)); + + ECHO(lock_acquire(&self->streamer.lock)); + self->streamer.hold_for_capture = 0; + condition_variable_notify_all(&self->streamer.ready_to_capture); + ECHO(lock_release(&self->streamer.lock)); + while (self->streamer.is_running && self->im.last_emitted_frame_id >= self->im.frame_id) { ECHO(condition_variable_wait(&self->im.frame_ready, &self->im.lock)); @@ -561,6 +579,9 @@ simcam_make_camera(enum BasicDeviceKind kind) condition_variable_init(&self->im.frame_ready); condition_variable_init(&self->software_trigger.trigger_ready); + lock_init(&self->streamer.lock); + condition_variable_init(&self->streamer.ready_to_capture); + return &self->camera; Error: if (self) diff --git a/acquire-driver-common/tests/integration/CMakeLists.txt b/acquire-driver-common/tests/integration/CMakeLists.txt index 5ddb46a..4f0d257 100644 --- a/acquire-driver-common/tests/integration/CMakeLists.txt +++ b/acquire-driver-common/tests/integration/CMakeLists.txt @@ -13,6 +13,7 @@ else () abort-while-waiting-for-trigger configure-triggering list-digital-lines + simcam-will-not-stall software-trigger-acquires-single-frames switch-storage-identifier write-side-by-side-tiff diff --git a/acquire-driver-common/tests/integration/simcam-will-not-stall.cpp b/acquire-driver-common/tests/integration/simcam-will-not-stall.cpp new file mode 100644 index 0000000..890da22 --- /dev/null +++ b/acquire-driver-common/tests/integration/simcam-will-not-stall.cpp @@ -0,0 +1,141 @@ +/// @file simcam-will-not-stall.cpp +/// Test that we can acquire frames from a slow-moving camera without hanging. + +#include "acquire.h" +#include "device/hal/device.manager.h" +#include "platform.h" +#include "logger.h" + +#include +#include + +void +reporter(int is_error, + const char* file, + int line, + const char* function, + const char* msg) +{ + fprintf(is_error ? stderr : stdout, + "%s%s(%d) - %s: %s\n", + is_error ? "ERROR " : "", + file, + line, + function, + msg); +} + +/// Helper for passing size static strings as function args. +/// For a function: `f(char*,size_t)` use `f(SIZED("hello"))`. +/// Expands to `f("hello",5)`. +#define SIZED(str) str, sizeof(str) + +#define L (aq_logger) +#define LOG(...) L(0, __FILE__, __LINE__, __FUNCTION__, __VA_ARGS__) +#define ERR(...) L(1, __FILE__, __LINE__, __FUNCTION__, __VA_ARGS__) +#define EXPECT(e, ...) \ + do { \ + if (!(e)) { \ + char buf[1 << 8] = { 0 }; \ + ERR(__VA_ARGS__); \ + snprintf(buf, sizeof(buf) - 1, __VA_ARGS__); \ + throw std::runtime_error(buf); \ + } \ + } while (0) +#define CHECK(e) EXPECT(e, "Expression evaluated as false: %s", #e) +#define DEVOK(e) CHECK(Device_Ok == (e)) +#define OK(e) CHECK(AcquireStatus_Ok == (e)) + +int +main() +{ + + auto runtime = acquire_init(reporter); + auto dm = acquire_device_manager(runtime); + CHECK(runtime); + CHECK(dm); + + AcquireProperties props = {}; + OK(acquire_get_configuration(runtime, &props)); + + DEVOK(device_manager_select(dm, + DeviceKind_Camera, + SIZED("simulated.*radial.*") - 1, + &props.video[0].camera.identifier)); + DEVOK(device_manager_select(dm, + DeviceKind_Storage, + SIZED("trash") - 1, + &props.video[0].storage.identifier)); + + OK(acquire_configure(runtime, &props)); + + AcquirePropertyMetadata metadata = { 0 }; + OK(acquire_get_configuration_metadata(runtime, &metadata)); + + props.video[0].camera.settings.binning = 1; + props.video[0].camera.settings.pixel_type = SampleType_u16; + props.video[0].camera.settings.shape = { + .x = 1920, + .y = 1080, + }; + props.video[0].max_frame_count = 100; + + OK(acquire_configure(runtime, &props)); + + const auto next = [](VideoFrame* cur) -> VideoFrame* { + return (VideoFrame*)(((uint8_t*)cur) + cur->bytes_of_frame); + }; + + const auto consumed_bytes = [](const VideoFrame* const cur, + const VideoFrame* const end) -> size_t { + return (uint8_t*)end - (uint8_t*)cur; + }; + + struct clock clock + {}; + // expected time to acquire frames + 100% + static double time_limit_ms = + (props.video[0].max_frame_count / 6.0) * 1000.0 * 2.0; + clock_init(&clock); + clock_shift_ms(&clock, time_limit_ms); + OK(acquire_start(runtime)); + { + uint64_t nframes = 0; + while (nframes < props.video[0].max_frame_count) { + struct clock throttle + {}; + clock_init(&throttle); + EXPECT(clock_cmp_now(&clock) < 0, + "Timeout at %f ms", + clock_toc_ms(&clock) + time_limit_ms); + VideoFrame *beg, *end, *cur; + OK(acquire_map_read(runtime, 0, &beg, &end)); + for (cur = beg; cur < end; cur = next(cur)) { + LOG("stream %d counting frame w id %d", 0, cur->frame_id); + CHECK(cur->shape.dims.width == + props.video[0].camera.settings.shape.x); + CHECK(cur->shape.dims.height == + props.video[0].camera.settings.shape.y); + ++nframes; + } + { + uint32_t n = (uint32_t)consumed_bytes(beg, end); + OK(acquire_unmap_read(runtime, 0, n)); + if (n) + LOG("stream %d consumed bytes %d", 0, n); + } + clock_sleep_ms(&throttle, 100.0f); + + LOG("stream %d nframes %d. remaining time %f s", + 0, + nframes, + -1e-3 * clock_toc_ms(&clock)); + } + + CHECK(nframes == props.video[0].max_frame_count); + } + + OK(acquire_stop(runtime)); + OK(acquire_shutdown(runtime)); + return 0; +} From aed05c26e4da6fa6040d5b8c499c1ee7b1f3b5a9 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Fri, 26 Jul 2024 09:49:17 -0400 Subject: [PATCH 2/7] Use a separate buffer to render simcam data, wait for trigger before render, sleep for balance of exposure time (if any). --- .../src/simcams/simulated.camera.c | 102 +++++++++----- .../integration/simcam-will-not-stall.cpp | 131 ++++++++++++------ 2 files changed, 156 insertions(+), 77 deletions(-) diff --git a/acquire-driver-common/src/simcams/simulated.camera.c b/acquire-driver-common/src/simcams/simulated.camera.c index 1d85486..fea6d45 100644 --- a/acquire-driver-common/src/simcams/simulated.camera.c +++ b/acquire-driver-common/src/simcams/simulated.camera.c @@ -63,9 +63,6 @@ struct SimulatedCamera struct clock throttle; int is_running; struct thread thread; - struct lock lock; - int hold_for_capture; - struct condition_variable ready_to_capture; } streamer; struct @@ -76,6 +73,7 @@ struct SimulatedCamera int64_t frame_id; int64_t last_emitted_frame_id; struct condition_variable frame_ready; + uint8_t frame_wanted; } im; struct @@ -220,27 +218,50 @@ simulated_camera_streamer_thread(struct SimulatedCamera* self) { clock_init(&self->streamer.throttle); + size_t bytes_of_data = aligned_bytes_of_image(&self->im.shape); + uint8_t* data = (uint8_t*)malloc(bytes_of_data); + int64_t frame_id = self->im.frame_id; + + const float exposure_time_ms = self->properties.exposure_time_us * 1e-3f; + while (self->streamer.is_running) { + if (self->properties.input_triggers.frame_start.enable) { + ECHO(lock_acquire(&self->im.lock)); + while (!self->software_trigger.triggered) { + ECHO(condition_variable_wait( + &self->software_trigger.trigger_ready, &self->im.lock)); + } + self->software_trigger.triggered = 0; + ECHO(lock_release(&self->im.lock)); + } + + clock_tic(&self->streamer.throttle); + struct ImageShape full = { 0 }; uint32_t origin[2] = { 0, 0 }; - ECHO(lock_acquire(&self->streamer.lock)); - while (self->streamer.hold_for_capture) { - condition_variable_wait(&self->streamer.ready_to_capture, - &self->streamer.lock); + // compute the full resolution shape and offset + { + ECHO(lock_acquire(&self->im.lock)); + ECHO(compute_full_resolution_shape_and_offset(self, &full, origin)); + ECHO(lock_release(&self->im.lock)); + + size_t nbytes = aligned_bytes_of_image(&full); + if (nbytes > bytes_of_data) { + free(data); + data = (uint8_t*)malloc(nbytes); + bytes_of_data = nbytes; + } } - ECHO(lock_release(&self->streamer.lock)); - - ECHO(lock_acquire(&self->im.lock)); - ECHO(compute_full_resolution_shape_and_offset(self, &full, origin)); + // generate the image switch (self->kind) { case BasicDevice_Camera_Random: - im_fill_rand(&full, self->im.data); + im_fill_rand(&full, data); break; case BasicDevice_Camera_Sin: ECHO(im_fill_pattern( - &full, (float)origin[0], (float)origin[1], self->im.data)); + &full, (float)origin[0], (float)origin[1], data)); break; case BasicDevice_Camera_Empty: break; // do nothing @@ -249,36 +270,48 @@ simulated_camera_streamer_thread(struct SimulatedCamera* self) "Unexpected index for the kind of simulated camera. Got: %d", self->kind); } - { + + // apply binning if applicable + if (self->properties.binning > 1) { int w = full.dims.width; int h = full.dims.height; int b = self->properties.binning >> 1; while (b) { - ECHO(bin2(self->im.data, w, h)); + ECHO(bin2(data, w, h)); b >>= 1; w >>= 1; h >>= 1; } } - if (self->properties.input_triggers.frame_start.enable) { - while (!self->software_trigger.triggered) { - ECHO(condition_variable_wait( - &self->software_trigger.trigger_ready, &self->im.lock)); - } - self->software_trigger.triggered = 0; + ++frame_id; + + float toc = (float)clock_toc_ms(&self->streamer.throttle); + + // TODO (aliddell): + // preferably this would be a wait on a condition variable with a + // timeout, but we don't have that in the platform yet. + if (self->streamer.is_running && toc < exposure_time_ms) { + clock_sleep_ms(&self->streamer.throttle, exposure_time_ms - toc); } - self->hardware_timestamp = clock_tic(0); - ++self->im.frame_id; + if (self->im.frame_wanted) { + ECHO(lock_acquire(&self->im.lock)); - ECHO(condition_variable_notify_all(&self->im.frame_ready)); - ECHO(lock_release(&self->im.lock)); + if (self->kind != BasicDevice_Camera_Empty) { + memcpy(self->im.data, data, bytes_of_data); + } + + self->hardware_timestamp = clock_tic(0); + self->im.frame_id = frame_id; + self->im.frame_wanted = 0; + ECHO(lock_release(&self->im.lock)); + } - if (self->streamer.is_running) - clock_sleep_ms(&self->streamer.throttle, - self->properties.exposure_time_us * 1e-3f); + ECHO(condition_variable_notify_all(&self->im.frame_ready)); } + + free(data); } // @@ -435,6 +468,8 @@ simcam_execute_trigger(struct Camera* camera) struct SimulatedCamera* self = containerof(camera, struct SimulatedCamera, camera); + self->im.frame_wanted = 1; + lock_acquire(&self->im.lock); self->software_trigger.triggered = 1; condition_variable_notify_all(&self->software_trigger.trigger_ready); @@ -470,18 +505,13 @@ simcam_get_frame(struct Camera* camera, CHECK(*nbytes >= bytes_of_image(&self->im.shape)); CHECK(self->streamer.is_running); - self->streamer.hold_for_capture = 1; + self->im.frame_wanted = 1; TRACE("last: %5d current %5d", self->im.last_emitted_frame_id, self->im.frame_id); ECHO(lock_acquire(&self->im.lock)); - ECHO(lock_acquire(&self->streamer.lock)); - self->streamer.hold_for_capture = 0; - condition_variable_notify_all(&self->streamer.ready_to_capture); - ECHO(lock_release(&self->streamer.lock)); - while (self->streamer.is_running && self->im.last_emitted_frame_id >= self->im.frame_id) { ECHO(condition_variable_wait(&self->im.frame_ready, &self->im.lock)); @@ -555,6 +585,7 @@ simcam_make_camera(enum BasicDeviceKind kind) }, .type=properties.pixel_type }, + .frame_wanted = 0, }, .camera={ .state = DeviceState_AwaitingConfiguration, @@ -573,9 +604,6 @@ simcam_make_camera(enum BasicDeviceKind kind) condition_variable_init(&self->im.frame_ready); condition_variable_init(&self->software_trigger.trigger_ready); - lock_init(&self->streamer.lock); - condition_variable_init(&self->streamer.ready_to_capture); - return &self->camera; Error: if (self) diff --git a/acquire-driver-common/tests/integration/simcam-will-not-stall.cpp b/acquire-driver-common/tests/integration/simcam-will-not-stall.cpp index 890da22..521f115 100644 --- a/acquire-driver-common/tests/integration/simcam-will-not-stall.cpp +++ b/acquire-driver-common/tests/integration/simcam-will-not-stall.cpp @@ -8,21 +8,49 @@ #include #include +#include -void +static class IntrospectiveLogger +{ + public: + IntrospectiveLogger() + : dropped_logs(0){}; + + // inspect for "[stream 0] Dropped", otherwise pass the mesage through + void report_and_inspect(int is_error, + const char* file, + int line, + const char* function, + const char* msg) + { + std::string m(msg); + if (m.length() >= 18 && m.substr(0, 18) == "[stream 0] Dropped") + ++dropped_logs; + + printf("%s%s(%d) - %s: %s\n", + is_error ? "ERROR " : "", + file, + line, + function, + msg); + } + + [[nodiscard]] bool frames_were_dropped() const { return dropped_logs > 0; } + void reset() { dropped_logs = 0; } + + private: + size_t dropped_logs; +} introspective_logger; + +static void reporter(int is_error, const char* file, int line, const char* function, const char* msg) { - fprintf(is_error ? stderr : stdout, - "%s%s(%d) - %s: %s\n", - is_error ? "ERROR " : "", - file, - line, - function, - msg); + introspective_logger.report_and_inspect( + is_error, file, line, function, msg); } /// Helper for passing size static strings as function args. @@ -46,13 +74,12 @@ reporter(int is_error, #define DEVOK(e) CHECK(Device_Ok == (e)) #define OK(e) CHECK(AcquireStatus_Ok == (e)) -int -main() +void +configure(AcquireRuntime* runtime, std::string_view camera_type) { - - auto runtime = acquire_init(reporter); - auto dm = acquire_device_manager(runtime); CHECK(runtime); + + const DeviceManager* dm = acquire_device_manager(runtime); CHECK(dm); AcquireProperties props = {}; @@ -60,7 +87,8 @@ main() DEVOK(device_manager_select(dm, DeviceKind_Camera, - SIZED("simulated.*radial.*") - 1, + camera_type.data(), + camera_type.size(), &props.video[0].camera.identifier)); DEVOK(device_manager_select(dm, DeviceKind_Storage, @@ -69,19 +97,22 @@ main() OK(acquire_configure(runtime, &props)); - AcquirePropertyMetadata metadata = { 0 }; - OK(acquire_get_configuration_metadata(runtime, &metadata)); - props.video[0].camera.settings.binning = 1; props.video[0].camera.settings.pixel_type = SampleType_u16; props.video[0].camera.settings.shape = { .x = 1920, .y = 1080, }; + props.video[0].camera.settings.exposure_time_us = 1; // very small exposure + props.video[0].max_frame_count = 100; OK(acquire_configure(runtime, &props)); +} +void +acquire(AcquireRuntime* runtime) +{ const auto next = [](VideoFrame* cur) -> VideoFrame* { return (VideoFrame*)(((uint8_t*)cur) + cur->bytes_of_frame); }; @@ -91,51 +122,71 @@ main() return (uint8_t*)end - (uint8_t*)cur; }; - struct clock clock - {}; + AcquireProperties props = {}; + OK(acquire_get_configuration(runtime, &props)); + // expected time to acquire frames + 100% static double time_limit_ms = - (props.video[0].max_frame_count / 6.0) * 1000.0 * 2.0; + (props.video[0].max_frame_count / 3.0) * 1000.0 * 2.0; + + struct clock clock = {}; clock_init(&clock); clock_shift_ms(&clock, time_limit_ms); + OK(acquire_start(runtime)); { uint64_t nframes = 0; while (nframes < props.video[0].max_frame_count) { - struct clock throttle - {}; + struct clock throttle = {}; clock_init(&throttle); + EXPECT(clock_cmp_now(&clock) < 0, "Timeout at %f ms", clock_toc_ms(&clock) + time_limit_ms); + VideoFrame *beg, *end, *cur; + OK(acquire_map_read(runtime, 0, &beg, &end)); for (cur = beg; cur < end; cur = next(cur)) { - LOG("stream %d counting frame w id %d", 0, cur->frame_id); - CHECK(cur->shape.dims.width == - props.video[0].camera.settings.shape.x); - CHECK(cur->shape.dims.height == - props.video[0].camera.settings.shape.y); ++nframes; } - { - uint32_t n = (uint32_t)consumed_bytes(beg, end); - OK(acquire_unmap_read(runtime, 0, n)); - if (n) - LOG("stream %d consumed bytes %d", 0, n); - } - clock_sleep_ms(&throttle, 100.0f); - LOG("stream %d nframes %d. remaining time %f s", - 0, - nframes, - -1e-3 * clock_toc_ms(&clock)); + uint32_t n = (uint32_t)consumed_bytes(beg, end); + OK(acquire_unmap_read(runtime, 0, n)); + + clock_sleep_ms(&throttle, 100.0f); } CHECK(nframes == props.video[0].max_frame_count); } OK(acquire_stop(runtime)); - OK(acquire_shutdown(runtime)); - return 0; +} + +int +main() +{ + int retval = 1; + + AcquireRuntime* runtime = acquire_init(reporter); + + try { + configure(runtime, "simulated.*sin*"); + acquire(runtime); + CHECK(!introspective_logger.frames_were_dropped()); + + introspective_logger.reset(); + + configure(runtime, "simulated.*empty.*"); + acquire(runtime); + CHECK(introspective_logger.frames_were_dropped()); + + retval = 0; + } catch (const std::exception& e) { + ERR("Exception: %s", e.what()); + } + + acquire_shutdown(runtime); + + return retval; } From 369915dad10f80a1425980e77bf031b74bdc3986 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Fri, 26 Jul 2024 10:54:16 -0400 Subject: [PATCH 3/7] Check that few frames are dropped with radial sine, rather than none. --- .../integration/simcam-will-not-stall.cpp | 32 ++++++++++++------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/acquire-driver-common/tests/integration/simcam-will-not-stall.cpp b/acquire-driver-common/tests/integration/simcam-will-not-stall.cpp index 521f115..d9c280a 100644 --- a/acquire-driver-common/tests/integration/simcam-will-not-stall.cpp +++ b/acquire-driver-common/tests/integration/simcam-will-not-stall.cpp @@ -7,16 +7,19 @@ #include "logger.h" #include +#include #include #include -static class IntrospectiveLogger +namespace { +class IntrospectiveLogger { public: IntrospectiveLogger() - : dropped_logs(0){}; + : dropped_frames(0) + , re("Dropped\\s*(\\d+)"){}; - // inspect for "[stream 0] Dropped", otherwise pass the mesage through + // inspect for "[stream 0] Dropped", otherwise pass the message through void report_and_inspect(int is_error, const char* file, int line, @@ -24,8 +27,11 @@ static class IntrospectiveLogger const char* msg) { std::string m(msg); - if (m.length() >= 18 && m.substr(0, 18) == "[stream 0] Dropped") - ++dropped_logs; + + std::smatch matches; + if (std::regex_search(m, matches, re)) { + dropped_frames += std::stoi(matches[1]); + } printf("%s%s(%d) - %s: %s\n", is_error ? "ERROR " : "", @@ -35,12 +41,14 @@ static class IntrospectiveLogger msg); } - [[nodiscard]] bool frames_were_dropped() const { return dropped_logs > 0; } - void reset() { dropped_logs = 0; } + size_t get_dropped_frames() const { return dropped_frames; } + void reset() { dropped_frames = 0; } private: - size_t dropped_logs; + size_t dropped_frames; + std::regex re; } introspective_logger; +} // namespace static void reporter(int is_error, @@ -74,6 +82,8 @@ reporter(int is_error, #define DEVOK(e) CHECK(Device_Ok == (e)) #define OK(e) CHECK(AcquireStatus_Ok == (e)) +const static size_t frame_count = 100; + void configure(AcquireRuntime* runtime, std::string_view camera_type) { @@ -105,7 +115,7 @@ configure(AcquireRuntime* runtime, std::string_view camera_type) }; props.video[0].camera.settings.exposure_time_us = 1; // very small exposure - props.video[0].max_frame_count = 100; + props.video[0].max_frame_count = frame_count; OK(acquire_configure(runtime, &props)); } @@ -173,13 +183,13 @@ main() try { configure(runtime, "simulated.*sin*"); acquire(runtime); - CHECK(!introspective_logger.frames_were_dropped()); + CHECK(introspective_logger.get_dropped_frames() < frame_count); introspective_logger.reset(); configure(runtime, "simulated.*empty.*"); acquire(runtime); - CHECK(introspective_logger.frames_were_dropped()); + CHECK(introspective_logger.get_dropped_frames() >= frame_count); retval = 0; } catch (const std::exception& e) { From 50bf046956ce1966474281a06f3aa6d16cefd40a Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Fri, 26 Jul 2024 16:41:40 -0400 Subject: [PATCH 4/7] Respond to PR comments. --- .../src/simcams/simulated.camera.c | 95 ++++++++----------- 1 file changed, 42 insertions(+), 53 deletions(-) diff --git a/acquire-driver-common/src/simcams/simulated.camera.c b/acquire-driver-common/src/simcams/simulated.camera.c index fea6d45..2557073 100644 --- a/acquire-driver-common/src/simcams/simulated.camera.c +++ b/acquire-driver-common/src/simcams/simulated.camera.c @@ -67,7 +67,8 @@ struct SimulatedCamera struct { - void* data; + void* frame_data; // for storing the image + void* render_data; // for rendering the image struct ImageShape shape; struct lock lock; int64_t frame_id; @@ -218,50 +219,38 @@ simulated_camera_streamer_thread(struct SimulatedCamera* self) { clock_init(&self->streamer.throttle); - size_t bytes_of_data = aligned_bytes_of_image(&self->im.shape); - uint8_t* data = (uint8_t*)malloc(bytes_of_data); int64_t frame_id = self->im.frame_id; - - const float exposure_time_ms = self->properties.exposure_time_us * 1e-3f; - while (self->streamer.is_running) { - if (self->properties.input_triggers.frame_start.enable) { - ECHO(lock_acquire(&self->im.lock)); - while (!self->software_trigger.triggered) { - ECHO(condition_variable_wait( - &self->software_trigger.trigger_ready, &self->im.lock)); - } - self->software_trigger.triggered = 0; - ECHO(lock_release(&self->im.lock)); - } - - clock_tic(&self->streamer.throttle); - struct ImageShape full = { 0 }; uint32_t origin[2] = { 0, 0 }; + ECHO(lock_acquire(&self->im.lock)); + while (self->properties.input_triggers.frame_start.enable && + !self->software_trigger.triggered) { + ECHO(condition_variable_wait(&self->software_trigger.trigger_ready, + &self->im.lock)); + } + self->software_trigger.triggered = 0; + // compute the full resolution shape and offset - { - ECHO(lock_acquire(&self->im.lock)); - ECHO(compute_full_resolution_shape_and_offset(self, &full, origin)); - ECHO(lock_release(&self->im.lock)); + ECHO(compute_full_resolution_shape_and_offset(self, &full, origin)); - size_t nbytes = aligned_bytes_of_image(&full); - if (nbytes > bytes_of_data) { - free(data); - data = (uint8_t*)malloc(nbytes); - bytes_of_data = nbytes; - } - } + const float exposure_time_ms = + self->properties.exposure_time_us * 1e-3f; + ECHO(lock_release(&self->im.lock)); + + clock_tic(&self->streamer.throttle); // generate the image switch (self->kind) { case BasicDevice_Camera_Random: - im_fill_rand(&full, data); + im_fill_rand(&full, self->im.render_data); break; case BasicDevice_Camera_Sin: - ECHO(im_fill_pattern( - &full, (float)origin[0], (float)origin[1], data)); + ECHO(im_fill_pattern(&full, + (float)origin[0], + (float)origin[1], + self->im.render_data)); break; case BasicDevice_Camera_Empty: break; // do nothing @@ -277,7 +266,7 @@ simulated_camera_streamer_thread(struct SimulatedCamera* self) int h = full.dims.height; int b = self->properties.binning >> 1; while (b) { - ECHO(bin2(data, w, h)); + ECHO(bin2(self->im.render_data, w, h)); b >>= 1; w >>= 1; h >>= 1; @@ -286,32 +275,28 @@ simulated_camera_streamer_thread(struct SimulatedCamera* self) ++frame_id; + // sleep for the remainder of the exposure time float toc = (float)clock_toc_ms(&self->streamer.throttle); - - // TODO (aliddell): - // preferably this would be a wait on a condition variable with a - // timeout, but we don't have that in the platform yet. - if (self->streamer.is_running && toc < exposure_time_ms) { + if (self->streamer.is_running) { clock_sleep_ms(&self->streamer.throttle, exposure_time_ms - toc); } if (self->im.frame_wanted) { ECHO(lock_acquire(&self->im.lock)); + size_t nbytes = aligned_bytes_of_image(&self->im.shape); if (self->kind != BasicDevice_Camera_Empty) { - memcpy(self->im.data, data, bytes_of_data); + memcpy(self->im.frame_data, self->im.render_data, nbytes); } self->hardware_timestamp = clock_tic(0); self->im.frame_id = frame_id; self->im.frame_wanted = 0; + + ECHO(condition_variable_notify_all(&self->im.frame_ready)); ECHO(lock_release(&self->im.lock)); } - - ECHO(condition_variable_notify_all(&self->im.frame_ready)); } - - free(data); } // @@ -419,8 +404,11 @@ simcam_set(struct Camera* camera, struct CameraProperties* settings) }; size_t nbytes = aligned_bytes_of_image(shape); - self->im.data = malloc(nbytes); - EXPECT(self->im.data, "Allocation of %llu bytes failed.", nbytes); + self->im.frame_data = malloc(nbytes); + EXPECT(self->im.frame_data, "Allocation of %llu bytes failed.", nbytes); + + self->im.render_data = malloc(nbytes); + EXPECT(self->im.render_data, "Allocation of %llu bytes failed.", nbytes); return Device_Ok; Error: @@ -468,9 +456,8 @@ simcam_execute_trigger(struct Camera* camera) struct SimulatedCamera* self = containerof(camera, struct SimulatedCamera, camera); - self->im.frame_wanted = 1; - lock_acquire(&self->im.lock); + self->im.frame_wanted = 1; self->software_trigger.triggered = 1; condition_variable_notify_all(&self->software_trigger.trigger_ready); lock_release(&self->im.lock); @@ -505,12 +492,11 @@ simcam_get_frame(struct Camera* camera, CHECK(*nbytes >= bytes_of_image(&self->im.shape)); CHECK(self->streamer.is_running); - self->im.frame_wanted = 1; - TRACE("last: %5d current %5d", self->im.last_emitted_frame_id, self->im.frame_id); ECHO(lock_acquire(&self->im.lock)); + self->im.frame_wanted = 1; while (self->streamer.is_running && self->im.last_emitted_frame_id >= self->im.frame_id) { @@ -521,7 +507,7 @@ simcam_get_frame(struct Camera* camera, goto Shutdown; } - memcpy(im, self->im.data, bytes_of_image(&self->im.shape)); // NOLINT + memcpy(im, self->im.frame_data, bytes_of_image(&self->im.shape)); // NOLINT info_out->shape = self->im.shape; info_out->hardware_frame_id = self->im.frame_id; info_out->hardware_timestamp = self->hardware_timestamp; @@ -539,8 +525,10 @@ simcam_close_camera(struct Camera* camera_) containerof(camera_, struct SimulatedCamera, camera); EXPECT(camera_, "Invalid NULL parameter"); simcam_stop(&camera->camera); - if (camera->im.data) - free(camera->im.data); + if (camera->im.frame_data) + free(camera->im.frame_data); + if (camera->im.render_data) + free(camera->im.render_data); free(camera); return Device_Ok; Error: @@ -569,7 +557,8 @@ simcam_make_camera(enum BasicDeviceKind kind) .properties = properties, .kind=kind, .im={ - .data=0, + .frame_data=0, + .render_data=0, .shape = { .dims = { .channels = 1, From ef515f1fee12bd723bb7ccd22698d3502706755f Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Tue, 30 Jul 2024 10:14:50 -0400 Subject: [PATCH 5/7] Respond to PR comments. --- .../src/simcams/simulated.camera.c | 27 +++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/acquire-driver-common/src/simcams/simulated.camera.c b/acquire-driver-common/src/simcams/simulated.camera.c index 2557073..e70d0d2 100644 --- a/acquire-driver-common/src/simcams/simulated.camera.c +++ b/acquire-driver-common/src/simcams/simulated.camera.c @@ -284,9 +284,10 @@ simulated_camera_streamer_thread(struct SimulatedCamera* self) if (self->im.frame_wanted) { ECHO(lock_acquire(&self->im.lock)); - size_t nbytes = aligned_bytes_of_image(&self->im.shape); - if (self->kind != BasicDevice_Camera_Empty) { - memcpy(self->im.frame_data, self->im.render_data, nbytes); + { + void* const tmp = self->im.frame_data; + self->im.frame_data = self->im.render_data; + self->im.render_data = tmp; } self->hardware_timestamp = clock_tic(0); @@ -373,6 +374,7 @@ simcam_set(struct Camera* camera, struct CameraProperties* settings) simcam_execute_trigger(camera); } + ECHO(lock_acquire(&self->im.lock)); self->properties = *settings; self->properties.pixel_type = settings->pixel_type; self->properties.input_triggers = (struct camera_properties_input_triggers_s){ @@ -404,12 +406,27 @@ simcam_set(struct Camera* camera, struct CameraProperties* settings) }; size_t nbytes = aligned_bytes_of_image(shape); + if (self->im.frame_data) + free(self->im.frame_data); + self->im.frame_data = malloc(nbytes); - EXPECT(self->im.frame_data, "Allocation of %llu bytes failed.", nbytes); + if (!self->im.frame_data) { + LOGE("Allocation of %llu bytes failed.", nbytes); + lock_release(&self->im.lock); + goto Error; + } + + if (self->im.render_data) + free(self->im.render_data); self->im.render_data = malloc(nbytes); - EXPECT(self->im.render_data, "Allocation of %llu bytes failed.", nbytes); + if (!self->im.render_data) { + LOGE("Allocation of %llu bytes failed.", nbytes); + lock_release(&self->im.lock); + goto Error; + } + lock_release(&self->im.lock); return Device_Ok; Error: return Device_Err; From 04cbefcecffa9953c62401f98a1e216de5e61a66 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Tue, 30 Jul 2024 10:30:56 -0400 Subject: [PATCH 6/7] Apply suggestions from code review Co-authored-by: Nathan Clack --- acquire-driver-common/src/simcams/simulated.camera.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/acquire-driver-common/src/simcams/simulated.camera.c b/acquire-driver-common/src/simcams/simulated.camera.c index e70d0d2..2cbc93d 100644 --- a/acquire-driver-common/src/simcams/simulated.camera.c +++ b/acquire-driver-common/src/simcams/simulated.camera.c @@ -406,8 +406,7 @@ simcam_set(struct Camera* camera, struct CameraProperties* settings) }; size_t nbytes = aligned_bytes_of_image(shape); - if (self->im.frame_data) - free(self->im.frame_data); + free(self->im.frame_data); self->im.frame_data = malloc(nbytes); if (!self->im.frame_data) { @@ -416,8 +415,7 @@ simcam_set(struct Camera* camera, struct CameraProperties* settings) goto Error; } - if (self->im.render_data) - free(self->im.render_data); + free(self->im.render_data); self->im.render_data = malloc(nbytes); if (!self->im.render_data) { From 8ae31e7d34d9bebbeaa25ed0f19ec8e397833292 Mon Sep 17 00:00:00 2001 From: Alan Liddell Date: Tue, 30 Jul 2024 12:13:38 -0400 Subject: [PATCH 7/7] Respond to PR comments. --- .../src/simcams/simulated.camera.c | 54 ++++++++++--------- 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/acquire-driver-common/src/simcams/simulated.camera.c b/acquire-driver-common/src/simcams/simulated.camera.c index 2cbc93d..677a86a 100644 --- a/acquire-driver-common/src/simcams/simulated.camera.c +++ b/acquire-driver-common/src/simcams/simulated.camera.c @@ -194,6 +194,19 @@ compute_strides(struct ImageShape* shape) st[i] = st[i - 1] * dims[i - 1]; } +static void* +checked_realloc(void* in, size_t nbytes) +{ + void* out = realloc(in, nbytes); + EXPECT(out, "Allocation of %llu bytes failed.", nbytes); + +Finalize: + return out; +Error: + free(in); + goto Finalize; +} + static void compute_full_resolution_shape_and_offset(const struct SimulatedCamera* self, struct ImageShape* shape, @@ -364,9 +377,10 @@ simcam_set(struct Camera* camera, struct CameraProperties* settings) if (!settings->binning) settings->binning = 1; - EXPECT(popcount_u8(settings->binning) == 1, - "Binning must be a power of two. Got %d.", - settings->binning); + if (popcount_u8(settings->binning) != 1) { + LOGE("Binning must be a power of two. Got %d.", settings->binning); + return Device_Err; + } if (self->properties.input_triggers.frame_start.enable && !settings->input_triggers.frame_start.enable) { @@ -374,6 +388,8 @@ simcam_set(struct Camera* camera, struct CameraProperties* settings) simcam_execute_trigger(camera); } + enum DeviceStatusCode status = Device_Ok; + ECHO(lock_acquire(&self->im.lock)); self->properties = *settings; self->properties.pixel_type = settings->pixel_type; @@ -406,28 +422,15 @@ simcam_set(struct Camera* camera, struct CameraProperties* settings) }; size_t nbytes = aligned_bytes_of_image(shape); - free(self->im.frame_data); - - self->im.frame_data = malloc(nbytes); - if (!self->im.frame_data) { - LOGE("Allocation of %llu bytes failed.", nbytes); - lock_release(&self->im.lock); - goto Error; - } - - free(self->im.render_data); - - self->im.render_data = malloc(nbytes); - if (!self->im.render_data) { - LOGE("Allocation of %llu bytes failed.", nbytes); - lock_release(&self->im.lock); - goto Error; - } + CHECK(self->im.frame_data = checked_realloc(self->im.frame_data, nbytes)); + CHECK(self->im.render_data = checked_realloc(self->im.render_data, nbytes)); +Finalize: lock_release(&self->im.lock); - return Device_Ok; + return status; Error: - return Device_Err; + status = Device_Err; + goto Finalize; } static enum DeviceStatusCode @@ -540,10 +543,9 @@ simcam_close_camera(struct Camera* camera_) containerof(camera_, struct SimulatedCamera, camera); EXPECT(camera_, "Invalid NULL parameter"); simcam_stop(&camera->camera); - if (camera->im.frame_data) - free(camera->im.frame_data); - if (camera->im.render_data) - free(camera->im.render_data); + + free(camera->im.frame_data); + free(camera->im.render_data); free(camera); return Device_Ok; Error: