GNOME Bugzilla – Bug 612244
v4l2sink updates
Last modified: 2012-10-23 16:44:05 UTC
Created attachment 155584 [details] Improve behavior for shared buffers Various updates for v4l2/v4l2sink. Rebased on latest git. Could someone apply after tree is un-frozen?
Created attachment 155585 [details] [review] don't render preroll buffers
Created attachment 155586 [details] [review] clear flags before reusing buffer from buffer pool
Created attachment 155587 [details] [review] Add support for omap24xxvout driver
Created attachment 155589 [details] [review] Add support for omap_vout driver
Created attachment 155590 [details] [review] Add support for blocking dequeue
Created attachment 155591 [details] [review] add "min-queued-bufs" property
Created attachment 155592 [details] [review] bufferpool should be destroyed at end up playback
Created attachment 155594 [details] re-enable x-overlay support
Created attachment 155595 [details] [review] add properties to control crop
Created attachment 155596 [details] fix race condition
Created attachment 155597 [details] [review] special handling for cases gst_buffer_make_metadata_writable()
Comment on attachment 155587 [details] [review] Add support for omap24xxvout driver Should this and the next patch be somehow conditional with a #ifdef OMAP somehow?
(In reply to comment #12) > (From update of attachment 155587 [details] [review]) > Should this and the next patch be somehow conditional with a #ifdef OMAP > somehow? It could be.. but I figured if the majority of devices using v4l2sink were OMAP, then it might as well work out of the box. I could add an --enable-omap or something like that to configure.ac, but that seemed overkill.
(In reply to comment #13) > (In reply to comment #12) > > (From update of attachment 155587 [details] [review] [details]) > > Should this and the next patch be somehow conditional with a #ifdef OMAP > > somehow? > > It could be.. but I figured if the majority of devices using v4l2sink were > OMAP, then it might as well work out of the box. I could add an --enable-omap > or something like that to configure.ac, but that seemed overkill. Okay, I was just wondering how to clearly mark such hardware specific workarounds, so that they are manageable in the long run.
(In reply to comment #14) > (In reply to comment #13) > > (In reply to comment #12) > > > (From update of attachment 155587 [details] [review] [details] [details]) > > > Should this and the next patch be somehow conditional with a #ifdef OMAP > > > somehow? > > > > It could be.. but I figured if the majority of devices using v4l2sink were > > OMAP, then it might as well work out of the box. I could add an --enable-omap > > or something like that to configure.ac, but that seemed overkill. > > Okay, I was just wondering how to clearly mark such hardware specific > workarounds, so that they are manageable in the long run. I'm open to suggestions.. I could mark them w/ a specific comment tag or something like this? so far I was happy to manage to keep the work-arounds limited to a single function ;-)
(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #13) > > > (In reply to comment #12) > > > > (From update of attachment 155587 [details] [review] [details] [details] [details]) > > > > Should this and the next patch be somehow conditional with a #ifdef OMAP > > > > somehow? > > > > > > It could be.. but I figured if the majority of devices using v4l2sink were > > > OMAP, then it might as well work out of the box. I could add an --enable-omap > > > or something like that to configure.ac, but that seemed overkill. > > > > Okay, I was just wondering how to clearly mark such hardware specific > > workarounds, so that they are manageable in the long run. > > I'm open to suggestions.. I could mark them w/ a specific comment tag or > something like this? > > so far I was happy to manage to keep the work-arounds limited to a single > function ;-) fwiw, these two patches (omap24xxvout and omap_vout driver support) could be moved to a different bugzilla ticket with some minor rebasing if this is preferred. I'm still interested if there is some precedent for work-arounds for various different existing drivers in other code? I'd kinda like it for things to work out-of-the-box for people on embedded platform, but I'm not really sure how to test for which driver it will be in configure script..
(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #14) > > > (In reply to comment #13) > > > > (In reply to comment #12) > > > > > (From update of attachment 155587 [details] [review] [details] [details] [details] [details]) > > > > > Should this and the next patch be somehow conditional with a #ifdef OMAP > > > > > somehow? > > > > > > > > It could be.. but I figured if the majority of devices using v4l2sink were > > > > OMAP, then it might as well work out of the box. I could add an --enable-omap > > > > or something like that to configure.ac, but that seemed overkill. > > > > > > Okay, I was just wondering how to clearly mark such hardware specific > > > workarounds, so that they are manageable in the long run. > > > > I'm open to suggestions.. I could mark them w/ a specific comment tag or > > something like this? > > > > so far I was happy to manage to keep the work-arounds limited to a single > > function ;-) > > fwiw, these two patches (omap24xxvout and omap_vout driver support) could be > moved to a different bugzilla ticket with some minor rebasing if this is > preferred. > > I'm still interested if there is some precedent for work-arounds for various > different existing drivers in other code? I'd kinda like it for things to work > out-of-the-box for people on embedded platform, but I'm not really sure how to > test for which driver it will be in configure script.. I've rebased on very latest gst-plugins-good, and also moved the two omap specific patches to last (so you could more easily apply the other patches without the omap specific patches if that is preferred). There is also the v4l2 support for norm property on that branch. These can be found here: http://gitorious.org/robclark-gstreamer/gst-plugins-good/commits/rebase_v4l2_patches_20100329 I'm not sure if you'd like me to attach all the new patches. And/or create a new bugzilla ticket for the v4l2 norm patch.
Created attachment 157864 [details] [review] Improve behavior for shared buffers
Created attachment 157865 [details] [review] don't render preroll buffers
Created attachment 157866 [details] [review] clear flags before reusing buffer from buffer pool
Created attachment 157867 [details] [review] Add support for blocking dequeue
Created attachment 157868 [details] [review] add "min-queued-bufs" property
Created attachment 157869 [details] [review] bufferpool should be destroyed at end of playback
Created attachment 157870 [details] [review] re-enable x-overlay support
Created attachment 157871 [details] [review] add properties to control crop
Created attachment 157872 [details] [review] fix race condition
Created attachment 157873 [details] [review] special handling for cases gst_buffer_make_metadata_writable()
(In reply to comment #17) > (In reply to comment #16) > > (In reply to comment #15) > > > (In reply to comment #14) > > > > (In reply to comment #13) > > > > > (In reply to comment #12) > > > > > > (From update of attachment 155587 [details] [review] [details] [details] [details] [details] [details]) > > > > > > Should this and the next patch be somehow conditional with a #ifdef OMAP > > > > > > somehow? > > > > > > > > > > It could be.. but I figured if the majority of devices using v4l2sink were > > > > > OMAP, then it might as well work out of the box. I could add an --enable-omap > > > > > or something like that to configure.ac, but that seemed overkill. > > > > > > > > Okay, I was just wondering how to clearly mark such hardware specific > > > > workarounds, so that they are manageable in the long run. > > > > > > I'm open to suggestions.. I could mark them w/ a specific comment tag or > > > something like this? > > > > > > so far I was happy to manage to keep the work-arounds limited to a single > > > function ;-) > > > > fwiw, these two patches (omap24xxvout and omap_vout driver support) could be > > moved to a different bugzilla ticket with some minor rebasing if this is > > preferred. > > > > I'm still interested if there is some precedent for work-arounds for various > > different existing drivers in other code? I'd kinda like it for things to work > > out-of-the-box for people on embedded platform, but I'm not really sure how to > > test for which driver it will be in configure script.. > > I've rebased on very latest gst-plugins-good, and also moved the two omap > specific patches to last (so you could more easily apply the other patches > without the omap specific patches if that is preferred). > > There is also the v4l2 support for norm property on that branch. These can be > found here: > > http://gitorious.org/robclark-gstreamer/gst-plugins-good/commits/rebase_v4l2_patches_20100329 > > I'm not sure if you'd like me to attach all the new patches. And/or create a > new bugzilla ticket for the v4l2 norm patch. I've pushed latest to here: http://gitorious.org/robclark-gstreamer/gst-plugins-good/commits/rebase_v4l2_patches_20100404 I've removed the two omap v4l2 driver specific patches.. I'll attach them and the v4l2-norm support patch on different tickets
After looking at these patches, my general feeling is that we should move v4l2sink into gst-plugins-bad at the next opportunity (ie. after the upcoming -good release in ca. two weeks).
(In reply to comment #29) > After looking at these patches, my general feeling is that we should move > v4l2sink into gst-plugins-bad at the next opportunity (ie. after the upcoming > -good release in ca. two weeks). I've no objection to moving v4l2sink into -bad... but it shares a lot of code in common with v4l2src and I'd rather not cut/paste all that code too.. I'm not sure if there is a reasonable way to handle that?
would it make sense to have a gst-plugin-good/gst-libs/sys/v4l2 with all the v4l2 helpers? That could also help to prototype gstreamer wrapper for upcomming v4l2 memory to meory operations (filters, codecs) in plugins-bad.
Could we please not put support libraries in every plugins package? There's a lot of changes necessary both inside GStreamer and outside (think packaging) if -bad suddenly started depending on -good, too. Can't we develop all the new code inside the current plugin or put the support libs into -base? (Note to self: It might make sense to have separate libs and plugin packages in 1.0 just for convenience.)
What exactly is the code that needs to be shared? Which files? How many lines? I don't think having some code copy'n'pasted is a big problem tbh, and it's not code that's touched very often anyway. The only reason the code in the v4l2 plugin still exists in the current form is because it was made non-experimental too early by someone without too much knowledge about why it was still experimental, which limited the refactoring that could be done (but was planned originally). Almost everyone I know who's hacked on v4l2src thinks that all the abstraction and indirection going on here is a disaster and doesn't help maintainability or hackability. No supporting library please.
(In reply to comment #33) > What exactly is the code that needs to be shared? Which files? How many lines? > > I don't think having some code copy'n'pasted is a big problem tbh, and it's not > code that's touched very often anyway. The only reason the code in the v4l2 > plugin still exists in the current form is because it was made non-experimental > too early by someone without too much knowledge about why it was still > experimental, which limited the refactoring that could be done (but was planned > originally). Almost everyone I know who's hacked on v4l2src thinks that all the > abstraction and indirection going on here is a disaster and doesn't help > maintainability or hackability. > > No supporting library please. fwiw: 631 1991 18109 gstv4l2bufferpool.c 97 400 3648 gstv4l2bufferpool.h 75 287 2526 gstv4l2.c 106 337 3061 gstv4l2colorbalance.c 104 387 5892 gstv4l2colorbalance.h 2120 6567 63282 gstv4l2object.c 239 890 10613 gstv4l2object.h 1005 3109 32368 gstv4l2sink.c 90 313 2591 gstv4l2sink.h 1015 3018 28656 gstv4l2src.c 99 307 2565 gstv4l2src.h 364 982 9991 gstv4l2tuner.c 198 723 12047 gstv4l2tuner.h 104 325 2916 gstv4l2vidorient.c 117 508 8916 gstv4l2vidorient.h 359 1036 9622 gstv4l2xoverlay.c 66 263 2998 gstv4l2xoverlay.h 858 2562 25613 v4l2_calls.c 145 561 5145 v4l2_calls.h 443 1362 12746 v4l2src_calls.c 45 215 1733 v4l2src_calls.h 8280 26143 265038 total of this bufferpool, colorbalance, v4l2object, tuner, vidorient, xoverlay (possibly) and v4l2_calls is shared at least to some extent by v4l2src and v4l2sink... bufferpool, v4l2object, and v4l2_calls would probably be shared by memory to memory elements. There is more code that is shared than not.
(In reply to comment #33) > What exactly is the code that needs to be shared? Which files? How many lines? > > I don't think having some code copy'n'pasted is a big problem tbh, and it's not > code that's touched very often anyway. The only reason the code in the v4l2 > plugin still exists in the current form is because it was made non-experimental > too early by someone without too much knowledge about why it was still > experimental, which limited the refactoring that could be done (but was planned > originally). Almost everyone I know who's hacked on v4l2src thinks that all the > abstraction and indirection going on here is a disaster and doesn't help > maintainability or hackability. > > No supporting library please. one idea... maybe it is time for v4l2src2.. ie. copy v4l2 plugin into -bad, rename v4l2src to v4l2src2 temporarily, as to not conflict with v4l2src in -good, and then hack away.. It would give some time to mature v4l2sink, implement other v4l2 related elements (and I have some potential interest in v4l2 filter elements too), do whatever refactoring is needed in v4l2src, etc, etc. This avoids support libs, and introducing cross plugin-package dependencies. Thoughts?
Rob, what about doing the cleanups in 0.11?
v4l2sink in 1.0 looks like it has been radically rewritten, so let's close this as OBSOLETE. Please re-open or file a new bug for any updates based on 1.0 (where lots of this should be easier to implement).