#3230: SmoothStreaming URLs that uses parenthesis '(...)' as part of … #3231
Conversation
…rt of the URL causes server side errors
| this.manifestUri = manifestUri == null ? null | ||
| : Util.toLowerInvariant(manifestUri.getLastPathSegment()).equals("manifest") ? manifestUri | ||
| : Uri.withAppendedPath(manifestUri, "Manifest"); | ||
| this.manifestUri = manifestUri == null ? null : lastPathSegment.contains("manifest") && lastPathSegment.contains("(") && lastPathSegment.contains(")")) ? manifestUri : lastPathSegment.equals("manifest") ? manifestUri : Uri.withAppendedPath(manifestUri, "Manifest"); |
There was a problem hiding this comment.
- Does this actually compile? What is
lastPathSegment? It doesn't seem to be defined anywhere. - I'm not convinced about the logic. It matches all kinds of random things, like "xy)(manifest". Why not use
Util.toLowerInvariant(manifestUri.getLastPathSegment()).matches("XXX")where "XXX" is a regular expression that more exactly describes what we consider a match?
There was a problem hiding this comment.
You are right - apologies for that, very sloppy of me.
And a reg-exp would be a much more elegant solution - will update
…rt of the URL causes server side errors: revised after comments
|
@ojw28 - updated the pull request - there is a duplication of the reg.exp used in Util.java and the SsMediaSource.java - not sure if you have a better, mode preferred place to put this or not. Hope that is OK now. |
| this.manifestUri = manifestUri == null ? null | ||
| : Util.toLowerInvariant(manifestUri.getLastPathSegment()).equals("manifest") ? manifestUri | ||
| : Uri.withAppendedPath(manifestUri, "Manifest"); | ||
| String lastPathSegment = Util.toLowerInvariant(manifestUri.getLastPathSegment()); |
There was a problem hiding this comment.
This will NullPointerException is manifestUri is null.
There was a problem hiding this comment.
You are right but by pulling together the reg-exp to check also for just 'manifest' we should be able to get rid of that local variable entirely.
| : Uri.withAppendedPath(manifestUri, "Manifest"); | ||
| String lastPathSegment = Util.toLowerInvariant(manifestUri.getLastPathSegment()); | ||
| // Using a regexp here to allow for URLs which contain (..) as their last part, e.g. http://host/content.ism/manifest(filter=0) - see #3230 | ||
| this.manifestUri = manifestUri == null ? null : lastPathSegment.equals("manifest") ? manifestUri : lastPathSegment.matches("manifest\\(.*\\)") ? manifestUri : Uri.withAppendedPath(manifestUri, "Manifest"); |
There was a problem hiding this comment.
It might be preferable to have a single regex that matches "manifest" and "manifest(...)". Something like "manifest(\\(.*\\))?" (I didn't check that's right!). Then you can get rid of the equals("manifest") case.
| @@ -901,7 +901,8 @@ public static int inferContentType(String fileName) { | |||
| } else if (fileName.endsWith(".m3u8")) { | |||
| return C.TYPE_HLS; | |||
| } else if (fileName.endsWith(".ism") || fileName.endsWith(".isml") | |||
| || fileName.endsWith(".ism/manifest") || fileName.endsWith(".isml/manifest")) { | |||
| || fileName.endsWith(".ism/manifest") || fileName.endsWith(".isml/manifest") | |||
| || Uri.parse(fileName).getLastPathSegment().matches("manifest\\(.*\\)")) { | |||
There was a problem hiding this comment.
I don't think we want to match if the .ism or .isml bit isn't there.
There was a problem hiding this comment.
Would you prefer if this is pushed into a reg-exp as well?
There was a problem hiding this comment.
I think ideally it would be a regex check directly on fileName (i.e. avoiding parsing as a uri, particularly given the method above converts from a uri into a string to call this method :)).
|
I pushed ce7aaab to fix this, since this hadn't been updated in a while. Let us know on the corresponding issue if there are any problem. Thanks! |
…the URL causes server side errors
Here is the suggested fix for #3230 in a pull request.