GNOME Bugzilla – Bug 744090
mpegtsmux: add support for adding/removing streams at runtime
Last modified: 2018-11-03 13:30:32 UTC
When mpegtsmux is in PLAYING oder PAUSED state, I can add sink-pads, but when I activate it and put data in, I get the Error: Stream on pad sink_xxx is not associated with any program. With this patch, a new pad is added to the default, when it is not associated with any program. So it's possible to simply add streams during state PLAYING and PAUSED. diff --git a/gst/mpegtsmux/mpegtsmux.c b/gst/mpegtsmux/mpegtsmux.c index 975c926..6df698d 100644 --- a/gst/mpegtsmux/mpegtsmux.c +++ b/gst/mpegtsmux/mpegtsmux.c @@ -1117,6 +1117,25 @@ mpegtsmux_collected_buffer (GstCollectPads * pads, GstCollectData * data, return GST_FLOW_OK; } + if((best->prog) == NULL) { + GST_WARNING_OBJECT (mux, "Stream on pad %" + GST_PTR_FORMAT " was not associated with any program, adding to default program", + COLLECT_DATA_PAD (best)); + + best->prog_id = DEFAULT_PROG_ID; + best->prog = mux->programs[best->prog_id]; + + if(best->stream == NULL) { + ret = mpegtsmux_create_stream (mux, best); + if (ret != GST_FLOW_OK) { + GST_ELEMENT_ERROR (mux, STREAM, MUX, ("Stream on pad %" + GST_PTR_FORMAT " is not associated with any program", + COLLECT_DATA_PAD (best)),(NULL)); + return GST_FLOW_ERROR; + } + } + } + prog = best->prog; if (prog == NULL) goto no_program;
Thanks for the patch. Could you please create a patch in "git format-patch" format with you as author and a proper commit message (containing a description and this bug url), and then attach it to this bug report?
Created attachment 296609 [details] [review] patch
Review of attachment 296609 [details] [review]: ::: gst/mpegtsmux/mpegtsmux.c @@ +1131,3 @@ + + best->prog_id = DEFAULT_PROG_ID; + best->prog = mux->programs[best->prog_id]; Why not check the stream to program map that is used for streams created since the beginning? You can likely refactor the code used in create_streams() for that, if the program isn't present you assume the default.
You're right. Thats the better solution. Do you have example code how to use the prog-map parameter ?
You can refactor the part that uses it in create_streams to a new function and use the same to decide the program_id for a stream in create_streams or when a new pad is requested.
Yes, but at the moment I have problems to build a test code that creates more than one program. I tried it with some lines from this: https://github.com/kanongil/gst-plugins-bad/blob/master/tests/examples/mpegtsmux/mpts_test2.c and endet up with a TS with one program and 3 video streams. (and without the "was not associated" warning). So if you have any code with them I can test if the program handling is working, please give me a link. I want to test it before and after I refactored the code. At the moment I can't say if this feature is anyway working.
Unfortunatelly I don't have, did you try running with GST_DEBUG enabled for mpegts to check what happened?
Yes, I working on it. looks like the muxer didn't found the pads in prog-map. I don't know why...beeing searching...
okay its working now
Do you have an updated patch?
Created attachment 297977 [details] [review] updated patch
yes I added the new patch.
Review of attachment 297977 [details] [review]: Thanks for the update, your patch looks good, just some minor stuff to update. First, could you please commit the code and provide patches by using 'git format-patch'. It makes easier to merge to our local trees and then push. Committing the code will make sure that you've gone through our indentation hook that makes sure your code follows the expected coding guidelines. Also please use a meaningful commit message, check the git log to see other commits and try to follow the pattern. Thanks again for taking time to update your patch. ::: gst/mpegtsmux/mpegtsmux.c @@ +748,3 @@ + if (!ret) + { + GST_ELEMENT_ERROR (mux, STREAM, MUX, ("Reading program map failed. Assuming default"), (NULL)); While we are at this, GST_ELEMENT_ERROR will cause an error message to be posted and the program will stop, I don't think this should lead to an error. So I think we should just use GST_ELEMENT_WARNING (that posts a warning message to the application) or just a GST_WARNING_OBJECT that will only print at the stderr a warning (the application won't get a message). @@ +1263,2 @@ { + GST_ELEMENT_ERROR (mux, STREAM, MUX, ("Can't init stream on pad %" GST_PTR_FORMAT , COLLECT_DATA_PAD (best)), (NULL)); Any reason to change the message here? I think the init_stream_settings should already have mentioned the reason why it failed to init the stream program. I think the original message here made more sense.
Okay, first of all, there must went anything wrong. I used git format-patch. Looks like I uploaded the wrong file....sorry. ::: gst/mpegtsmux/mpegtsmux.c @@ +748,3 @@ + if (!ret) + { + GST_ELEMENT_ERROR (mux, STREAM, MUX, ("Reading program map failed. Assuming default"), (NULL)); Hmm....I copied it from the old functions. But it's a good time to think about this part of code. In my opinion, when the developer adds a program map, he want to generate more than one program. If this doesn't works, all video and audio streams are added to one program. Normally this stream is not usable as intendet and we should stop with a error because the user doesn't suppose to get a stream like this. But when we do this, maybee we don't need the following line in code: idx = DEFAULT_PROG_ID; @@ +1263,2 @@ { + GST_ELEMENT_ERROR (mux, STREAM, MUX, ("Can't init stream on pad %" GST_PTR_FORMAT , COLLECT_DATA_PAD (best)), (NULL)); Hmm...yes, the old message is better.
(In reply to Andreas Schuler from comment #14) > Okay, first of all, there must went anything wrong. I used git format-patch. > Looks like I uploaded the wrong file....sorry. > > ::: gst/mpegtsmux/mpegtsmux.c > @@ +748,3 @@ > + if (!ret) > + { > + GST_ELEMENT_ERROR (mux, STREAM, MUX, ("Reading program map > failed. Assuming default"), (NULL)); > > > Hmm....I copied it from the old functions. But it's a good time to think > about this part of code. In my opinion, when the developer adds a program > map, he want to generate more than one program. If this doesn't works, all > video and audio streams are added to one program. Normally this stream is > not usable as intendet and we should stop with a error because the user > doesn't suppose to get a stream like this. > > But when we do this, maybee we don't need the following line in code: > idx = DEFAULT_PROG_ID; Imagine you are using this to record a live event, so erroring out because of a human error (the program map wasn't properly set) would cause you to lose the recording, while just posting a warning gives the choice for the application to continue or abort. I believe being more resilient is usually better, the 'broken' mpegts can be fixed with remuxing into the desired format while erroring out could make the content lost forever. Anyway this is not part of the intent of your patch, so if you desire it to remain posting an error I will make no objection on merging your work. The discussion for error/warning here can be done as a separate issue.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/208.