-
Notifications
You must be signed in to change notification settings - Fork 68
Add availabilityStartTime attribute support #47
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
Conversation
What do you think of these changes?
mpd/mpd.go
Outdated
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.
How about exporting these supported attributes as constants? eg AttrAvailabilityStartTime.
Apologies for taking a while to respond to this. I've been discussing this change with others members of the team.
I like the new constructor signature from the perspective of it's shorter and neater, however there is a drawback to the map[string]string approach (aside from it being a breaking API change) in that the attributes that are available are now less discoverable (no IDE will be able to know which strings should go here). Another consideration is that the constructor signature served to declare which fields are mandatory. Using an attribute map also loses this hinting.
How about we make this two constructors? We could leave the previous constructor as-is, and add a new one NewDynamicMPD that requires and sets the availabilityStartTime attribute? This way we can avoid making a breaking API change, and we can keep using a non-map method signature to make the required parameters more obvious.
(At some point in the future, we could consider other ideas such as using a builder-pattern for mpd construction)
What do you think?
Hi @stuarthicks no problems about the delay, we're all usually too busy =)
I liked your suggestion, I'll treat it as our first suggestion:
- a new constructor
func NewDynamicMPD(profile DashProfile, mediaPresentationDuration string, minBufferTime string, availabilityStartTime string) *MPD { }
- how about making this new constructor to receive the
MPD structand we can even exclude the profile argument and retrieve all the others attributes from it and if we want to add a new attribute we would add it first to the struct and treat internally.
A legit case could be to create an in-memory MPD struct to dump as an XML MPD.
func NewDynamicMPD(mpd *MPD) *MPD { }
Thanks for all the input but besides all this, I'd like to propose as well an extension point, for those who need a specific attribute but can't wait for a new release (PR check, Merge, Release) and therefore we'd keep this IDE and strongly typed lib friendly but it still opened.
// something similar to this P1 = MPD.raw()[0]["Period"]; P2 = MPD.raw()[1]["Period"]; fmt.Printf(P1["start"]);
PS: I'm kinda new to golang so forgive me if my ideas are not so tied to the go's style.
@stuarthicks @leandromoreira
I think another solution to resolve this issue.
- Create
MPDAttrinterface and implement it as each attributes.
type MPDAttr interface { GetStrptr() *string } type AvailabilityStartTime struct { strptr *string } func (attr *AvailabilityStartTime) GetStrptr() *string { return attr.strptr } func AvailabilityStartTimeAttr(value string) MPDAttr { return &AvailabilityStartTime{strptr: &value} } type MediaPresentationDuration struct { strptr *string } func (attr *MediaPresentationDuration) GetStrptr() *string { return attr.strptr } func MediaPresentationDurationAttr(value string) MPDAttr { return &MediaPresentationDuration{strptr: &value} }
NewMPDreceive theMPDAttras variable length argument and set the strptr from attributes. (minBufferTimeis mandatory)
func NewMPD(profile DashProfile, minBufferTime string, attributes ...MPDAttr) *MPD { period := &Period{} mpd := &MPD{ XMLNs: Strptr("urn:mpeg:dash:schema:mpd:2011"), Profiles: Strptr((string)(profile)), Type: Strptr("static"), MinBufferTime: Strptr(minBufferTime), period: period, Periods: []*Period{period}, } for _, attr := range attributes { switch attr.(type) { case *AvailabilityStartTime: mpd.AvailabilityStartTime = attr.GetStrptr() case *MediaPresentationDuration: mpd.MediaPresentationDuration = attr.GetStrptr() } } return mpd } func Example() { NewMPD(DASH_PROFILE_LIVE, "PT5S") NewMPD(DASH_PROFILE_LIVE, "PT5S", AvailabilityStartTimeAttr("PT0S"), MediaPresentationDurationAttr("PT0S")) }
If you are afraid breaking API change, using NewDynamicMPD instead of.
@miyukki That's a great suggestion, thanks! Very elegant. The only change I'd suggest is a stylistic one, and that would be to make the attributes have Attr as a prefix in the names, rather than a suffix, eg. AttrAvailabilityStartTime. It groups them nicely together when searching for attributes, in the same way errors are often named like ErrFoo.
@leandromoreira Are you happy to re-work this PR like that? What do you think about this new idea?
@stuarthicks @leandromoreira
I opened another PR to resolve this, for using this feature in my project. #51
Replaced by #51
Uh oh!
There was an error while loading. Please reload this page.
NewMPDconstructor to accept a map of attributes (easier to expand to new attributes)EmptyStrPtrto work with an empty string which I need to be seen asnilpointers.