Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Closed
leandromoreira wants to merge 3 commits into zencoder:master from globocom:master

Conversation

@leandromoreira
Copy link

@leandromoreira leandromoreira commented Mar 9, 2018
edited
Loading

  • Changed the NewMPD constructor to accept a map of attributes (easier to expand to new attributes)
  • Added a new helper function EmptyStrPtr to work with an empty string which I need to be seen as nil pointers.
  • Added a new test case for availabilityStartTime
  • Fixed existent tests

miyukki reacted with thumbs up emoji
Copy link
Author

What do you think of these changes?

mpd/mpd.go Outdated
Type: Strptr(defaultValue(attributes["type"], "static")),
MediaPresentationDuration: EmptyStrPtr(attributes["mediaPresentationDuration"]),
MinBufferTime: EmptyStrPtr(attributes["minBufferTime"]),
AvailabilityStartTime: EmptyStrPtr(attributes["availabilityStartTime"]),
Copy link
Contributor

@stuarthicks stuarthicks Mar 19, 2018

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.

leandromoreira reacted with thumbs up emoji
Copy link
Contributor

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?

leandromoreira reacted with thumbs up emoji

Copy link
Author

leandromoreira commented Mar 21, 2018
edited
Loading

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:

  1. a new constructor
func NewDynamicMPD(profile DashProfile, mediaPresentationDuration string, minBufferTime string, availabilityStartTime string) *MPD {
}
  1. how about making this new constructor to receive the MPD struct and 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.

Copy link
Contributor

miyukki commented Jun 15, 2018
edited
Loading

@stuarthicks @leandromoreira
I think another solution to resolve this issue.

  1. Create MPDAttr interface 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}
}
  1. NewMPD receive the MPDAttr as variable length argument and set the strptr from attributes. (minBufferTime is 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.

leandromoreira and stuarthicks reacted with thumbs up emoji

Copy link
Contributor

@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?

Copy link
Contributor

miyukki commented Jun 26, 2018

@stuarthicks @leandromoreira
I opened another PR to resolve this, for using this feature in my project. #51

leandromoreira reacted with thumbs up emoji

Copy link
Contributor

Replaced by #51

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

Reviewers

@stuarthicks stuarthicks stuarthicks requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /