Skip to content

Conversation

@gvb26
Copy link
Collaborator

@gvb26 gvb26 commented Aug 12, 2025

Description

Addresses an issue where for some partners' (Cox) content is causing a crash when DAI is enabled. Was traced back to how we are handling tags.

Change Notes

  • Refactors Key index tag logic within HLSPlaylistStructureConstructor

Pre-submission Checklist

  • I ran the unit tests locally before checking in.
  • I made sure there were no compiler warnings before checking in.
  • I have written useful documentation for all public code.
  • I have written unit tests for this new feature.

@CLAassistant
Copy link

CLAassistant commented Aug 12, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@theRealRobG theRealRobG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a test case that reproduces what you are seeing?

Looking through this method, the only way I can see this happening, is if the playlist delivered has no media segments, which is quite strange... Is that the case here? I wrote a test (in HLSMediaSpanTests) that reproduces the crash:

func testWeirdNoSegmentsScenario() {
    let hlsArray = [
        "#EXTM3U\n",
        "#EXT-X-TARGETDURATION:6\n",
        "#EXT-X-VERSION:3\n",
        "#EXT-X-MEDIA-SEQUENCE:0\n",
        "#EXT-X-PLAYLIST-TYPE:VOD\n",
        "#EXT-X-KEY:METHOD=NONE\n",
        "#EXT-X-MAP:URI=\"test.mp4\",BYTERANGE=\"610@0\"\n"
    ]
    let hlsString = hlsArray.joined()
    runTest(hlsString: hlsString, expectedSpans: [])
}

If that is the case, then maybe an approach that fits the method better, is to add something like

// If the playlist contains no segments then there are no spans
if mediaSegmentGroups.isEmpty {
    return []
}

to the top of the method. I think that empty spans in the case of no segments fits the intention of the property best.

But to properly understand the best approach, I'd like to know a bit more about your case that is causing the crash.

