GNOME Bugzilla – Bug 599469
resindvd: problems pre-rolling in the absence of audio
Last modified: 2012-01-27 11:14:52 UTC
Switching to autoconvert for handling the audio streams has introduced a regression on some DVDs. Sequences which move immediately to a still frame with no audio, or which play a long video sequence without audio no longer preroll. Previously, it was the responsibility of the 'rsnaudiomunge' element to generate a fake audio pre-roll buffer in these cases, in response to new-segment update events. Since autoconvert caches events until the first caps are set (with the first audio data buffer) and it can plug a decoder, the new-segment events don't arrive in rsnaudiomunge. One fix would be to have a property on autoconvert that makes it start with a plugged identity element, and replace it later when the audio arrives. I don't think that's desirable default behaviour for autoconvert of course. A pre-requisite for supporting that would also be having autoconvert cache segment info and inform the elements about their segment with a NewSegment update delivered after the element is plugged. I'll be producing a patch for this, unless someone can suggest a better approach?
Created attachment 146218 [details] [review] autoconvert: Cache incoming new segment events and push to children. Cache incoming new-segment info. When installing a child element, inform it of the current segment info.
Created attachment 146219 [details] [review] autoconvert: Support installing identity as the initial child Add the 'initial-identity' property, which inserts identity for at startup for event passing, and replaces it with a new child when the first buffer (and caps) actually arrives.
Created attachment 146220 [details] [review] resindvd: Set the new initial-identity property on autoconvert
Instead of adding an identity element, maybe it there should be "if (!subelement && let_buffers_through) gst_pad_push_event(srcpad, event)". And have a "let-non-buffers-through" property ? I don't think we want an identity left in there once the real elements are created.
That could work too. I might give it a try later. Is there any reason to make it a configurable option, or should it just be the default behaviour? Can you provide any test cases for autoconvert so we can make sure it continues to work as originally intended?
This change is wrong, it causes newsegment events to arrive downstream before any caps are known. This must not happen and causes problems with autopluggers (and similar elements: playsinkvideoconvert for example). Also this behaviour is now broken, see bug #665205 . Because identity (basetransform) now caches serialized events until the caps are known, the newsegment events never arrive on the srcpad of identity but is queued inside identity forever. Then autoconvert plugs the new, correct elements but no new newsegment event is sent to them that is also allowed to be pushed downstream. This has to be solved differently, for example with a preroll buffer before autoconvert.
I've reverted this change now to at least have working playback of normal DVDs again. The best way to fix this would probably be to insert an raw, empty audio buffer for prerolling somewhere in resindvdbin. Jan, where would this best be done? commit 780d774a5deaca6a87ab3905028fc32a1a2e0087 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Wed Dec 7 13:48:53 2011 +0100 rsndec: Don't use the initial-identity property on autoconvert It was removed, see bug #599469, #665205 commit dcf04269e1eab9d3a015218165f1027b1e4bce51 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Wed Dec 7 13:46:43 2011 +0100 autoconvert: Remove the initial-identity property from autoconvert Initially creating an identity element to forward serialized events downstream before any caps are known is broken behaviour. Serialized events should only be forwarded downstream if the caps are already known, otherwise autopluggers and other elements using pad-blocks will fail. This behaviour also doesn't work anymore after basetransform was fixed to queue serialized events until the caps are known as a result of fixing bug #659571. See bug #599469, #665205.
Also, do you have a test menu for this by any chance, or happen to remember some titles that do?
I'm not sure where to insert the event. It might be easiest to put it in a new munge element just before autoconvert, or else to add a 'dummy' pad to the demuxer just for the 'no audio' case.
It seems that "Inglourious Basterds" is one title where it happens. My edition has a menu to pick english/german/turkish and I don't get any menu navigation here. Getting this error. playsinkconvertbin gstplaysinkconvertbin.c:481:gst_play_sink_convert_bin_cache_converter_caps:<GstPlaySinkAudioConvert@0x1f4f220> No conversion elements In totem I can select "Go DVD Menu" and that makes it play the DVD (although duration is 0 and I can't seek).
I've got a possible patch (attached), which I'm not sure about, because though it fixes the issue (first still frame menu without audio now prerolls, and I can select menu items), I do not get any audio in the second video (the actual video), but I can't tell if that's due to the patch or not because it did not preroll before :) The patch causes the MPEG demuxer to not create a new pad for the dummy stream, which in turns prevents anything from waiting on that pad where no audio would flow. After starting the actual movie, there is an actual real audio stream, so a pad is created for the real audio, which should work fine (and it prerolls fine, video plays, but no audio after half a dozen AC3 buffers). Does anyone who knows what that dummy stream is about know if my patch is correct, and the no-audio-in-movie is unrelated (so a second bug) ?
Created attachment 205629 [details] [review] resdvdsrc: fix preroll wedge when no audio is present
Ah, I think I see what's wrong. Not adding a pad on the first movie will cause that pad not to be linked to the audio sink, and that won't be re-done when switching movies. So the patch cannot work with that system. Back to reading the code.
Created attachment 205704 [details] [review] resindvd: fix preroll on titles with no audio track
Fixed with thaytan's suggestion of adding another munge before the decoder and frobbing the template caps. Works with my test case (and the audio works fine in the next video). It's a bit of a hack, but not nearly as bad as I thought I'd end up with. Hopefully it also works on the other test case mentioned above. I'll push soonish (plus a few cleanups) unless NAK.
Excellent :) With an audiomunge in place before autoconvert, we might not need the second one at all, but I'd want to test that. It's not much overhead to leave it in place. The only thing I'd consider changing would be to add the audioconvert in first in rsnaudiodec, and then subtract audio/x-raw-float from the desired_caps - so as to avoid autoplugging any hypothetical 'Decoder' class element that appears in the future and accepts 'audio/x-raw-float' for some strange reason.
Created attachment 205823 [details] [review] resindvd: fix preroll on titles with no audio track
Both these changes work for my test case. New patch attached with those in.
Review of attachment 205823 [details] [review]: looks good
Thanks. A few small tidbits are going with it. commit 1c0ccc45f458a2a3e9dbe49cc2084fbcb3f10267 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Fri Jan 20 15:34:27 2012 +0000 resindvd: fix preroll on titles with no audio track https://bugzilla.gnome.org/show_bug.cgi?id=599469 commit 27eb76bf45112263452b7b47cecd75022d480835 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Fri Jan 20 12:25:09 2012 +0000 rsndvdsrc: fix leak commit 35c96af778507a44ef800f5b110fd6c7b8433a81 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Fri Jan 20 12:15:20 2012 +0000 resindvdbin: fix video/audio mixup in error message commit dbad02437c73b4bdb79cac5b8fcc738181db70f0 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk> Date: Fri Jan 20 10:42:21 2012 +0000 rsnaudiomunge: keep the object ref longer Since we do not get a ref to the pad, I'm not certain it's safe to drop the object and use the pad later, so hold the object ref till we're done with the pad.
GStreamer-CRITICAL **: gst_caps_intersect_full: assertion `GST_IS_CAPS (caps1)' failed Program received signal SIGTRAP, Trace/breakpoint trap.
+ Trace 229501
Thread 2848242544 (LWP 20295)
Damn, and I'm getting this too somehow. How can I have possibly managed to miss that ? :S Fixing...
Fixed. I had tested it, honest...
For ensonic: Run with rsnaudiomunge:5, and you should see the following message if audiomunge sends the dummy buffer: GST_LOG_OBJECT (munge, "Sending %u bytes (%" GST_TIME_FORMAT ") of audio data with TS %" GST_TIME_FORMAT, buf_size, GST_TIME_ARGS (fill_time), GST_TIME_ARGS (start));