Add video capture API proposal#5477
Conversation
|
Separation of list procedures is a good thing that can be done without this PR. |
|
@sezero yes, this is done thanks ! Should be ok ! |
|
A few notes (all of which can be rectified easily later when
|
|
@sezero thanks ! I've fixed. The whole thing is mostly a draft now ... |
|
Is there a specific reason you want a dedicated SDL_VideoCaptureDeviceID type Here is a very minor tidy-up: test/Makefile.os2 part is commented out because diff --git a/Makefile.os2 b/Makefile.os2
index 9060e0e..3902d25 100644
--- a/Makefile.os2
+++ b/Makefile.os2
@@ -77,3 +77,4 @@ SRCS+= SDL_blit.c SDL_blit_0.c SDL_blit_1.c SDL_blit_A.c SDL_blit_auto.c &
SDL_pixels.c SDL_rect.c SDL_RLEaccel.c SDL_shape.c SDL_stretch.c &
- SDL_surface.c SDL_video.c SDL_video_capture.c SDL_clipboard.c SDL_vulkan_utils.c SDL_egl.c
+ SDL_surface.c SDL_video.c SDL_clipboard.c SDL_video_capture.c SDL_egl.c &
+ SDL_vulkan_utils.c
diff --git a/test/testvideocapture.c b/test/testvideocapture.c
index e1a7816..3672092 100644
--- a/test/testvideocapture.c
+++ b/test/testvideocapture.c
@@ -353,4 +353,6 @@ int main(int argc, char **argv)
{
+ static int x = 0;
SDL_Rect r;
- static int x = 0; x += 10; if (x > 1000) x = 0;
+ x += 10;
+ if (x > 1000) x = 0;
r.x = x;
diff --git a/test/Makefile.os2 b/test/Makefile.os2
index e238af4..2ea51b0 100644
--- a/test/Makefile.os2
+++ b/test/Makefile.os2
@@ -29,3 +29,3 @@ TARGETS = testatomic.exe testdisplayinfo.exe testbounds.exe testdraw2.exe &
testsurround.exe testyuv.exe testgl2.exe testvulkan.exe testnative.exe &
- testautomation.exe
+ testautomation.exe #testvideocapture.exe
|
|
@sezero I think this is fixed. Do you receive also the notification the ci build failed for this branch ? |
Yes, CI seems fixed.
OK |
| * | ||
| * \sa SDL_VideoCaptureReleaseFrame | ||
| */ | ||
| extern DECLSPEC int SDLCALL SDL_VideoCaptureAcquireFrame(SDL_VideoCaptureDeviceID id, SDL_VideoCaptureFrame *frame, Uint64 *ticks); |
There was a problem hiding this comment.
I've just been looking at the header, I haven't looked at implementation. One important distinction between different video capture APIs is blocking vs non blocking. I assume if you call this function and the webcam hasn't "generated" a new image, it will block until it can give you a new frame?
There was a problem hiding this comment.
no, this is a non blocking API. that may change.
But making a blocking api is on top is easier.
I've updated the header
There was a problem hiding this comment.
That's cool, I think a non blocking API is more user friendly.
So it might return the same frame pixel data on multiple calls?
I don't see any header updates on this, I'm thinking it should explicitly say it doesn't block if there isn't a new frame available, it does X.
pygame.camera has a query_image() function that returns True if a new frame is ready. That seems very helpful for a non blocking api, you can only UpdateTexture or convert to a surface when needed.
| * | ||
| * \sa SDL_GetNumVideoCaptureDevices | ||
| */ | ||
| extern DECLSPEC const char * SDLCALL SDL_GetVideoCaptureDeviceName(int index); |
There was a problem hiding this comment.
I love to see it! Seems like lots of video capture APIs don't provide a way to get the device name. On Linux this is probably just /dev/video, but on Windows or Mac this could be nice and descriptive.
There was a problem hiding this comment.
yes..
on ios this is very verbose. on linux: /dev/video. on android just number ...
maybe this can be improved
There was a problem hiding this comment.
I don't have any ideas to make this better, I just wanted to give it some appreciation 😄 (Feel free to resolve)
Maybe the header should specify UTF8 encoded string, which I assume is what you're using.
| #define SDL_VIDEO_CAPTURE_TYPE_COMPRESSED 1 | ||
| #define SDL_VIDEO_CAPTURE_TYPE_EMULATED 2 |
There was a problem hiding this comment.
How does this work? Every frame format is either compressed or emulated?
I assume if a frame format is in a compressed format, an uncompressed format needs to be emulated. If you have video "compressed" in H263, you would want to emulate that as RGBA32, for instance.
There was a problem hiding this comment.
webcam can output a jpeg frame (compressed) or RGBA (emulated).
this is just some info about format that the driver give (on android I think)
maybe this could be removed if not helpfull
There was a problem hiding this comment.
I found the flags in the V4l2 docs: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-enum-fmt.html
I think these are separate things
You can have something compressed and not emulated
You can have something compressed and emulated
I don't think this API is planning to allow people to get compressed formats, since one specifies things in SDL_PixelFormats.
So the useful bit of information is whether the format you're looking at is "native" or "emulated," where "native" is better for performance, theoretically.
It's strange to me that V4l2 allows you to enumerate emulated video types like this. I have some experience with Microsoft Media Foundation, and I am the author of the pygame.camera backend for Windows. In MSMF, you do the emulation yourself using "Media Foundation Transforms." These MFTs, as they are called (no relation to NFTs), give out very little guarantees. So an MSMF function of this function would have to create an MFT of [webcam format] and then try to set the output format to everything in [formats desired] and add the ones that worked to a list. Sorry, that's kind of a ramble-- this can be ported to MSMF, it's just less convenient than the V4L2 API here.
|
I've been chewing on all the ways to enumerate things for a couple days, it just seemed odd to me and I needed to think about why. It works like this: The indentation shows the "dependency chain" of the functions. You need a camera, then a format the camera supports, then a frame size. Looking at how this works, and given the fact that this API returns pixels instead of a Surface, and it makes me think this is meant for use with Textures, with LockTexture or UpdateTexture. Because with that, the user is looking for a valid webcam configuration in a pixelformat supported by the Renderer. Tell me if I'm assuming this all wrong. 😄 Here's an alternative idea- Maybe these VideoCaptureSpecs should be sorted so the highest resolution modes are first. |
|
@Starbuck5 this is still a work in progress. things are changing while trying on backend. but currently it starts This can be done progressively:enumerate device / format / sizes. start capture. |
|
autotools need the equivalent changes made in cmake. Untested patch to diff --git a/configure.ac b/configure.ac
index 1d96004..b18d214 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2189,13 +2189,22 @@ CheckCOCOA()
#import <Cocoa/Cocoa.h>
]],[])], [have_cocoa=yes],[])
AC_MSG_RESULT($have_cocoa)
- CFLAGS="$save_CFLAGS"
if test x$have_cocoa = xyes; then
AC_DEFINE(SDL_VIDEO_DRIVER_COCOA, 1, [ ])
SOURCES="$SOURCES $srcdir/src/video/cocoa/*.m"
SUMMARY_video="${SUMMARY_video} cocoa"
have_video=yes
fi
+ AC_MSG_CHECKING(for CoreMedia framework)
+ have_coremedia=no
+ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
+ #import <AVFoundation/AVFoundation.h>
+ #import <CoreMedia/CoreMedia.h>
+ ]],[])], [have_coremedia=yes],[])
+ CFLAGS="$save_CFLAGS"
+ if test x$have_coremedia = xyes; then
+ SOURCES="$SOURCES $srcdir/src/video/SDL_video_capture.m"
+ fi
fi
}
@@ -4218,12 +4227,14 @@ *-ios-*)
SOURCES="$SOURCES $srcdir/src/video/uikit/*.m"
SUMMARY_video="${SUMMARY_video} uikit"
have_video=yes
+ SOURCES="$SOURCES $srcdir/src/video/SDL_video_capture.m"
EXTRA_LDFLAGS="$EXTRA_LDFLAGS -lm -liconv -lobjc"
EXTRA_LDFLAGS="$EXTRA_LDFLAGS -Wl,-framework,AVFoundation"
EXTRA_LDFLAGS="$EXTRA_LDFLAGS -Wl,-framework,AudioToolbox"
EXTRA_LDFLAGS="$EXTRA_LDFLAGS -Wl,-framework,CoreAudio"
EXTRA_LDFLAGS="$EXTRA_LDFLAGS -Wl,-framework,CoreGraphics"
EXTRA_LDFLAGS="$EXTRA_LDFLAGS -Wl,-framework,CoreMotion"
+ EXTRA_LDFLAGS="$EXTRA_LDFLAGS -Wl,-framework,CoreMedia"
EXTRA_LDFLAGS="$EXTRA_LDFLAGS -Wl,-framework,Foundation"
EXTRA_LDFLAGS="$EXTRA_LDFLAGS -Wl,-framework,GameController"
EXTRA_LDFLAGS="$EXTRA_LDFLAGS -Wl,-framework,OpenGLES"
@@ -4323,6 +4334,9 @@ *-*-darwin* )
# The Mac OS X platform requires special setup.
EXTRA_LDFLAGS="$EXTRA_LDFLAGS -lobjc"
EXTRA_LDFLAGS="$EXTRA_LDFLAGS -Wl,-framework,CoreVideo"
+ if test x$have_coremedia = xyes; then
+ EXTRA_LDFLAGS="$EXTRA_LDFLAGS -Wl,-framework,CoreMedia"
+ fi
EXTRA_LDFLAGS="$EXTRA_LDFLAGS -Wl,-framework,Cocoa"
EXTRA_LDFLAGS="$EXTRA_LDFLAGS -Wl,-framework,Carbon"
EXTRA_LDFLAGS="$EXTRA_LDFLAGS -Wl,-framework,IOKit" |
|
@sezero Updated thanks ! Maybe we also need a true macosx tester with a webcam ! |
|
Did some build testing using autotools (cross-compile on Linux).
Here is the patch file I generated: patch_5477a.txt
(EDIT: a bad patch was attached, now corrected.) |
|
I've applied the patch 5477a.txt |
|
OK, either way the same changes are in. |
|
A simple attempt to build on MacOS (./configure, make) failed. |
Did you run |
And here is a patch that just does that for you, for convenience: patch_configure.txt |
|
This is probably my inexperience in C that I can't resolve these. I didn't run autogen, but after I did I was able to configure, make, and make install the branch of SDL. I now get problems trying to compile the test program: |
That's because @1bsyl hasn't integrated the new capture apis into |
If you want to test things, do: And then build SDL and install. Then testvideocapture should link properly. |
|
I can build it now! It doesn't detect my camera. I'm on a macbook pro with an integrated webcam. When I click devices, it says number of devices is 0. It's probably because of the DiscoverySession thing, that's new in MacOS 10.15, and I'm running 10.13. |
Great!
That part is for Sylvain |
|
@sezero should I apply the previous patch_configure.txt ? @Starbuck5 instead of: return devices;would do something: if ([devices count] > 0) {
return devices;
} else {
AVCaptureDevice *captureDevice = [AVCaptureDevice defaultDeviceWithMediaType:AVMediaTypeVideo];
if (captureDevice == nil) {
return devices;
} else {
NSArray<AVCaptureDevice *> default_device = @[ captureDevice ];
return default_device;
}
}
(this is un-tested..) |
4e3154e to
2c077d0
Compare
yes, I had a patch at first, with zero copy integration (ryan suggested to remove this). |
Let's come back to that? We'll definitely want it, but we'll probably need something like the hardware surface support in testffmpeg.c to display them and that is a good follow-on to this PR. |
ok, I'll see that next week ! |
I would probably get this PR merged before working on hardware surface support. We'll also want it for Windows as well. |
9a689d9 to
fb53dc3
Compare
f9768e8 to
8a4f88e
Compare
f60a242 to
f2be527
Compare
|
@libsdl-org/a-team, any other comments before this is merged? |
madebr
left a comment
There was a problem hiding this comment.
I've got a few non-blocking questions.
| * | ||
| * \sa SDL_GetNumVideoCaptureFormats | ||
| */ | ||
| extern DECLSPEC int SDLCALL SDL_GetVideoCaptureFormat(SDL_VideoCaptureDevice *device, |
There was a problem hiding this comment.
Would returning an array of struct SDL_VideoCaptureFormat be a nicer API instead of a combination of SDL_GetVideoCaptureFormat and SDL_GetNumVideoCaptureFormats?
You're also returning an array that should be freed with SDL_GetVideoCaptureDevices.
There was a problem hiding this comment.
yes, this is a possibility.
format is: SDL_PixelFormatEnum
but there are also sometimes unknow eg: MPEG compressed frame.
(per format, there is a list of framesize available)
| * Non blocking API. If there is a frame available, frame->num_planes is non 0. | ||
| * If frame->num_planes is 0 and returned code is 0, there is no frame at that time. |
There was a problem hiding this comment.
Is polling this function the only way to know a new frame is available?
There was a problem hiding this comment.
yes, it's a non-blocking function, it checks if a frame get received
| * Non blocking API. If there is a frame available, frame->num_planes is non 0. | ||
| * If frame->num_planes is 0 and returned code is 0, there is no frame at that time. | ||
| * | ||
| * After used, the frame should be released with SDL_ReleaseVideoCaptureFrame |
There was a problem hiding this comment.
Can multiple SDL_VideoCaptureFrames be used simultaneously? Or will this function fail until the previous frmame is released?
There was a problem hiding this comment.
I'd say yes. because there several buffer available internally.
but it may also depend on the hardware, which could prevent from acquiring a new frame it the first one haven't been release.
There was a problem hiding this comment.
edit:
the generic layer, doesn't limit the number of buffer.
it's the video capture back-end that can push as many frame as it want/can.
for linux, the backend is hard-coded at 8 buffers.
if the user doesn't release the 8 frame, the acquisition is stucked / lacking free buffers
| extern DECLSPEC int SDLCALL SDL_StopVideoCapture(SDL_VideoCaptureDevice *device); | ||
|
|
||
| /** | ||
| * Use this function to shut down video_capture processing and close the video_capture device. |
There was a problem hiding this comment.
What happens if a SDL_VideoCaptureFrame is not released prior to closing the capture device?
Does SDL_ReleaseVideoCaptureFrame after SDL_CloseVideoCapture still work (and not leak)?
There was a problem hiding this comment.
yes, good point...
currently SDL_ReleaseVideoCaptureFrame need the "device", so a releaseFrame won't work.
maybe for some driver, it's not possible to use the frame after the device is closed.
for some other maybe it would be possible.
We could for the release all frame in the CloseDevice (or StopDevice?)
or prevent a CloseDevice if some frame a not released ?
also. frame that are recorded but not acquired would also need to be release :)
-> so probably, best is to release all frame (acquirer or not) in CloseDevice
There was a problem hiding this comment.
(btw CloseDevice invalidates the 'device', so SDL_ReleaseVideoCaptureFrame would do invalid memory read/write)
There was a problem hiding this comment.
ok, I am wrong. here.
when a frame is acquired, it has to be release before closing the device
- on android, it's an AImage, so we could release the image anyway
- but on linux, we need the file descriptor of the device
There was a problem hiding this comment.
(but we still need to release the non acquired frames!!) so the commit is ok
There was a problem hiding this comment.
I've added a comment in the public header, for ReleaseFrame()
6b3ec97 to
987edfa
Compare
|
so still maybe:
|
2814f33 to
2d21db5
Compare
|
I'm thinking I'd like to move away from the ALLOW_ANY_CHANGE flag thing, like we did for the audio subsystem, but I'm also thinking I'd like to iterate on that in main, so @1bsyl doesn't have to manage this in a branch any longer. So let's resolve whatever else is pending here, merge, and deal with those sort of tweaks where it's easier for all of us to make changes and Sylvain doesn't have to keep merging a PR with the latest. |
2d21db5 to
56073f7
Compare
|
yet, another detach-merge-commit-rebase-push :) |
|
Merged, thank you! |
I thought it could be interesting to add a SDL Video Capture API (was also suggested in #4)
This only provides a set of Open/Start/Stop, Get/Release Frame + query format/size features.
With implementation for
there is a test program (testvideocapture)
to build, you need to run first
./src/dynapi/gendynapi.py