Incorrect appending of stringUri when baseUri contains a query string

[phpeng-ms]

When baseUri contains a query string, the stringUri component is incorrectly appended. This is because Util.getMergedUri(baseUri, stringUri) calls Uri.withAppendedPath(baseUri, stringUri), which adds stringUri as an additional path, then appends the original query from baseUri to the end. The expected behaviour should be a straight concatenation of stringUri to the end of baseUri.

For example, given:
baseUri = https://foobar.com/video/ABCD1234?videoformat=dash&
stringUri = quality=video360p&time=0

The expected URI would be:
https://foobar.com/video/ABCD1234?videoformat=dash&quality=video360p&time=0

The current (incorrect) URI is:
https://foobar.com/video/ABCD1234/quality=video360p&time=0?videoformat=dash&

The following is a temporary special-case patch to handle this case. A proper fix would be better.

--- a/library/src/main/java/com/google/android/exoplayer/util/Util.java
+++ b/library/src/main/java/com/google/android/exoplayer/util/Util.java
@@ -179,6 +179,13 @@ public final class Util {
     if (uri.isAbsolute()) {
       return uri;
     }
+    if (!TextUtils.isEmpty(baseUri.getQuery())) {
+      if (baseUri.toString().endsWith("&")) {
+        return Uri.parse(baseUri + stringUri);
+      } else {
+        return Uri.parse(baseUri + "&" + stringUri);
+      }
+    }
     return Uri.withAppendedPath(baseUri, stringUri);
   }

[ojw28]

You're right in saying that the result isn't correct, but I don't think your result is correct either. According to RFC3986, I think the following are true:

• Your stringUri should be interpreted as a path rather than as query parameters, because it doesn't have a preceding ?. The current logic is getting this right.
• The ABCD1234 part of baseUri should be discarded when the paths are merged. The current logic is getting this wrong.
• videoformat=dash& should be discarded. It's never the case that query parameters from baseUri and stringUri should both be retained after the merge. In all cases the ones from baseUri should be discarded, except the case where stringUri defines neither a path nor query parameters. The current logic is getting this wrong.

So I think the correct result for your example is:

https://foobar.com/video/quality=video360p&time=0

Which is in line with the 2nd example in section 5.4.1 of the spec.

We should fix the implementation, but doing so wont solve your case. If you're actually trying to merge URLs in the form you gave in your example, then I think you'll need to change the way you're specifying them in your manifests so that they merge properly according to the spec. I think you'd probably need to do something more like the 6th example in 5.4.1. For example:

baseUri = https://foobar.com/video/ABCD1234
stringUri = ?videoFormat=dash&quality=video360p&time=0

Which would merge in the way you expect.

[jeoliva]

Agree with ojw28. In fact, adding query string parameters of the base url will cause issues when working with some CDN's, that offer some security mechanisms that are based on a query string parameter that must be included just in the master request.

[ojw28]

@jeoliva Do you think it's necessary for us to implement remove_dot_segments, as per 5.2.4? It seems pretty obscure that (a) they'd ever appear, and (b) the server also couldn't handle them. Strictly speaking I guess we should. But in practice...

[phpeng-ms]

@ojw28 Thanks for the explanation. For your second bullet point, if we were to reformat our manifest to follow the 5.4.1 example 6, (i.e. baseUri = https://foobar.com/video/ABCD1234), would the merge be correct with the current ExoPlayer master branch logic or would we still have to wait for a fix? Is there any combination that would give the expected merged URI with the current implementation?

[jeoliva]

@ojw28, in my humble opinion I don't think implementing remove_dot_segments is an urgent need. I don't know any CDN, media server or encoder that is producing HLS streams whose segments/media playlists/encryption urls are defined using "../" or "./" (much less using other character combinations like /../ or /./).

And, as you said, in 99.9% of cases this is going to be supported directly by the web server receiving the requests. However, strictly speaking we should implement that also in the client/player side. The good point is we don't need to reinvent the wheel, this is a very known piece of code (that is even part of Android source code). See canonicalizePath method here: https://android.googlesource.com/platform/libcore/+/9b510df35b57946d843ffc34cf23fdcfc84c5220/luni/src/main/java/libcore/net/url/UrlUtils.java

So, given how easy this would be, my two cents go to adding this. Not something urgent, but a task for the future.

[ojw28]

@phpeng-ms I think that would work correctly with the current version of ExoPlayer, yes.

[ojw28]

@jeoliva Thanks for the link! I have a change I'm working on that correctly resolves for all normal and abnormal cases in the RFC, which is a good sign. I hope to push tomorrow, although it might take another day.