From d7391394d3eb3d84e37a635ae5d0e71d862a589d Mon Sep 17 00:00:00 2001 From: Sam Lantinga Date: Sun, 16 Jun 2024 18:12:38 -0700 Subject: [PATCH] Switched the camera format to use framerate ratio instead of interval ratio This is more intuitive for game developers and users. Fixes https://github.com/libsdl-org/SDL/issues/9896 --- include/SDL3/SDL_camera.h | 4 +- src/camera/SDL_camera.c | 44 +++++++++---------- src/camera/SDL_syscamera.h | 2 +- src/camera/android/SDL_camera_android.c | 4 +- src/camera/coremedia/SDL_camera_coremedia.m | 20 ++++----- src/camera/emscripten/SDL_camera_emscripten.c | 14 +++--- .../SDL_camera_mediafoundation.c | 12 ++--- src/camera/pipewire/SDL_camera_pipewire.c | 5 +-- src/camera/v4l2/SDL_camera_v4l2.c | 15 ++++--- test/testcamera.c | 4 +- 10 files changed, 60 insertions(+), 64 deletions(-) diff --git a/include/SDL3/SDL_camera.h b/include/SDL3/SDL_camera.h index 79cf7366d5..1cd665ef05 100644 --- a/include/SDL3/SDL_camera.h +++ b/include/SDL3/SDL_camera.h @@ -82,8 +82,8 @@ typedef struct SDL_CameraSpec SDL_Colorspace colorspace; /**< Frame colorspace */ int width; /**< Frame width */ int height; /**< Frame height */ - int interval_numerator; /**< Frame rate numerator ((dom / num) == fps, (num / dom) == duration) */ - int interval_denominator; /**< Frame rate demoninator ((dom / num) == fps, (num / dom) == duration) */ + int framerate_numerator; /**< Frame rate numerator ((num / denom) == FPS, (denom / num) == duration in seconds) */ + int framerate_denominator; /**< Frame rate demoninator ((num / denom) == FPS, (denom / num) == duration in seconds) */ } SDL_CameraSpec; /** diff --git a/src/camera/SDL_camera.c b/src/camera/SDL_camera.c index 41e48ce194..a36149ccff 100644 --- a/src/camera/SDL_camera.c +++ b/src/camera/SDL_camera.c @@ -84,7 +84,7 @@ char *SDL_GetCameraThreadName(SDL_CameraDevice *device, char *buf, size_t buflen return buf; } -int SDL_AddCameraFormat(CameraFormatAddData *data, SDL_PixelFormatEnum format, SDL_Colorspace colorspace, int w, int h, int interval_numerator, int interval_denominator) +int SDL_AddCameraFormat(CameraFormatAddData *data, SDL_PixelFormatEnum format, SDL_Colorspace colorspace, int w, int h, int framerate_numerator, int framerate_denominator) { SDL_assert(data != NULL); if (data->allocated_specs <= data->num_specs) { @@ -102,8 +102,8 @@ int SDL_AddCameraFormat(CameraFormatAddData *data, SDL_PixelFormatEnum format, S spec->colorspace = colorspace; spec->width = w; spec->height = h; - spec->interval_numerator = interval_numerator; - spec->interval_denominator = interval_denominator; + spec->framerate_numerator = framerate_numerator; + spec->framerate_denominator = framerate_denominator; data->num_specs++; @@ -119,7 +119,7 @@ static int ZombieWaitDevice(SDL_CameraDevice *device) { if (!SDL_AtomicGet(&device->shutdown)) { // !!! FIXME: this is bad for several reasons (uses double, could be precalculated, doesn't track elasped time). - const double duration = ((double) device->actual_spec.interval_numerator / ((double) device->actual_spec.interval_denominator)); + const double duration = ((double) device->actual_spec.framerate_denominator / ((double) device->actual_spec.framerate_numerator)); SDL_Delay((Uint32) (duration * 1000.0)); } return 0; @@ -388,14 +388,14 @@ static int SDLCALL CameraSpecCmp(const void *vpa, const void *vpb) } // still here? We care about framerate less than format or size, but faster is better than slow. - if (a->interval_numerator && !b->interval_numerator) { + if (a->framerate_numerator && !b->framerate_numerator) { return -1; - } else if (!a->interval_numerator && b->interval_numerator) { + } else if (!a->framerate_numerator && b->framerate_numerator) { return 1; } - const float fpsa = ((float) a->interval_denominator)/ ((float) a->interval_numerator); - const float fpsb = ((float) b->interval_denominator)/ ((float) b->interval_numerator); + const float fpsa = ((float)a->framerate_numerator / a->framerate_denominator); + const float fpsb = ((float)b->framerate_numerator / b->framerate_denominator); if (fpsa > fpsb) { return -1; } else if (fpsb > fpsa) { @@ -483,7 +483,7 @@ SDL_CameraDevice *SDL_AddCameraDevice(const char *name, SDL_CameraPosition posit SDL_Log("CAMERA: Adding device '%s' (%s) with %d spec%s%s", name, posstr, num_specs, (num_specs == 1) ? "" : "s", (num_specs == 0) ? "" : ":"); for (int i = 0; i < num_specs; i++) { const SDL_CameraSpec *spec = &device->all_specs[i]; - SDL_Log("CAMERA: - fmt=%s, w=%d, h=%d, numerator=%d, denominator=%d", SDL_GetPixelFormatName(spec->format), spec->width, spec->height, spec->interval_numerator, spec->interval_denominator); + SDL_Log("CAMERA: - fmt=%s, w=%d, h=%d, numerator=%d, denominator=%d", SDL_GetPixelFormatName(spec->format), spec->width, spec->height, spec->framerate_numerator, spec->framerate_denominator); } #endif @@ -1024,23 +1024,23 @@ static void ChooseBestCameraSpec(SDL_CameraDevice *device, const SDL_CameraSpec closest->format = bestfmt; // We have a resolution and a format, find the closest framerate... - const float wantfps = spec->interval_denominator ? (spec->interval_numerator / spec->interval_denominator) : 0.0f; + const float wantfps = spec->framerate_denominator ? ((float)spec->framerate_numerator / spec->framerate_denominator) : 0.0f; float closestfps = 9999999.0f; for (int i = 0; i < num_specs; i++) { const SDL_CameraSpec *thisspec = &device->all_specs[i]; if ((thisspec->format == closest->format) && (thisspec->width == closest->width) && (thisspec->height == closest->height)) { - if ((thisspec->interval_numerator == spec->interval_numerator) && (thisspec->interval_denominator == spec->interval_denominator)) { - closest->interval_numerator = thisspec->interval_numerator; - closest->interval_denominator = thisspec->interval_denominator; + if ((thisspec->framerate_numerator == spec->framerate_numerator) && (thisspec->framerate_denominator == spec->framerate_denominator)) { + closest->framerate_numerator = thisspec->framerate_numerator; + closest->framerate_denominator = thisspec->framerate_denominator; break; // exact match, stop looking. } - const float thisfps = thisspec->interval_denominator ? (thisspec->interval_numerator / thisspec->interval_denominator) : 0.0f; + const float thisfps = thisspec->framerate_denominator ? ((float)thisspec->framerate_numerator / thisspec->framerate_denominator) : 0.0f; const float fpsdiff = SDL_fabsf(wantfps - thisfps); if (fpsdiff < closestfps) { // this is a closest FPS? Take it until something closer arrives. closestfps = fpsdiff; - closest->interval_numerator = thisspec->interval_numerator; - closest->interval_denominator = thisspec->interval_denominator; + closest->framerate_numerator = thisspec->framerate_numerator; + closest->framerate_denominator = thisspec->framerate_denominator; } } } @@ -1075,9 +1075,9 @@ SDL_Camera *SDL_OpenCameraDevice(SDL_CameraDeviceID instance_id, const SDL_Camer ChooseBestCameraSpec(device, spec, &closest); #if DEBUG_CAMERA - SDL_Log("CAMERA: App wanted [(%dx%d) fmt=%s interval=%d/%d], chose [(%dx%d) fmt=%s interval=%d/%d]", - spec ? spec->width : -1, spec ? spec->height : -1, spec ? SDL_GetPixelFormatName(spec->format) : "(null)", spec ? spec->interval_numerator : -1, spec ? spec->interval_denominator : -1, - closest.width, closest.height, SDL_GetPixelFormatName(closest.format), closest.interval_numerator, closest.interval_denominator); + SDL_Log("CAMERA: App wanted [(%dx%d) fmt=%s framerate=%d/%d], chose [(%dx%d) fmt=%s framerate=%d/%d]", + spec ? spec->width : -1, spec ? spec->height : -1, spec ? SDL_GetPixelFormatName(spec->format) : "(null)", spec ? spec->framerate_numerator : -1, spec ? spec->framerate_denominator : -1, + closest.width, closest.height, SDL_GetPixelFormatName(closest.format), closest.framerate_numerator, closest.framerate_denominator); #endif if (camera_driver.impl.OpenDevice(device, &closest) < 0) { @@ -1095,9 +1095,9 @@ SDL_Camera *SDL_OpenCameraDevice(SDL_CameraDeviceID instance_id, const SDL_Camer if (spec->format == SDL_PIXELFORMAT_UNKNOWN) { device->spec.format = closest.format; } - if (spec->interval_denominator == 0) { - device->spec.interval_numerator = closest.interval_numerator; - device->spec.interval_denominator = closest.interval_denominator; + if (spec->framerate_denominator == 0) { + device->spec.framerate_numerator = closest.framerate_numerator; + device->spec.framerate_denominator = closest.framerate_denominator; } } else { SDL_copyp(&device->spec, &closest); diff --git a/src/camera/SDL_syscamera.h b/src/camera/SDL_syscamera.h index 752d446c69..6ad966381e 100644 --- a/src/camera/SDL_syscamera.h +++ b/src/camera/SDL_syscamera.h @@ -64,7 +64,7 @@ typedef struct CameraFormatAddData int allocated_specs; } CameraFormatAddData; -int SDL_AddCameraFormat(CameraFormatAddData *data, SDL_PixelFormatEnum format, SDL_Colorspace colorspace, int w, int h, int interval_numerator, int interval_denominator); +int SDL_AddCameraFormat(CameraFormatAddData *data, SDL_PixelFormatEnum format, SDL_Colorspace colorspace, int w, int h, int framerate_numerator, int framerate_denominator); typedef struct SurfaceList { diff --git a/src/camera/android/SDL_camera_android.c b/src/camera/android/SDL_camera_android.c index bf6d6d16c4..7dbe251eb6 100644 --- a/src/camera/android/SDL_camera_android.c +++ b/src/camera/android/SDL_camera_android.c @@ -655,11 +655,11 @@ static void GatherCameraSpecs(const char *devid, CameraFormatAddData *add_data, const long long duration = (long long) i64ptr[3]; SDL_Log("CAMERA: possible fps %s %dx%d duration=%lld", SDL_GetPixelFormatName(sdlfmt), fpsw, fpsh, duration); if ((duration > 0) && (fpsfmt == fmt) && (fpsw == w) && (fpsh == h)) { - SDL_AddCameraFormat(add_data, sdlfmt, colorspace, w, h, duration, 1000000000); + SDL_AddCameraFormat(add_data, sdlfmt, colorspace, w, h, 1000000000, duration); } } #else - SDL_AddCameraFormat(add_data, sdlfmt, colorspace, w, h, 1, 30); + SDL_AddCameraFormat(add_data, sdlfmt, colorspace, w, h, 30, 1); #endif } diff --git a/src/camera/coremedia/SDL_camera_coremedia.m b/src/camera/coremedia/SDL_camera_coremedia.m index 153ec105b2..5b4bbaaf48 100644 --- a/src/camera/coremedia/SDL_camera_coremedia.m +++ b/src/camera/coremedia/SDL_camera_coremedia.m @@ -255,7 +255,7 @@ static int COREMEDIA_OpenDevice(SDL_CameraDevice *device, const SDL_CameraSpec * // Pick format that matches the spec const int w = spec->width; const int h = spec->height; - const int rate = spec->interval_denominator; + const float rate = (float)spec->framerate_numerator / spec->framerate_denominator; AVCaptureDeviceFormat *spec_format = nil; NSArray *formats = [avdevice formats]; for (AVCaptureDeviceFormat *format in formats) { @@ -273,7 +273,7 @@ static int COREMEDIA_OpenDevice(SDL_CameraDevice *device, const SDL_CameraSpec * } for (AVFrameRateRange *framerate in format.videoSupportedFrameRateRanges) { - if ((rate == (int) SDL_ceil((double) framerate.minFrameRate)) || (rate == (int) SDL_floor((double) framerate.maxFrameRate))) { + if (rate >= framerate.minFrameRate && rate <= framerate.maxFrameRate) { spec_format = format; break; } @@ -381,7 +381,7 @@ static void GatherCameraSpecs(AVCaptureDevice *device, CameraFormatAddData *add_ continue; } -NSLog(@"Available camera format: %@\n", fmt); +//NSLog(@"Available camera format: %@\n", fmt); SDL_PixelFormatEnum device_format = SDL_PIXELFORMAT_UNKNOWN; SDL_Colorspace device_colorspace = SDL_COLORSPACE_UNKNOWN; CoreMediaFormatToSDL(CMFormatDescriptionGetMediaSubType(fmt.formatDescription), &device_format, &device_colorspace); @@ -393,16 +393,12 @@ NSLog(@"Available camera format: %@\n", fmt); const int w = (int) dims.width; const int h = (int) dims.height; for (AVFrameRateRange *framerate in fmt.videoSupportedFrameRateRanges) { - int rate; + int numerator = 0, denominator = 1; - rate = (int) SDL_ceil((double) framerate.minFrameRate); - if (rate) { - SDL_AddCameraFormat(add_data, device_format, device_colorspace, w, h, 1, rate); - } - rate = (int) SDL_floor((double) framerate.maxFrameRate); - if (rate) { - SDL_AddCameraFormat(add_data, device_format, device_colorspace, w, h, 1, rate); - } + SDL_CalculateFraction(framerate.minFrameRate, &numerator, &denominator); + SDL_AddCameraFormat(add_data, device_format, device_colorspace, w, h, numerator, denominator); + SDL_CalculateFraction(framerate.maxFrameRate, &numerator, &denominator); + SDL_AddCameraFormat(add_data, device_format, device_colorspace, w, h, numerator, denominator); } } } diff --git a/src/camera/emscripten/SDL_camera_emscripten.c b/src/camera/emscripten/SDL_camera_emscripten.c index 6d0310c312..39278d8db2 100644 --- a/src/camera/emscripten/SDL_camera_emscripten.c +++ b/src/camera/emscripten/SDL_camera_emscripten.c @@ -102,8 +102,8 @@ static void SDLEmscriptenCameraDevicePermissionOutcome(SDL_CameraDevice *device, { device->spec.width = device->actual_spec.width = w; device->spec.height = device->actual_spec.height = h; - device->spec.interval_numerator = device->actual_spec.interval_numerator = 1; - device->spec.interval_denominator = device->actual_spec.interval_denominator = fps; + device->spec.framerate_numerator = device->actual_spec.framerate_numerator = fps; + device->spec.framerate_denominator = device->actual_spec.framerate_denominator = 1; SDL_CameraDevicePermissionOutcome(device, approved ? SDL_TRUE : SDL_FALSE); } @@ -115,8 +115,8 @@ static int EMSCRIPTENCAMERA_OpenDevice(SDL_CameraDevice *device, const SDL_Camer const device = $0; const w = $1; const h = $2; - const interval_numerator = $3; - const interval_denominator = $4; + const framerate_numerator = $3; + const framerate_denominator = $4; const outcome = $5; const iterate = $6; @@ -129,8 +129,8 @@ static int EMSCRIPTENCAMERA_OpenDevice(SDL_CameraDevice *device, const SDL_Camer constraints.video.height = h; } - if ((interval_numerator > 0) && (interval_denominator > 0)) { - var fps = interval_denominator / interval_numerator; + if ((framerate_numerator > 0) && (framerate_denominator > 0)) { + var fps = framerate_numerator / framerate_denominator; constraints.video.frameRate = { ideal: fps }; } @@ -199,7 +199,7 @@ static int EMSCRIPTENCAMERA_OpenDevice(SDL_CameraDevice *device, const SDL_Camer console.error("Tried to open camera but it threw an error! " + err.name + ": " + err.message); dynCall('viiiii', outcome, [device, 0, 0, 0, 0]); // we call this a permission error, because it probably is. }); - }, device, spec->width, spec->height, spec->interval_numerator, spec->interval_denominator, SDLEmscriptenCameraDevicePermissionOutcome, SDL_CameraThreadIterate); + }, device, spec->width, spec->height, spec->framerate_numerator, spec->framerate_denominator, SDLEmscriptenCameraDevicePermissionOutcome, SDL_CameraThreadIterate); return 0; // the real work waits until the user approves a camera. } diff --git a/src/camera/mediafoundation/SDL_camera_mediafoundation.c b/src/camera/mediafoundation/SDL_camera_mediafoundation.c index 811d2487b7..5076b134d1 100644 --- a/src/camera/mediafoundation/SDL_camera_mediafoundation.c +++ b/src/camera/mediafoundation/SDL_camera_mediafoundation.c @@ -765,7 +765,7 @@ static int MEDIAFOUNDATION_OpenDevice(SDL_CameraDevice *device, const SDL_Camera ret = IMFMediaType_SetUINT64(mediatype, &SDL_MF_MT_FRAME_SIZE, (((UINT64)spec->width) << 32) | ((UINT64)spec->height)); CHECK_HRESULT("MFSetAttributeSize(frame_size)", ret); - ret = IMFMediaType_SetUINT64(mediatype, &SDL_MF_MT_FRAME_RATE, (((UINT64)spec->interval_numerator) << 32) | ((UINT64)spec->interval_denominator)); + ret = IMFMediaType_SetUINT64(mediatype, &SDL_MF_MT_FRAME_RATE, (((UINT64)spec->framerate_numerator) << 32) | ((UINT64)spec->framerate_denominator)); CHECK_HRESULT("MFSetAttributeRatio(frame_rate)", ret); ret = IMFSourceReader_SetCurrentMediaType(srcreader, (DWORD)MF_SOURCE_READER_FIRST_VIDEO_STREAM, NULL, mediatype); @@ -940,12 +940,12 @@ static void GatherCameraSpecs(IMFMediaSource *source, CameraFormatAddData *add_d w = (UINT32)(val >> 32); h = (UINT32)val; if (SUCCEEDED(ret) && w && h) { - UINT32 interval_numerator = 0, interval_denominator = 0; + UINT32 framerate_numerator = 0, framerate_denominator = 0; ret = IMFMediaType_GetUINT64(mediatype, &SDL_MF_MT_FRAME_RATE, &val); - interval_numerator = (UINT32)(val >> 32); - interval_denominator = (UINT32)val; - if (SUCCEEDED(ret) && interval_numerator && interval_denominator) { - SDL_AddCameraFormat(add_data, sdlfmt, colorspace, (int) w, (int) h, (int)interval_numerator, (int)interval_denominator); + framerate_numerator = (UINT32)(val >> 32); + framerate_denominator = (UINT32)val; + if (SUCCEEDED(ret) && framerate_numerator && framerate_denominator) { + SDL_AddCameraFormat(add_data, sdlfmt, colorspace, (int) w, (int) h, (int)framerate_numerator, (int)framerate_denominator); } } } diff --git a/src/camera/pipewire/SDL_camera_pipewire.c b/src/camera/pipewire/SDL_camera_pipewire.c index 8c44fda534..4bf35ec3f4 100644 --- a/src/camera/pipewire/SDL_camera_pipewire.c +++ b/src/camera/pipewire/SDL_camera_pipewire.c @@ -517,7 +517,7 @@ static int PIPEWIRECAMERA_OpenDevice(SDL_CameraDevice *device, const SDL_CameraS SPA_FORMAT_VIDEO_format, SPA_POD_Id(sdl_format_to_id(spec->format)), SPA_FORMAT_VIDEO_size, SPA_POD_Rectangle(&SPA_RECTANGLE(spec->width, spec->height)), SPA_FORMAT_VIDEO_framerate, - SPA_POD_Fraction(&SPA_FRACTION(spec->interval_numerator, spec->interval_denominator))); + SPA_POD_Fraction(&SPA_FRACTION(spec->framerate_numerator, spec->framerate_denominator))); if ((res = PIPEWIRE_pw_stream_connect(device->hidden->stream, PW_DIRECTION_INPUT, @@ -625,8 +625,7 @@ static void collect_rates(CameraFormatAddData *data, struct param *p, SDL_PixelF SPA_FALLTHROUGH; case SPA_CHOICE_Enum: for (i = 0; i < n_vals; i++) { - // denom and num are switched, because SDL expects an interval, while pw provides a rate - if (SDL_AddCameraFormat(data, sdlfmt, colorspace, size->width, size->height, rates[i].denom, rates[i].num) < 0) { + if (SDL_AddCameraFormat(data, sdlfmt, colorspace, size->width, size->height, rates[i].num, rates[i].denom) < 0) { return; // Probably out of memory; we'll go with what we have, if anything. } } diff --git a/src/camera/v4l2/SDL_camera_v4l2.c b/src/camera/v4l2/SDL_camera_v4l2.c index 971b172c2c..7debf29676 100644 --- a/src/camera/v4l2/SDL_camera_v4l2.c +++ b/src/camera/v4l2/SDL_camera_v4l2.c @@ -538,16 +538,16 @@ static int V4L2_OpenDevice(SDL_CameraDevice *device, const SDL_CameraSpec *spec) return SDL_SetError("Error VIDIOC_S_FMT"); } - if (spec->interval_numerator && spec->interval_denominator) { + if (spec->framerate_numerator && spec->framerate_denominator) { struct v4l2_streamparm setfps; SDL_zero(setfps); setfps.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; if (xioctl(fd, VIDIOC_G_PARM, &setfps) == 0) { - if ( (setfps.parm.capture.timeperframe.numerator != spec->interval_numerator) || - (setfps.parm.capture.timeperframe.denominator = spec->interval_denominator) ) { + if ( (setfps.parm.capture.timeperframe.denominator != spec->framerate_numerator) || + (setfps.parm.capture.timeperframe.numerator = spec->framerate_denominator) ) { setfps.type = V4L2_BUF_TYPE_VIDEO_CAPTURE; - setfps.parm.capture.timeperframe.numerator = spec->interval_numerator; - setfps.parm.capture.timeperframe.denominator = spec->interval_denominator; + setfps.parm.capture.timeperframe.numerator = spec->framerate_numerator; + setfps.parm.capture.timeperframe.denominator = spec->framerate_denominator; if (xioctl(fd, VIDIOC_S_PARM, &setfps) == -1) { return SDL_SetError("Error VIDIOC_S_PARM"); } @@ -661,7 +661,7 @@ static int AddCameraFormat(const int fd, CameraFormatAddData *data, SDL_PixelFor const float fps = (float) denominator / (float) numerator; SDL_Log("CAMERA: * Has discrete frame interval (%d / %d), fps=%f", numerator, denominator, fps); #endif - if (SDL_AddCameraFormat(data, sdlfmt, colorspace, w, h, numerator, denominator) == -1) { + if (SDL_AddCameraFormat(data, sdlfmt, colorspace, w, h, denominator, numerator) == -1) { return -1; // Probably out of memory; we'll go with what we have, if anything. } frmivalenum.index++; // set up for the next one. @@ -673,7 +673,8 @@ static int AddCameraFormat(const int fd, CameraFormatAddData *data, SDL_PixelFor const float fps = (float) d / (float) n; SDL_Log("CAMERA: * Has %s frame interval (%d / %d), fps=%f", (frmivalenum.type == V4L2_FRMIVAL_TYPE_STEPWISE) ? "stepwise" : "continuous", n, d, fps); #endif - if (SDL_AddCameraFormat(data, sdlfmt, colorspace, w, h, n, d) == -1) { + // SDL expects framerate, V4L2 provides interval + if (SDL_AddCameraFormat(data, sdlfmt, colorspace, w, h, d, n) == -1) { return -1; // Probably out of memory; we'll go with what we have, if anything. } d += (int) frmivalenum.stepwise.step.denominator; diff --git a/test/testcamera.c b/test/testcamera.c index 678c77a95f..9e661064ae 100644 --- a/test/testcamera.c +++ b/test/testcamera.c @@ -134,8 +134,8 @@ int SDL_AppInit(void **appstate, int argc, char *argv[]) } SDL_CameraSpec *pspec = &spec; - spec.interval_numerator = 1000; - spec.interval_denominator = 1; + spec.framerate_numerator = 1000; + spec.framerate_denominator = 1; camera = SDL_OpenCameraDevice(camera_id, pspec); if (!camera) {