[Bug 744090] mpegtsmux doesn't permit to add stream during play

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=744090

Thiago Sousa Santos <thiagossantos at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #297977|none                        |reviewed
             status|                            |

--- Comment #13 from Thiago Sousa Santos <thiagossantos at gmail.com> ---
Review of attachment 297977:
 --> (https://bugzilla.gnome.org/review?bug=744090&attachment=297977)

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.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.