Fix HLS download failure when EXT-X-DEFINE variables are referenced in media playlist segment URLs

[prashant9934]

PR Description:

Summary

Fixes HLS download failures for streams where the multivariant playlist defines
variables via #EXT-X-DEFINE:NAME=...,VALUE=... and media playlists reference
them in segment URLs without an explicit #EXT-X-DEFINE:IMPORT=... tag.

Fixes #3258

Problem

HlsDownloader (via SegmentDownloader) uses a single HlsPlaylistParser()
instance constructed with HlsMultivariantPlaylist.EMPTY. This instance first
parses the multivariant playlist, then parses each media playlist. However, the
parsed multivariant playlist was never stored back into the parser instance.

When media playlists are subsequently parsed, multivariantPlaylist is still
EMPTY, so:

• variableDefinitions is unavailable during replaceVariableReferences()
• {$variable} references in segment URLs pass through unsubstituted
• Downloads fail with HTTP errors on malformed URLs
• HlsMediaPeriod.getStreamKeys() crashes with IndexOutOfBoundsException

During playback this works correctly because HlsPlaylistTracker constructs
the parser with the real multivariant playlist.

Fix

HlsPlaylistParser.java:

• Make multivariantPlaylist field non-final
• After parsing a multivariant playlist in parse(), store it in the instance
• Subsequent parseMediaPlaylist() calls on the same instance now have access
  to the correct variableDefinitions for variable substitution

HlsMediaPeriod.java:

• Add bounds checks in getStreamKeys() to prevent IndexOutOfBoundsException
  when redundantGroupIndicesPerWrapper has an empty array or when
  renditionRedundantGroupIndex exceeds the redundant groups list size

Testing

Tested with an HLS stream using:

#EXT-X-DEFINE:NAME="awsContentSegmentPrefix",VALUE="https://cdn.example.com/"

in the multivariant playlist, with media playlist segment URLs referencing
{$awsContentSegmentPrefix} directly. Download completes successfully with
variables resolved. Existing playback behavior is unchanged

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[marcbaechinger]

Thanks. Please can you file the CLA? We can't process this before the CLA requirements.

As you are saying that the problem manifests in download use cases only. Would you be able to craft a unit test that demonstrates the failure and verifies the fix? That's what we have to do anyway internally when processing your fix, and if you are able to provide this, this may make taking the pull a bit quicker. Thanks for considering adding such a unit test. We'd need the CLA in any case.

[marcbaechinger]

Many thanks for the fix!

I've added some comment.

First though, the change is based on the 'release' branch. We would need that from the 'main' branch.

I suggested some changes in the comments. If you want to apply these and rebase the change onto main, please let me know. This way you get the credits in the commit logs attributed to your user.

Otherwise, I have this change ready in a local change where I veryfied my suggestions for change. So quickest would be I just apply this change and send it for internal review.

Please let me know what you prefer and give me a go to continue on my change or make a new PR and let me know when you are ready for review with the changes from above applied.

Thanks again for the change and the unit tests!

[marcbaechinger]

I think we should not load here. When looking at SegmentDownloader.getManifest (from which HlsDownaloder) extends, we see that internally this is processed through execute. This method handles quite some cases that need to be handled properly and also integrates with the PriorityTaskManager to do some priority management.

I think instead we should overload SegmentDownloader.getManifest and add an additional parameter that takes a Parser<M>. We can then in HlsSegmentParser pass down either the childManfestParser or the original parser that we pass down to super in the constructor. After calling super we cab assign it to a new field HlsPlaylistParser hlsPlaylistParser of HlsDownloader and we can then use it like:

Parser<HlsPlaylist> childManifestParser =
        hasVariables
            ? new HlsPlaylistParser(multivariantPlaylist, /* previousMediaPlaylist= */ null)
            : hlsPlaylistParser;

Instead of the if (childManfestParser != null) { we can then instead just do:

// calling the new overload method in the super class `SegementDownloader`
mediaPlaylist =
            (HlsMediaPlaylist)
                getManifest(dataSource, childManifestParser, mediaPlaylistDataSpec, removing);

This way we take advantage of the harness in execute in the same way as before.

[marcbaechinger]

rename from childManifestParser to mediaPlaylistParser ?

[prashant9934]

Many thanks for the fix!

I've added some comment.

First though, the change is based on the 'release' branch. We would need that from the 'main' branch.

I suggested some changes in the comments. If you want to apply these and rebase the change onto main, please let me know. This way you get the credits in the commit logs attributed to your user.

Otherwise, I have this change ready in a local change where I veryfied my suggestions for change. So quickest would be I just apply this change and send it for internal review.

Please let me know what you prefer and give me a go to continue on my change or make a new PR and let me know when you are ready for review with the changes from above applied.

Thanks again for the change and the unit tests!

Many thanks for the fix!

I've added some comment.

First though, the change is based on the 'release' branch. We would need that from the 'main' branch.

I suggested some changes in the comments. If you want to apply these and rebase the change onto main, please let me know. This way you get the credits in the commit logs attributed to your user.

Otherwise, I have this change ready in a local change where I veryfied my suggestions for change. So quickest would be I just apply this change and send it for internal review.

Please let me know what you prefer and give me a go to continue on my change or make a new PR and let me know when you are ready for review with the changes from above applied.

Thanks again for the change and the unit tests!

Thanks for the review and the suggestions! Please go ahead and apply your change with the suggested modifications and send it for internal review. Happy to have it land quickly — attribution isn't a concern for me.

Let me know if there's anything else I can help with!