Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add missing MSS UUID #269

Merged
merged 1 commit into from
Oct 8, 2023
Merged

Conversation

julijane
Copy link
Contributor

In uuid.go there are already the UUID for Smooth Streaming Tfxd and Tfrf boxes defined. It makes sense to also add the UUID for Smooth Streaming StreamManifest and LiveServerManifest. This PR adds that. Also added references to the MSS Standard sections for all four UUID.

I'm unsure on the names, wdyt?

This adds the UUID for Microsoft Smooth Streaming Protocol for
StreamManifest and LiveServerManifest
@tobbee
Copy link
Collaborator

tobbee commented Sep 12, 2023

HI @julijan, thanks for your contribution.

The UUIDs seem to be correct and pointers to the standard is nice, but I wonder to what extent the new UUIDs are useful without adding support for the corresponding boxes StreamManifestBox and LiveServerManifestBox?

@julijane
Copy link
Contributor Author

julijane commented Sep 12, 2023

I don't understand the question. What more support is needed? The Manifests are just text data, which can just be put in the "UnknownPayload" box which is []byte anyways. I saw no point in adding special fields for that as I could not see any benefit.

The support for the Tfxd and Tfrf boxes is quite lowlevel as well, one can't directly create such a box but has to create a TfxdData struct, then a UUIDBox struct and put the TfxdData struct in the Tfxd field of the UUIDBox struct and manually set the UUID. So they don't have really more support either, except for the special data structs which make sense as those well have structured data. But as said, the manifest boxes are just text.

My current code (without using this patch in mp4ff) does it like this in my project:

	liveManifestBox := &mp4.UUIDBox{
		UnknownPayload: liveManifestBoxData,
	}
	liveManifestBox.SetUUID("a5d40b30-e814-11dd-ba2f-0800200c9a66")

But this makes me wonder: Should there maybe be a proper NewUUIDBox function? And maybe NewUUIDTfxdBox, NewUUIDTfrfBox etc. functions? Or should the Encode function automatically set the UUID if it finds for example the Tfxd field populated? As it is right now it all seems a bit clunky. WDYT?

@tobbee
Copy link
Collaborator

tobbee commented Sep 14, 2023

I didn't recall that these boxes are all inside a "uuid" box, so the "LSBoxType" in the MSS spec just says "uuid" in hex.

The code has mainly been used to parse and change smooth streaming assets, so there are cases for "tfrf" and "tfxd" inside "DecodeUUIDBox" uuid. Even though the payload for the LiveServerManifestBox and its sibling is just a manifest, I still think it makes sense to add cases for the two new constants including new subtypes.

However, your focus seems to be on generation of boxes, and I agree that it is clunky.

Since you have an immediate use case, I'm interested in what you think would be nicest for that case.
It sounds like a good idea to have new functions NewUUIDTfxdBox etc as you wrote.
And then maybe have a function NewUUIDBox(uuid, version, flags, payload) for the general case?
UnknownPayload is not the best name if the UUID is known, but if it is explained in a comment
that the new UUIDs are matched to it, a new name or an alternative rawPayload is not needed?

@tobbee
Copy link
Collaborator

tobbee commented Oct 3, 2023

@julijane I haven't seen any suggestion from you to add new box creation functions, so I assume you're fine for the moment?

It is a small improvement to get the references and the UUIDs into the code, so please rebase on the current master branch so that we can merge this PR in a clean way.

@tobbee tobbee merged commit 9573cca into Eyevinn:master Oct 8, 2023
7 checks passed
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.

2 participants