GNOME Bugzilla – Bug 765528
gstmpdparser: GstUri changed unexpectedly within combine_urls()
Last modified: 2018-11-03 13:49:53 UTC
Created attachment 326680 [details] issued MPD file Hi all: I met a problem when combine_urls() is executed the result URL is broken. Such as: expected result = "https://r8---sn-ipoxu-u2xl.googlevideo.com/videoplayback?id=cf12500690581ecb&itag=133&source=youtube&requiressl=yes&ms=au&pl=16&mm=31&mv=m&mn=sn-ipoxu-u2xl&initcwndbps=1245000&ratebypass=yes&mime=video/mp4&gir=yes&clen=3617089&lmt=1439380361025617&dur=121.746&signature=8C0600A08A8FA47C3734F42B4721E2143EACDF94.705D52170E63A2E70291789D67FAC9C139B12965&mt=1461557621&upn=hP1NJs1_Lng&sver=3&fexp=3300118,3300134,3300161,3312381,9410705,9416126,9416891,9422596,9426927,9427482,9428398,9431012,9431364,9431865,9432033,9432132,9432684,9432839,9433096,9433193,9433425,9433947,9433997&key=dg_yt0&ip=42.73.160.27&ipbits=0&expire=1461579370&sparams=ip,ipbits,expire,id,itag,source,requiressl,ms,pl,mm,mv,mn,initcwndbps,ratebypass,mime,gir,clen,lmt,dur <https://r8---sn-ipoxu-u2xl.googlevideo.com/videoplayback?id=cf12500690581ecb&itag=133&source=youtube&requiressl=yes&ms=au&pl=16&mm=31&mv=m&mn=sn-ipoxu-u2xl&initcwndbps=1245000&ratebypass=yes&mime=video/mp4&gir=yes&clen=3617089&lmt=1439380361025617&dur=121.746&signature=8C0600A08A8FA47C3734F42B4721E2143EACDF94.705D52170E63A2E70291789D67FAC9C139B12965&mt=1461557621&upn=hP1NJs1_Lng&sver=3&fexp=3300118%2c3300134%2c3300161%2c3312381%2c9410705%2c9416126%2c9416891%2c9422596%2c9426927%2c9427482%2c9428398%2c9431012%2c9431364%2c9431865%2c9432033%2c9432132%2c9432684%2c9432839%2c9433096%2c9433193%2c9433425%2c9433947%2c9433997&key=dg_yt0&ip=42.73.160.27&ipbits=0&expire=1461579370&sparams=ip%2cipbits%2cexpire%2cid%2citag%2csource%2crequiressl%2cms%2cpl%2cmm%2cmv%2cmn%2cinitcwndbps%2cratebypass%2cmime%2cgir%2cclen%2clmt%2cdur> " broken result = "https://r8---sn-ipoxu-u2xl.googlevideo.com/videoplayback " static GstUri * combine_urls (GstUri * base, GList * list, gchar ** query, GstActiveStream * stream) { GstUri *ret = base; ... /*here the ret is still sound*/ gst_uri_set_query_table (ret, NULL); /*here the ret is broken...*/ } The attached is the DASH MPD where this issue happened. Could someone help on checking it? Thanks.
Can you provide a simple testcase for this, by adding something to gst-plugins-bad/tests/check/elements/dash_mpd.c? combine_urls() is setting the queries to NULL, but that is expected. The query part is returned and supposed to be used by the caller. Not happening everywhere and the attached patch probably fixes it. Can you confirm?
Created attachment 327135 [details] [review] mpdparser: Copy over already existing query strings when combining URLs
Dear slomo: I have tried your patch but failed.. After doing some checks, I found the issue happened when downloading index or header segment. It does NOT assemble queryURL & baseURL in this case so the issue happened. I made a patch as attached as well as the MPD file which leads to the issue. Please kindly refer to it. Thank you~
Created attachment 327196 [details] [review] patch to resolve this issue patch to resolve this issue
Created attachment 327197 [details] MPD file where issue happened MPD file where issue happened
Is your patch needed on top of mine, or is yours alone enough?
Dear slomo: It is only with 2nd patch. I think the root cause is the same procedure should be applied to index & header segment also. Otherewise, once the query strings appear their URL will be wronglyt truncated & an invalid URL will lead to request fail. I am not so experienced in handling URL so please kindly give it a review. Thank you!
Review of attachment 327196 [details] [review]: Thanks for the patch, a few things to fix. Also please follow our commit message style, check git log to see other commits for reference. And add a valid author/email to the patch. ::: ext/dash/gstmpdparser.c @@ +3076,3 @@ g_return_val_if_fail (stream != NULL, NULL); + if (!InitializationURL->sourceURL) { InitializationURL might be NULL, causing a segfault here.
Created attachment 327359 [details] [review] tests: dash_mpd: test for urls with query parameters This test depicts the problem and also shows that the propsed patch segfaults
Created attachment 328685 [details] [review] Resolve the issue if queryURL appears when downloading index or initialization segment. Resolve the issue if queryURL appears when downloading index or initialization segment
Dear Thiago: As according to your suggestion, a new patch has been sent. Thank you.
Review of attachment 328685 [details] [review]: Code looks ok, but please provide a better commit message. Just check git log of any module and you will see how to properly format the message. Also, do you know which part of the spec mentions how these URL parameters should be copied around? Couldn't find anything specific about this.
John, can you update your patch?
-- 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/380.