On 3/11/21 2:29 PM, Derek Lesho wrote:
On 3/11/21 3:15 PM, Zebediah Figura (she/her) wrote:
On 3/10/21 1:33 PM, Derek Lesho wrote:
Since our source pad is not part of any element, gstreamer won't end up activating it directly through the state transition. Instead, if the downstream element doesn't activate the source pad into pull mode during the transition to the READY state, we activate our pad in push mode.
Signed-off-by: Derek Lesho dlesho@codeweavers.com
dlls/winegstreamer/wg_parser.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index b8dd75d5fff..67f65264a8d 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -58,7 +58,7 @@ struct wg_parser pthread_mutex_t mutex; pthread_cond_t init_cond; - bool no_more_pads, has_duration, error; + bool no_more_pads, has_duration, error, pull_mode; pthread_cond_t read_cond, read_done_cond; struct @@ -1331,9 +1331,12 @@ static gboolean src_activate_mode_cb(GstPad *pad, GstObject *parent, GstPadMode GST_DEBUG("%s source pad for parser %p in %s mode.", activate ? "Activating" : "Deactivating", parser, gst_pad_mode_get_name(mode)); + parser->pull_mode = false;
I think this statement is redundant—the field will already be initialized to zero, and I don't think we should be activated in pull-mode directly after being activated in push-mode.
Makes sense.
switch (mode) { case GST_PAD_MODE_PULL: + parser->pull_mode = activate; return TRUE; case GST_PAD_MODE_PUSH: return activate_push(pad, activate); @@ -1573,6 +1576,8 @@ static void CDECL wg_parser_disconnect(struct wg_parser *parser) pthread_mutex_unlock(&parser->mutex); gst_element_set_state(parser->container, GST_STATE_NULL); + if (!parser->pull_mode) + gst_pad_set_active(parser->my_src, 0); gst_pad_unlink(parser->my_src, parser->their_sink); gst_object_unref(parser->my_src); gst_object_unref(parser->their_sink); @@ -1628,6 +1633,8 @@ static BOOL decodebin_parser_init_gst(struct wg_parser *parser) } gst_element_set_state(parser->container, GST_STATE_PAUSED); + if (!parser->pull_mode) + gst_pad_set_active(parser->my_src, 1); ret = gst_element_get_state(parser->container, NULL, NULL, -1); if (ret == GST_STATE_CHANGE_FAILURE) {
I think this and the following should happen after the gst_element_get_state() call, right? We shouldn't spawn a thread if state change fails.
In theory that might risk a deadlock—i.e. GStreamer doesn't make guarantees that NULL->READY->PAUSED can happen without any buffers being pushed—but I don't know if it happens in practice.
Yeah, it does deadlock and I think Gstreamer specifies that the PAUSED state indicates that pipeline is prerolled.
https://gstreamer.freedesktop.org/documentation/plugin-development/basics/st...
"GST_STATE_PAUSED is ... At this point the pipeline is 'prerolled' and ready to render data immediately."
That's assuming that the sink prerolls—which most sinks do, but we don't.
For what it's worth, it isn't guaranteed (although in practice it seems to be the case) that even the transition from NULL to READY is completely synchronous. The most correct code would involve first transitioning to READY, checking the result, activating the pad, then continuing to transition to PAUSED.
Unfortunately it's not quite that simple. Pads are activated on READY->PAUSED, not on NULL->READY, and upstream pads have to be activated *after* downstream ones.
The really correct solution to that deadlock is to create our own element (or, actually, use appsrc—now that winegstreamer is split into PE and Unix parts
Yeah, I still believe GstAppSrc is the cleaner solution, as the interface is quite simple and well documented. But functionally, the manual pad mode should work just as well, it's up to you.
I don't remember what discussion we had about appsrc in the first place (if we had any?), but I think my argument was that it wasn't really a *better* fit than using getrange callbacks directly, i.e. it wasn't saving us much work (at best it would have saved us the need to separately handle push mode, but it was better to simply not implement push mode in the media source).
But that's less true now, given the way that read requests are handled at the Unix library boundary, and the combined backend interface for DirectShow and Media Foundation, and I think appsrc may be a better fit now. The "need-data"/"push-data" model looks like a better fit, that might allow us to avoid the current awkward callback marshalling, and we'd be able to get rid of push-mode handling completely.
, that option is starting to look more attractive, especially if we want to make winegstreamer compatible with WoW64 emulation.)
I'm not sure I see how this is relevant to the WoW64 emulation, if you don't mind, could you explain this?
One reason that appsrc is undesirable (which I probably didn't mention earlier) is that it always takes buffers allocated by the "app", as it were, rather than by the element calling gst_pad_get_range(). That's fine if the downstream element didn't allocate its own buffer, but if it did (and oggdemux is one major example of an element that does) appsrc will do an extra blit.
That should be avoided if possible. I'm not particularly concerned about latency—I don't think it should be a bottleneck, as long as the machine has enough cores—but I am more concerned about CPU usage, and cases where we might be CPU limited and *don't* have enough cores. Yes, it's probably premature optimization, but I'm a lot warier about removing an optimization already present than I am about introducing a new one.
Currently we fill read buffers allocated by the Unix side—either already allocated by a GStreamer element, or allocated by us using gst_buffer_new_and_alloc(). That's not compatible with WoW64 emulation (aka 32-on-64-bit support), as it requires the 32-bit PE side to write into a 64-bit pointer. Instead we would need to write into a buffer allocated from the 32-bit side, and pass that to the Unix side. But this also means that we have to do a blit.
[Note that we can in theory avoid the blit by wrapping the caller-allocated buffer in a custom GstBuffer class and passing that down to the Unix side. We still have to do a blit if reading into a downstream-allocated GstBuffer, but we can at least avoid it otherwise.]
@@ -1679,6 +1686,8 @@ static BOOL avi_parser_init_gst(struct wg_parser *parser) } gst_element_set_state(parser->container, GST_STATE_PAUSED); + if (!parser->pull_mode) + gst_pad_set_active(parser->my_src, 1); ret = gst_element_get_state(parser->container, NULL, NULL, -1); if (ret == GST_STATE_CHANGE_FAILURE) { @@ -1733,6 +1742,8 @@ static BOOL mpeg_audio_parser_init_gst(struct wg_parser *parser) gst_pad_set_active(stream->my_sink, 1); gst_element_set_state(parser->container, GST_STATE_PAUSED); + if (!parser->pull_mode) + gst_pad_set_active(parser->my_src, 1); ret = gst_element_get_state(parser->container, NULL, NULL, -1); if (ret == GST_STATE_CHANGE_FAILURE) { @@ -1787,6 +1798,8 @@ static BOOL wave_parser_init_gst(struct wg_parser *parser) gst_pad_set_active(stream->my_sink, 1); gst_element_set_state(parser->container, GST_STATE_PAUSED); + if (!parser->pull_mode) + gst_pad_set_active(parser->my_src, 1); ret = gst_element_get_state(parser->container, NULL, NULL, -1); if (ret == GST_STATE_CHANGE_FAILURE) {