GNOME Bugzilla – Bug 743326
flvdemux: create pads as soon as we can.
Last modified: 2017-01-16 14:42:33 UTC
Otherwise the flow combiner could get confused thus: 1) Have an application that only links video pads 2) The demuxer sees audio tags first 3) It creates the pad and adds it to the flow combiner 4) Pushing returns NOT_LINKED 5) The flow combiner only knows the audio pad, returns NOT_LINKED too 6) Streaming stops, internal data error yada yada Reproduce with: ges-launch-1.0 /path/to/flv 0 5 (GES creates one uridecodebin per stream type) This patch creates the pads when parsing the header, but as it seems some flv files might have headers that lie with respect to the streams they contain, we still make sure to create the pad in parse_tag_video and parse_tag_audio.
Created attachment 295138 [details] [review] flvdemux: create pads as soon as we can.
I see the bug and we should fix it. How was this handled before flowcombiner? What would happen if the headers advertise Video + Audio but we never get data for one of them? It seems like that pad would never be exposed and would prevent a NOT_LINKED return as it would always be on OK. Maybe we want to destroy this pad it if isn't exposed after some time? Another solution would be to check if we have emitted no-more-pads after a push and ignore NOT_LINKED returns until we get no-more-pads but I'm ok with your proposal, too.
(In reply to comment #2) > I see the bug and we should fix it. Indeed :) > How was this handled before flowcombiner? The fields audio_linked and video_linked were used. > What would happen if the headers advertise Video + Audio but we never get data > for one of them? It seems like that pad would never be exposed and would > prevent a NOT_LINKED return as it would always be on OK. That's what I figure will happen, I don't have such broken flvs, but there's a comment in the code saying it can happen, unfortunately that's only a comment with no reference. > Maybe we want to > destroy this pad it if isn't exposed after some time? That seems wrong, except if something in the spec allowed us to make sure there could be no chunks of a certain type after X chunks, but I don't think it is the case. > Another solution would be to check if we have emitted no-more-pads after a push > and ignore NOT_LINKED returns until we get no-more-pads but I'm ok with your > proposal, too. Yeah, I think I mentioned that to you the other day, can be a bit more involved but that would definitely improve flow combiner for everyone so there's that. In the case of flvdemux though, if the "lying header" bug happens, no-more-pads might never get emitted. That's however a different issue that already exists and could certainly be addressed in some way.
It's common in RTMP that audio and video streams are dynamically added and removed (audio/video conference). I've tested the patch with the following FLVs: video-audio.flv - 10 seconds of video and 10 seconds of video+audio. https://drive.google.com/file/d/0B12AhxvnYHrATTFaTC11NXZ3dTA/view?usp=sharing audio-video.flv - 10 seconds of audio and 10 seconds of audio+video. https://drive.google.com/file/d/0B12AhxvnYHrAUWZDbVl0dml2T0U/view?usp=sharing With the patch, the pad of the late stream is added but it plays only for about a second and stops (instead of 10 seconds). What about removing a stream? It can probably be in the scope of this patch too.
(In reply to Ben from comment #4) > It's common in RTMP that audio and video streams are dynamically added and > removed (audio/video conference). > > I've tested the patch with the following FLVs: > video-audio.flv - 10 seconds of video and 10 seconds of video+audio. > https://drive.google.com/file/d/0B12AhxvnYHrATTFaTC11NXZ3dTA/view?usp=sharing > > audio-video.flv - 10 seconds of audio and 10 seconds of audio+video. > https://drive.google.com/file/d/0B12AhxvnYHrAUWZDbVl0dml2T0U/view?usp=sharing > > With the patch, the pad of the late stream is added but it plays only for > about a second and stops (instead of 10 seconds). > Curious, what happens without the patch by the way? > What about removing a stream? It can probably be in the scope of this patch > too. Nope that's not in the scope of that patch, the purpose was just to avoid flowcombiner getting confused when only one pad is linked. We should merge that patch by the way!
(In reply to Mathieu Duponchelle from comment #5) > (In reply to Ben from comment #4) > > It's common in RTMP that audio and video streams are dynamically added and > > removed (audio/video conference). > > > > I've tested the patch with the following FLVs: > > video-audio.flv - 10 seconds of video and 10 seconds of video+audio. > > https://drive.google.com/file/d/0B12AhxvnYHrATTFaTC11NXZ3dTA/view?usp=sharing > > > > audio-video.flv - 10 seconds of audio and 10 seconds of audio+video. > > https://drive.google.com/file/d/0B12AhxvnYHrAUWZDbVl0dml2T0U/view?usp=sharing > > > > With the patch, the pad of the late stream is added but it plays only for > > about a second and stops (instead of 10 seconds). > > > > Curious, what happens without the patch by the way? Without the patch playbin plays the first stream and ignores the late stream. > > > What about removing a stream? It can probably be in the scope of this patch > > too. > > Nope that's not in the scope of that patch, the purpose was just to avoid > flowcombiner getting confused when only one pad is linked. We should merge > that patch by the way!
Thanks for taking the time to report this. This particular bug has already been reported into our bug tracking system, but please feel free to report any further bugs you find. *** This bug has been marked as a duplicate of bug 764260 ***