-
Notifications
You must be signed in to change notification settings - Fork 511
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
[DASH] Template based url construction with RepresentationID. #848
base: main
Are you sure you want to change the base?
Conversation
e21575b
to
83cb6a2
Compare
Hi @kqyang, any thoughts on this PR? |
@sr1990 Thanks for working on it. Yes, that is a great feature. (And sorry for late reply, super busy recently..) I'll try to look at the PR some time this week. |
Sounds good. Thanks. Once I know that the approach is correct, I can move ahead with adding unit tests and documentation. |
Ping |
bump |
|
||
/// Used as Representation id for template based url construction using | ||
/// $RepresentationID$. | ||
std::string rep_id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be added as a muxer option. Will update the PR soon.
I'm trying to catch up on the PR backlog as we work to revive the project. I agree with this feature in principle, but haven't reviewed the code yet. Please merge main into your branch and fix conflicts, and make whatever other changes you're considering (I see a comment you left about moving |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the adaptation set level segment template seems fine, but non-numeric representation IDs does not seem fine. We could consider allowing the user to use manually specified numeric representation IDs, but it seems like it would be safer to leave representation IDs automatically generated and instead provide a facility to use the generated representation ID in the output filename.
@@ -133,6 +133,8 @@ struct StreamDescriptor { | |||
bool dash_only = false; | |||
/// Set to true to indicate that the stream is for hls only. | |||
bool hls_only = false; | |||
|
|||
std::string rep_id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in my experience allowing non-numeric representation IDs is bound to cause problems, last time I tried I believe ExoPlayer failed to parse non-numeric representation IDs for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of having the user specify the representation ID, what about allowing to specify using the representation ID in the output filename and then substitute in the computed representation ID by packager.
@@ -66,6 +66,10 @@ class XmlNode { | |||
/// @param id is the ID for this element. | |||
void SetId(uint32_t id); | |||
|
|||
/// Sets 'id=rep_id' attribute. | |||
/// @param id is the ID for this element. | |||
void SetIdString(std::string id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a problem for the same reason I mentioned elsewhere, even internally id is expected to be uint. So the option of passing in an alternate string ID isn't great.
Also exoplayer seems to be parsing Representation id as a string
|
Trying to implement template based url construction that uses$RepresentationID$ . This will add SegmentTemplate below adaptationSet instead of every Representation.
I have added a new stream descriptor field called rep_id that takes in the representation id.
Example:
and following is the generated mpd:
This is similar to MP4Box where ids are specified along with tracks:
Example:
@kqyang please let me know what you think about this approach.