GNOME Bugzilla – Bug 701681
[regression] NASA HLS stream
Last modified: 2013-08-20 12:10:48 UTC
This HLS stream works fine in 1.0.7, but segfaults with git master from today. URL: http://www.nasa.gov/multimedia/nasatv/NTV-Public-IPS.m3u8 This segfaults: gst-launch-1.0 playbin uri=http://www.nasa.gov/multimedia/nasatv/NTV-Public-IPS.m3u8 (gdb) bt
+ Trace 232017
That service is not available anymore :( Got another link ?
It's actually now a redirect... another case that's broken It redirects to http://public.infozen.cshls.lldns.net/infozen/public/public.m3u8 This one doesn't crash, but it still has problems.
Created attachment 249053 [details] [review] 0001-query-add-new-redirection-uri-the-URI-query.patch
Created attachment 249054 [details] [review] 0001-souphttpsrc-add-redirection-to-the-URI-query.patch
Created attachment 249055 [details] [review] 0001-hlsdemux-add-support-for-redirections.patch Add support for redirections. This was discussed with slomo and __tim a couple of months ago, I hope this is still the correct way to do it
Review of attachment 249053 [details] [review]: ::: gst/gstquery.c @@ +1442,3 @@ + * gst_query_parse_uri_redirection: + * @query: a #GstQuery + * @uri: (out callee-allocates) (allow-none): the storage for the current URI This is not callee-allocates, just a normal out parameter with (transfer full)
(In reply to comment #6) > Review of attachment 249053 [details] [review]: > > ::: gst/gstquery.c > @@ +1442,3 @@ > + * gst_query_parse_uri_redirection: > + * @query: a #GstQuery > + * @uri: (out callee-allocates) (allow-none): the storage for the current URI > > This is not callee-allocates, just a normal out parameter with (transfer full) It's actually copy pasted from gst_query_parse_uri, so I believe this one needs to be fixed too :) Also, is there anything I should add in the description for the new API (eg: @since 1.2) ?
Yes, please fix the other one too then :) "Since: 1.2" should be added, look in other files for the place where to add it in the docs comment
Why should the URI query not just return the redirected URI? And do we differentiate between temporary + permanent redirect here?
(In reply to comment #9) > Why should the URI query not just return the redirected URI? In a regular scenario you don't really care about the redirection, but in this particular case it's used to resolve the relative uri in the variant playlist. Having both allows the element decide which one it wants to use. > > And do we differentiate between temporary + permanent redirect here? I don't know whether we should take this in consideration or not... Reading a bit more about this particula the case, it's not the type of redirection I am fixing here. In this case we have: curl -i -L http://www.nasa.gov/multimedia/nasatv/NTV-Public-IPS.m3u8 HTTP/1.0 200 OK [...] #EXTM3U http://public.infozen.cshls.lldns.net/infozen/public/public.m3u8 Which is different from: $ curl -i -L http://localhost/testRedirection.m3u8 HTTP/1.1 302 Found Location: http://devimages.apple.com/iphone/samples/bipbop/bipbopall.m3u8 Which is the case being fixed with my patches
(In reply to comment #9) > Why should the URI query not just return the redirected URI? Because that could break backwards compatibility and also is a bit unexpected IMHO. > And do we differentiate between temporary + permanent redirect here? Maybe don't call it redirect but "actual" URI? This should probably be just the URI that is actually used now, no matter if it's a permanent or temporary redirect. (In reply to comment #10) > Reading a bit more about this particula the case, it's not the type of > redirection I am fixing here. > > In this case we have: > curl -i -L http://www.nasa.gov/multimedia/nasatv/NTV-Public-IPS.m3u8 > HTTP/1.0 200 OK > [...] > #EXTM3U > http://public.infozen.cshls.lldns.net/infozen/public/public.m3u8 That would be interesting to support too but is a completely different problem :) Tim probably meant HTTP status codecs 301/308 vs 302/307
Does it still segfault after Andoni's changes btw?
It's probably still segfaulting because this playlist doesn't contain any #EXT-X-STREAM-INF entry.
In fact this example is not even a variant playlist but a regular playlist and shouldn't be detected as an hls by typefind.
But it has #EXTM3U, so that kind of means HLS... and I assume this "just works" with apple stuff
(In reply to comment #15) > But it has #EXTM3U, so that kind of means HLS... and I assume this "just works" > with apple stuff What I mean is that it should probably be handled as a regular playlist is handled. It just parses the first uri and redirects to it which is then parsed as a real HLS playlist and handled by the element itself.
So what should happen here? One of the patches still needs some work
Created attachment 249896 [details] [review] 0001-query-add-new-redirection-uri-the-URI-query.patch
Created attachment 249897 [details] [review] 0002-query-fix-annotation-for-gst_query_parse_uri.patch
Review of attachment 249896 [details] [review]: ::: gst/gstquark.c @@ +69,3 @@ "GstEventStreamStart", "stream-id", "GstEventContext", "GstQueryContext", + "GstMessageNeedContext", "GstMessageHaveContext", "context", "context-types", + "uri-redirection" You'll need to rebase that against git master, but just do that before you push ;) ::: gst/gstquery.c @@ +1459,3 @@ + if (uri) + *uri = g_value_dup_string (gst_structure_id_get_value (structure, + GST_QUARK (URI_REDIRECTION))); I think this might give assertions if there is no uri-redirection field. You might need to check if gst_structure_id_get_value() returns NULL
> ::: gst/gstquery.c > @@ +1459,3 @@ > + if (uri) > + *uri = g_value_dup_string (gst_structure_id_get_value (structure, > + GST_QUARK (URI_REDIRECTION))); > > I think this might give assertions if there is no uri-redirection field. You > might need to check if gst_structure_id_get_value() returns NULL You should be able to replace this entire if (uri) block with just: gst_structure_id_get (structure, GST_QUARK (URI_REDIRECTION), uri, NULL);
Created attachment 251195 [details] [review] 0001-query-add-new-redirection-uri-the-URI-query.patch Rebase with current master and apply Tim's recommendation.
Review of attachment 251195 [details] [review]: Looks good
commit 0d824938948776a1e2a36de83b9bff00bbd07f50 Author: Andoni Morales Alastruey <ylatuya@gmail.com> Date: Tue Jul 23 16:25:27 2013 +0200 query: fix annotation for gst_query_parse_uri commit a70b5fc16a0473a58e6a20cc498d8116e3e6eec9 Author: Andoni Morales Alastruey <ylatuya@gmail.com> Date: Fri Apr 19 12:14:54 2013 +0200 query: add new redirection uri the URI query commit b9faeab236f36d69a264c42632577e59564c0be1 Author: Andoni Morales Alastruey <ylatuya@gmail.com> Date: Sat Jul 13 01:50:56 2013 +0200 souphttpsrc: add redirection to the URI query commit 28609ca93c1fc0e536071f2ebd46b1cae41a7d9b Author: Andoni Morales Alastruey <ylatuya@gmail.com> Date: Sat Jul 13 02:50:52 2013 +0200 hlsdemux: add support for redirections
What else is missing for this bug?
For the real redirection scenario it's fixed. The indirection one is not really a valid case for HLS and they seem to have fixed it in http://www.nasa.gov/multimedia/nasatv/NTV-Public-IPS.m3u8 which now has: #EXTM3U #EXT-X-STREAM-INF:PROGRAM-ID=1127167744,BANDWIDTH=1000000 http://public.infozen.cshls.lldns.net/infozen/public/public/public_200.m3u8 #EXT-X-STREAM-INF:PROGRAM-ID=1127167744,BANDWIDTH=400000 http://public.infozen.cshls.lldns.net/infozen/public/public/public_400.m3u8 #EXT-X-STREAM-INF:PROGRAM-ID=1127167744,BANDWIDTH=200000 http://public.infozen.cshls.lldns.net/infozen/public/public/public_200.m3u8
So it can be closed now?
Created attachment 252199 [details] [review] fix for segfault from uninitialized uri Adding uri=NULL fixes a segfault when playing this stream: http://184.72.239.149/vod/smil:bigbuckbunnyiphone.smil/playlist.m3u8
(In reply to comment #28) > Created an attachment (id=252199) [details] [review] > fix for segfault from uninitialized uri > > Adding uri=NULL fixes a segfault when playing this stream: > http://184.72.239.149/vod/smil:bigbuckbunnyiphone.smil/playlist.m3u8 I am not able to reproduce it here. Are you using latest master branch from -base -good and -bad?
(In reply to comment #28) > Created an attachment (id=252199) [details] [review] > fix for segfault from uninitialized uri > > Adding uri=NULL fixes a segfault when playing this stream: > http://184.72.239.149/vod/smil:bigbuckbunnyiphone.smil/playlist.m3u8 > > > The call to gst_query_parse_uri_redirection assumes that the URI > parameter is always set, but if there is a case when this > assumption does not hold true. See gst/gstquery.c: > > g_return_if_fail (GST_QUERY_TYPE (query) == GST_QUERY_URI); If this g_return_if_fail() is triggered, all bets are off really. It is a programming mistake when that happens. Having said that, I don't mind initialising the uri to NULL, I think it's a good precaution, but I disagree that it's worth fixing because the g_return_if_fail() could happen..
I believe this was fixed by: commit 24b4ea3418e9cbf1ccf0d34e9b66b00e7b48d6db Author: Andoni Morales Alastruey <ylatuya@gmail.com> Date: Tue Aug 13 12:02:29 2013 +0200 souphttpsrc: fix critical setting a NULL uri redirection
And also related probably bug #706160
http://184.72.239.149/vod/smil:bigbuckbunnyiphone.smil/playlist.m3u8 works fine here for me with latest git, without any assertions or warnings or anything. Is there still a problem for anybody or can this bug be closed?
Works fine for me too, let's close this.