As an aside, to be honest, I haven't ever used mediaSpans, so had to read through the repository on what the intended usage is. It seems to provide information about "spans of segments", supposed to be defined by the "spanner tags" (the tags that apply to the next segment and all subsequent segments until the next instance of the tag), though application has only been done for EXT-X-KEY... Which seems strange, as there are several other tags that span (even when this was introduced in the first commit back in 2017 there were other spanner tags like EXT-X-MAP). Personally, I think it would be best to deprecate this, and either don't replace it or replace it with a method that can check for spans defined by a particular tag (I don't think that the spec guarantees that spans defined by different types of tags will be equal, for example, the EXT-X-MAP may change without changing the EXT-X-KEY as is done during ad transitions where the key method is continuously NONE).

@gvb26
Copy link
Collaborator Author

gvb26 commented Aug 14, 2025

Could you add a test case that reproduces what you are seeing?

Looking through this method, the only way I can see this happening, is if the playlist delivered has no media segments, which is quite strange... Is that the case here? I wrote a test (in HLSMediaSpanTests) that reproduces the crash:

Hi @theRealRobG so this is a bit of a strange case... We are hitting a nil value when DAI was enabled after about 45min to an hour of watching TVE Linear content (TNT GO etc) in our Cox client only. The assumption being for some reason unknown to me, Cox client mixed with DAI reaches a point in the playlist where its not delivering any media segments...

Its a very strange and niche defect that luckily this block fixes. I will test your suggestion and add that test you asked for

@gvb26 gvb26 requested a review from rmigneco August 29, 2025 20:31
@rmigneco
Copy link
Collaborator

rmigneco commented Sep 2, 2025

@gvb26 Can you please add a test case as Rob suggested?

@gvb26
Copy link
Collaborator Author

gvb26 commented Sep 2, 2025

So I've added the tests and made a few more adjustments but it appears like there is no CI/CD and xcode config issue that is causing builds to fail up here. I've been debugging but appears like Github actions is getting hung up on xcodebuild configs as well as trying to build mamba for mac. Pasted a couple errors I've been getting:

2025-09-02T19:16:19.8648260Z The following build commands failed:
2025-09-02T19:16:19.8649160Z     cd /Users/runner/work/mamba/mamba
2025-09-02T19:16:19.8706360Z 	SwiftCompile normal arm64 /Users/runner/work/mamba/mamba/mambaTests/HLSPlaylistStructureTests.swift (in target 'mambaTVOSTests' from project 'mamba')
xcodebuild[946:5894] Writing error result bundle to /var/folders/x7/ch5v91h56_zbvbd1y2f600dm0000gn/T/ResultBundle_2025-02-09_19-12-0015.xcresult
xcodebuild: error: missing value for key 'name' of option 'Destination'
Writing error result bundle to /var/folders/x7/ch5v91h56_zbvbd1y2f600dm0000gn/T/ResultBundle_2025-02-09_19-02-0045.xcresult
xcodebuild: error: Unable to find a device matching the provided destination specifier:
		{ platform:iOS Simulator, OS:latest, name:iPhone 16 }

	The requested device could not be found because no available devices matched the request.

	Available destinations for the "mamba" scheme:
		{ platform:macOS, arch:arm64, variant:Mac Catalyst, id:0000FE00-C648B17BC1592925, name:My Mac }
		{ platform:macOS, arch:x86_64, variant:Mac Catalyst, id:0000FE00-C648B17BC1592925, name:My Mac }
		{ platform:macOS, arch:arm64, variant:Designed for [iPad,iPhone], id:0000FE00-C648B17BC1592925, name:My Mac }
		{ platform:iOS, id:dvtdevice-DVTiPhonePlaceholder-iphoneos:placeholder, name:Any iOS Device }
		{ platform:iOS Simulator, id:dvtdevice-DVTiOSDeviceSimulatorPlaceholder-iphonesimulator:placeholder, name:Any iOS Simulator Device }
		{ platform:visionOS Simulator, arch:arm64, variant:Designed for [iPad,iPhone], id:45D3DA2C-3E1F-43B8-BC11-C3BB180B5CD2, OS:2.3, name:Apple Vision Pro }
		{ platform:visionOS Simulator, arch:arm64, variant:Designed for [iPad,iPhone], id:87256B89-5AF0-4A41-8C67-ABCC8EC18FBE, OS:2.4, name:Apple Vision Pro }
		{ platform:visionOS Simulator, arch:arm64, variant:Designed for [iPad,iPhone], id:27777E2E-1663-4AEE-AB8B-3DE71AA9C7DB, OS:2.5, name:Apple Vision Pro }

	Ineligible destinations for the "mamba" scheme:
		{ platform:macOS, variant:Mac Catalyst, name:Any Mac, error:Any Mac’s macOS platform doesn’t match mamba.framework’s supported platforms. You can change mamba.framework’s Base SDK or Supported Platforms to support Any Mac. }

Just having a hard time seeing where these changes could trigger this

Copy link
Collaborator

@theRealRobG theRealRobG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you added the tests in, but it looks like your tests are calling runTest that exists within HLSMediaSpanTests, and your tests are added to HLSPlaylistStructureTests (which does not have a runTest method). Running these locally I can see them failing:
Screenshot 2025-09-03 at 18 27 25

If I move these tests to HLSMediaSpanTests then I can see that everything passes:
Screenshot 2025-09-03 at 18 30 08

Copy link
Collaborator

@theRealRobG theRealRobG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some extra comments after taking a look at the rest of the changes.

@gvb26 gvb26 force-pushed the bugfix/cox-hls-error-dai branch from 5326f7b to 8614292 Compare September 4, 2025 21:50
@gvb26 gvb26 force-pushed the bugfix/cox-hls-error-dai branch from 8614292 to 8397813 Compare September 4, 2025 21:59
Copy link
Collaborator

@rmigneco rmigneco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments are mostly style related but I am reticent to remove the assert(keyCount == keyTags.count, "we missed a key tag") and replace with a print because its really easy to ignore console prints. If it's a weird enough scenario that we need to be notified by it, assert may be the best course.
In another world/version of mamba, we might have the ability to log a warning but that's not the current state of things.

Copy link
Collaborator

@theRealRobG theRealRobG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for the changes!

Now, I knew this would hit us eventually, but the branch protection is expecting a pass on a test run with iPhone 15, but the latest simulator is using iPhone 16 (I couldn't figure out how to give custom names for jobs when matrix is used in the Action) - so let me update the expectations in the repo settings and hopefully this should then be mergeable 🤞

@theRealRobG
Copy link
Collaborator

theRealRobG commented Sep 6, 2025

Yep, looks good now, all required status checks have passed 🥳

Feel free to merge... We can make a release afterwards.

@rmigneco
Copy link
Collaborator

rmigneco commented Sep 8, 2025

@theRealRobG not sure we want/need this in mamba 2.x.x so we'll keep it for 1.x unless you feel otherwise

@rmigneco rmigneco merged commit 56242bc into develop_1.x Sep 8, 2025
4 checks passed
@theRealRobG
Copy link
Collaborator

No worries @rmigneco, that makes sense... I might take some time to cherry-pick them to the 2.x branch when I get a moment later on. Though each time I do one of those, I more and more get closer to the idea that we should just abandon v2 completely 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants