After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 622500 - [v4l2sink] destroy open buffers when changing to NULL
[v4l2sink] destroy open buffers when changing to NULL
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
unspecified
Other Linux
: Normal blocker
: 0.10.24
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-06-23 15:20 UTC by mgr
Modified: 2010-07-06 14:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[gstv4l2sink] destroy open buffers when changing to NULL (1.67 KB, patch)
2010-06-23 15:20 UTC, mgr
none Details | Review

Description mgr 2010-06-23 15:20:08 UTC
Created attachment 164405 [details] [review]
[gstv4l2sink] destroy open buffers when changing to NULL

In the case we change the State from READY_TO_NULL the open buffers
still hold a open dup filedescriptor to the device, therefore the device
release function will not be called and the device probably answer with
-EBUSY when we reopen it at transition NULL_TO_READY.
Comment 1 Rob Clark 2010-06-23 15:50:50 UTC
fyi, this is also solved by one of the patches on #612244

I'll refresh those patches over the weekend of before.. but maybe would be a good idea to get them merged.  I'm not sure if this ticket should be marked as a duplicate?
Comment 2 Tim-Philipp Müller 2010-06-25 18:05:22 UTC
Some of those patches in the other bug look mergeable, others look rather dubious or make me think v4l2sink should be moved to -bad really.

I might to apply some of these patches after the first pre-release, but I don't really want to block on this now.

I'm not merging this patch because it doesn't conform to our indenting guidelines (please don't disable the git pre-commit hook and/or install gnu-indent), and I can't be bothered to clean it up now.

And the patch from the other bug is missing the gst_v4l2_object_stop_streaming() bit and also contains a gratuitious(?) STATE_OFF => STATE_PENDING_STREAMON change.
Comment 3 Tim-Philipp Müller 2010-07-06 10:12:51 UTC
Due to lack of response from original reporter I've only committed a minimal change now, ie. the buffer_pool_destroy(), and IIRC Rob expressed doubts about the stop_streaming bit.

I assume the STATE_OFF => STATE_PENDING_STREAMON change in the other state transition (in the other patch) wasn't really related or required.
Comment 4 mgr 2010-07-06 10:33:14 UTC
Sorry, i have been busy last days on hacking the kernel.
I think the not pretty indented "gst_v4l2_object_stop_streaming(v4l2sink->v4l2object);" was just added as mentioned in the TODO comment above. At this moment i don't have lot time to check on this again. But it seems to me that the applied patch should still release the locked filedescriptor in the bufferpool.

So, thanks a lot.
mgr
Comment 5 Tim-Philipp Müller 2010-07-06 10:45:11 UTC
Ok, thanks for the follow-up.
Comment 6 Rob Clark 2010-07-06 14:03:57 UTC
(In reply to comment #3)
> 
> I assume the STATE_OFF => STATE_PENDING_STREAMON change in the other state
> transition (in the other patch) wasn't really related or required.

Just fwiw, the STATE_OFF => STATE_PENDING_STREAMON is to properly handle state change sequence like: PAUSED->READY->PAUSED..

In this case, the buffers are still allocated, but a VIDIOC_STREAMON is needed in order to start playing again.

so maybe not exactly the same issue as the need to destroy buffers when going back to NULL state... but a related state change issue.

The _stop_streaming() change should definitely be a different patch... because I think a few other things are needed to handle renegotiating caps properly.  (It's something on my TODO list, but haven't gotten too yet.. feel free to file a bug and assign to me for proper handling of caps renegotiation.)