After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 701681 - [regression] NASA HLS stream
[regression] NASA HLS stream
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal blocker
: 1.1.4
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-06-05 23:46 UTC by Olivier Crête
Modified: 2013-08-20 12:10 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
0001-query-add-new-redirection-uri-the-URI-query.patch (3.70 KB, patch)
2013-07-13 00:52 UTC, Andoni Morales
needs-work Details | Review
0001-souphttpsrc-add-redirection-to-the-URI-query.patch (2.72 KB, patch)
2013-07-13 00:52 UTC, Andoni Morales
committed Details | Review
0001-hlsdemux-add-support-for-redirections.patch (924 bytes, patch)
2013-07-13 00:54 UTC, Andoni Morales
committed Details | Review
0001-query-add-new-redirection-uri-the-URI-query.patch (3.71 KB, patch)
2013-07-23 14:27 UTC, Andoni Morales
reviewed Details | Review
0002-query-fix-annotation-for-gst_query_parse_uri.patch (809 bytes, patch)
2013-07-23 14:28 UTC, Andoni Morales
committed Details | Review
0001-query-add-new-redirection-uri-the-URI-query.patch (3.59 KB, patch)
2013-08-08 16:42 UTC, Andoni Morales
committed Details | Review
fix for segfault from uninitialized uri (1.05 KB, patch)
2013-08-19 11:05 UTC, A Ashley
needs-work Details | Review

Description Olivier Crête 2013-06-05 23:46:25 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
  • #0 ff_pred8x8_top_dc_8_mmxext
    at libavcodec/x86/h264_intrapred.asm line 797
  • #1 hl_decode_mb_simple_8
    at libavcodec/h264_mb_template.c line 166
  • #2 decode_slice
    at libavcodec/h264.c line 3737
  • #3 execute_decode_slices
    at libavcodec/h264.c line 3826
  • #4 decode_nal_units
    at libavcodec/h264.c line 4115
  • #5 decode_frame
    at libavcodec/h264.c line 4205
  • #6 frame_worker_thread
    at libavcodec/pthread.c line 378
  • #7 start_thread
    at pthread_create.c line 308
  • #8 clone
    at ../sysdeps/unix/sysv/linux/x86_64/clone.S line 114

Comment 1 Edward Hervey 2013-07-07 09:28:12 UTC
That service is not available anymore :( Got another link ?
Comment 2 Olivier Crête 2013-07-12 17:00:04 UTC
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.
Comment 3 Andoni Morales 2013-07-13 00:52:23 UTC
Created attachment 249053 [details] [review]
0001-query-add-new-redirection-uri-the-URI-query.patch
Comment 4 Andoni Morales 2013-07-13 00:52:58 UTC
Created attachment 249054 [details] [review]
0001-souphttpsrc-add-redirection-to-the-URI-query.patch
Comment 5 Andoni Morales 2013-07-13 00:54:44 UTC
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
Comment 6 Sebastian Dröge (slomo) 2013-07-13 08:22:16 UTC
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)
Comment 7 Andoni Morales 2013-07-13 10:45:51 UTC
(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) ?
Comment 8 Sebastian Dröge (slomo) 2013-07-13 11:46:51 UTC
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
Comment 9 Tim-Philipp Müller 2013-07-13 21:48:29 UTC
Why should the URI query not just return the redirected URI?

And do we differentiate between temporary + permanent redirect here?
Comment 10 Andoni Morales 2013-07-13 22:08:16 UTC
(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
Comment 11 Sebastian Dröge (slomo) 2013-07-15 06:01:33 UTC
(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
Comment 12 Sebastian Dröge (slomo) 2013-07-15 06:31:18 UTC
Does it still segfault after Andoni's changes btw?
Comment 13 Andoni Morales 2013-07-15 10:58:44 UTC
It's probably still segfaulting because this playlist doesn't contain any #EXT-X-STREAM-INF entry.
Comment 14 Andoni Morales 2013-07-15 11:02:47 UTC
In fact this example is not even a variant playlist but a regular playlist and shouldn't be detected as an hls by typefind.
Comment 15 Olivier Crête 2013-07-15 11:06:56 UTC
But it has #EXTM3U, so that kind of means HLS... and I assume this "just works" with apple stuff
Comment 16 Andoni Morales 2013-07-15 11:18:25 UTC
(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.
Comment 17 Sebastian Dröge (slomo) 2013-07-23 13:00:04 UTC
So what should happen here? One of the patches still needs some work
Comment 18 Andoni Morales 2013-07-23 14:27:32 UTC
Created attachment 249896 [details] [review]
0001-query-add-new-redirection-uri-the-URI-query.patch
Comment 19 Andoni Morales 2013-07-23 14:28:12 UTC
Created attachment 249897 [details] [review]
0002-query-fix-annotation-for-gst_query_parse_uri.patch
Comment 20 Sebastian Dröge (slomo) 2013-07-23 16:23:09 UTC
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
Comment 21 Tim-Philipp Müller 2013-07-23 16:36:24 UTC
> ::: 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);
Comment 22 Andoni Morales 2013-08-08 16:42:57 UTC
Created attachment 251195 [details] [review]
0001-query-add-new-redirection-uri-the-URI-query.patch

Rebase with current master and apply Tim's recommendation.
Comment 23 Sebastian Dröge (slomo) 2013-08-12 13:36:08 UTC
Review of attachment 251195 [details] [review]:

Looks good
Comment 24 Andoni Morales 2013-08-12 14:42:27 UTC
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
Comment 25 Sebastian Dröge (slomo) 2013-08-13 09:02:55 UTC
What else is missing for this bug?
Comment 26 Andoni Morales 2013-08-13 11:10:52 UTC
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
Comment 27 Sebastian Dröge (slomo) 2013-08-13 13:49:32 UTC
So it can be closed now?
Comment 28 A Ashley 2013-08-19 11:05:48 UTC
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
Comment 29 Andoni Morales 2013-08-19 12:03:24 UTC
(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?
Comment 30 Tim-Philipp Müller 2013-08-19 12:04:58 UTC
(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..
Comment 31 Andoni Morales 2013-08-19 12:09:30 UTC
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
Comment 32 Sebastian Dröge (slomo) 2013-08-19 12:32:54 UTC
And also related probably bug #706160
Comment 33 Sebastian Dröge (slomo) 2013-08-20 07:45:44 UTC
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?
Comment 34 Tim-Philipp Müller 2013-08-20 12:10:48 UTC
Works fine for me too, let's close this.