GNOME Bugzilla – Bug 577843
input-selector causes problems with DVD menus in playbin2
Last modified: 2009-04-23 10:10:55 UTC
playbin2 breaks some forms of DVD menu navigation, such as the chapters menu on back to the future. It seems the problem is that segment updates sent on the video and subpicture pads from rsndvdbin do not reach the dvdspu element (blocked in selectors somewhere?). Without them, there's no way for some forms of 'still frame' menus to redraw the button highlights when the mouse moves over a different button.
Looking briefly at the code in inputselector - it doesn't handle newsegment events with update=TRUE properly at all - in all cases, it puts the segment information into a GstSegment, and then outputs a new segment with update=FALSE whenever the next buffer comes in. This means it never sends segment closes (except when switching pads), and will break sparse stream playback as per the sparse-streams doc in docs/design.
I ran into this in songbird a while back. I have a hacky patch for it locally; somewhere on my todo-list is figuring this stuff up properly and upstreaming it. If you get to it first... great.
Changing module and bug title accordingly.
Created attachment 133076 [details] [review] The hacky patch I referred to earlier
Created attachment 133120 [details] [review] alternative patch This patch is more like what I have in mind. The problem I need to solve is ensuring that segments are forwarded even when there's no data. For sparse streams - DVD subpictures, or still frames with an audio track - there are segment updates arriving with no data in between. Mike, I'm curious if this approach covers the use case you encountered also? Can you describe it? I'd like to add some unit tests for this stuff before committing if possible.
Your patch doesn't look right to me - it still results in an update=FALSE segment being sent when it receives an update=TRUE segment. Why is it doing it that way? My case is specifically that update=TRUE new segment events MUST be forwarded as update=TRUE. (I only really tested with single-stream audio files - so the selector isn't actually doing any meaningful selecting) Details: - using playbin2 - some element sends a closed (i.e. has valid start AND stop) new-segment event. - later, said element decides it didn't actually want that segment to have a valid stop time. So it sends an update-new-segment that is the same as the original one, but with stop=GST_CLOCK_TIME_NONE - current input-selector forwards this as a new, update=FALSE, new-segment - so the old segment with the incorrect stop time is still used. - my patch makes it preserve the update flag Something like your patch might be needed too - I think we're actually seeing different problems with vaguely-related underlying causes, and we need to fix both things. I'm not entirely convinced that I understand new-segment semantics (particular for sparse streams) well enough to fix this properly.
I think you've misread my patch. It does two things: 1) If a segment with update == TRUE is received, and there's no active pad, activate the pad we're receiving the incoming segment on, and send a segment start (with update=FALSE) to kick things off, then do 2) because it's now the active pad. 2) If any newsegment (update can be either TRUE or FALSE) is received on the *active* sink pad, forward it immediately, don't defer it to the streaming thread (this is the if (!active_pad) { forward=FALSE } part of the patch). For your use case, I think downstream will receive the events it needs.
Ah, I did misread your patch - I think your general approach makes sense. Also, in a quick test, your patch seems to work for my testcase. However, I still don't get how your patch accomplishes _your_ goals. (below, line numbers are in the file with your patch applied): 1. gstinputselector:353: we activate the pad we receive _any_ event on, if there's no active pad. 2. gstinputselector:398: so now, your if (update && sel->active_sinkpad == NULL) is always false because active_sinkpad can't be NULL, so that new code is never used. Also, if we hadn't already activated the sinkpad in (1), we wouldn't forward the event: we would have set forward = FALSE at line 358. So... conceptually this looks fine, but needs some fixes? Unless I'm still missing something?
Created attachment 133146 [details] [review] new patch for input-selector segment handling Oh - you're right! I had completely missed those lines at the top of the event handler. With those in place, I can reduce my patch to the attached section, and assume that the first event (new segment with update=FALSE, perhaps) will activate the pad, and we only need to worry about subsequent newsegment events.
This version looks good to me.
Created attachment 133149 [details] [review] even simpler patch for input-selector segments Here's an even simpler patch, that simply relies on the fact that forward was set to FALSE at the top for (pad != active_pad && !select_all). Also adds a big comment. Should be functionally identical to the previous patch, as select_all mode makes every incoming event set that pad to active anyway.
Committed and pushed as 5307933825f54d3d1b92bcb1bcfb5142ca359263