GNOME Bugzilla – Bug 779453
gst-player: fix gst-player failed to load external subtitle uri
Last modified: 2017-03-09 08:32:36 UTC
set_uri_internal will free subtitle uri but this uri has not been set to playbin by set_suburi_internal
Created attachment 347027 [details] [review] gst-player: fix gst-player failed to load external subtitle uri
Review of attachment 347027 [details] [review]: ::: gst-libs/gst/player/gstplayer.c @@ +585,2 @@ /* if have suburi from previous playback then free it */ + gchar *suburi = NULL; Don't put new variable declarations into the middle of a block @@ +586,3 @@ + gchar *suburi = NULL; + g_object_get(self->playbin, "suburi", &suburi, NULL); + if (self->suburi && suburi && strcmp(self->suburi, suburi)==0) { Shouldn't self->suburi always be the same as playbin's suburi? When is it not?
(In reply to Sebastian Dröge (slomo) from comment #2) > Review of attachment 347027 [details] [review] [review]: > > ::: gst-libs/gst/player/gstplayer.c > @@ +585,2 @@ > /* if have suburi from previous playback then free it */ > + gchar *suburi = NULL; > > Don't put new variable declarations into the middle of a block > > @@ +586,3 @@ > + gchar *suburi = NULL; > + g_object_get(self->playbin, "suburi", &suburi, NULL); > + if (self->suburi && suburi && strcmp(self->suburi, suburi)==0) { > > Shouldn't self->suburi always be the same as playbin's suburi? When is it > not? When there is a suburi from previous playback.
Comment on attachment 347027 [details] [review] gst-player: fix gst-player failed to load external subtitle uri Ah I see. Then please just update the patch and I'll merge it
Created attachment 347364 [details] [review] gst-player: fix gst-player failed to load external subtitle uri
Review of attachment 347364 [details] [review]: Please explain in more detail what this fixes in the commit message. Also we don't use "Signed-off-by: ..." ::: gst-libs/gst/player/gstplayer.c @@ +586,3 @@ /* if have suburi from previous playback then free it */ + g_object_get(self->playbin, "suburi", &suburi, NULL); + if (self->suburi && suburi && strcmp(self->suburi, suburi)==0) { So we reset the suburi here if there was a previous one and it was the same as the current one? Why? Shouldn't we just always reset it to NULL, no matter if playbin or GstPlayer thought there was a previous one or not? Please also run "gst-indent" over your changes before sending patches. @@ +590,3 @@ self->suburi = NULL; g_object_set (self->playbin, "suburi", NULL, NULL); + g_free (suburi); You're leaking suburi here if suburi!=NULL but any of the other conditions is false.
(In reply to Sebastian Dröge (slomo) from comment #6) > Review of attachment 347364 [details] [review] [review]: > > Please explain in more detail what this fixes in the commit message. Also we > don't use "Signed-off-by: ..." Ok. > > ::: gst-libs/gst/player/gstplayer.c > @@ +586,3 @@ > /* if have suburi from previous playback then free it */ > + g_object_get(self->playbin, "suburi", &suburi, NULL); > + if (self->suburi && suburi && strcmp(self->suburi, suburi)==0) { > > So we reset the suburi here if there was a previous one and it was the same > as the current one? Why? Shouldn't we just always reset it to NULL, no > matter if playbin or GstPlayer thought there was a previous one or not? Yes, you are right, we just need always reset playbin's suburi to NULL when change uri. Let the application to handle the suburi change and call gst_player_set_suburi() to load new subtitle > > > Please also run "gst-indent" over your changes before sending patches. > > @@ +590,3 @@ > self->suburi = NULL; > g_object_set (self->playbin, "suburi", NULL, NULL); > + g_free (suburi); > > You're leaking suburi here if suburi!=NULL but any of the other conditions > is false.
Created attachment 347444 [details] [review] gst-player: fix gst-player failed to load external subtitle uri
Review of attachment 347444 [details] [review]: ::: gst-libs/gst/player/gstplayer.c @@ -586,3 @@ - if (self->suburi) { - g_free (self->suburi); - self->suburi = NULL; You're now not freeing self->suburi and not setting it to NULL. Apart from that this looks good now
(In reply to Sebastian Dröge (slomo) from comment #9) > Review of attachment 347444 [details] [review] [review]: > > ::: gst-libs/gst/player/gstplayer.c > @@ -586,3 @@ > - if (self->suburi) { > - g_free (self->suburi); > - self->suburi = NULL; > > You're now not freeing self->suburi and not setting it to NULL. Apart from > that this looks good now We cannot free and set self->suburi to NULL. Before start play, user may set uri and suburi. After user call set_uri and set_suburi, the two operations will not be conducted immediately. As a result, set_uri_internal will free suburi which is user set at beginning and set_suburi_internal will set NULL to suburi property.
Then set_uri() (not set_uri_internal() should probably get rid of the suburi). Or overall the way how uri/suburi setting is done should be refactored.
Created attachment 347523 [details] [review] gst-player: fix gst-player failed to load external subtitle uri
commit 79be2e8b7b153bb0389fb09951b197a92b493357 Author: Haihua Hu <jared.hu@nxp.com> Date: Thu Mar 2 14:36:56 2017 +0800 player: Fix setting of external subtitle URI gst_player_set_uri_internal shouldn't free suburi which maybe set by user to load external subtitle before start play. It just need reset playbin's subutri property to NULL no matter if there was a previous one or not. https://bugzilla.gnome.org/show_bug.cgi?id=779453