First stage adding additional parameters to match spec #42
Conversation
# Conflicts: # mpd/mpd_test.go Fixup tests.
mpd/mpd.go
Outdated
| SegmentProfiles *string `xml:"segmentProfiles,attr"` | ||
| Codecs *string `xml:"codecs,attr"` | ||
| MaximumSAPPeriod *string `xml:"MaximumSAPPeriod,attr"` | ||
| StartWithSAP *string `xml:"startWithSAP,attr"` |
There was a problem hiding this comment.
was an int64 before are we planning to change the type.
|
@bcasinclair Hi Andrew I've added the IDs to AdaptationSets and moved in your CommonAttributesAndElements and removed the ones in the AS, this moved the order of XML a bit. I've fixed up the tests also. |
|
@mbowBC makes sense to me. I think what is there is a good start. There is a lot more that could be added |
mpd/segment_timeline_test.go
Outdated
| "testing" | ||
| "time" | ||
|
|
||
| "fmt" |
There was a problem hiding this comment.
Import spacing needs fixing
mpd/segment_timeline_test.go
Outdated
| found, err := tc.In.WriteToString() | ||
| require.NoError(t, err) | ||
| expected := testfixtures.LoadFixture("fixtures/" + tc.Out) | ||
| fmt.Println(expected) |
There was a problem hiding this comment.
Can get rid of these now
| } | ||
|
|
||
| type Representation struct { | ||
| ID *string `xml:"id,attr"` // Audio + Video |
There was a problem hiding this comment.
It's more normal to have CommonAttributesAndElements as the first line in the struct - this can go back to where it was
There was a problem hiding this comment.
Normal I'd agree but in this case, the inner CommonAttributesAndElements would get picked up first by XMLMarshal and it's ordering by struct order. (no guarantee this will always be the case but..) XML doesn't care about the order of attr , but it's more human readable to have the ID at the front IMO.
mpd/mpd.go
Outdated
| MimeType *string `xml:"mimeType,attr"` | ||
| SegmentProfiles *string `xml:"segmentProfiles,attr"` | ||
| Codecs *string `xml:"codecs,attr"` | ||
| MaximumSAPPeriod *string `xml:"MaximumSAPPeriod,attr"` |
There was a problem hiding this comment.
Why is the XML attr for this capitalised (but the others aren't)?
There was a problem hiding this comment.
?? can you come point this out I'm being blind.
There was a problem hiding this comment.
xml:"MaximumSAPPeriod,attr instead of xml:"maximumSAPPeriod,attr
There was a problem hiding this comment.
good spot, I couldn't see that 👓 👍
| // startWithSAP - Starts With SAP (i.e. 1). | ||
| // lang - Language (i.e. en). | ||
| func (period *Period) AddNewAdaptationSetAudio(mimeType string, segmentAlignment bool, startWithSAP int64, lang string) (*AdaptationSet, error) { | ||
| func (period *Period) AddNewAdaptationSetAudio(id string, mimeType string, segmentAlignment bool, startWithSAP int64, lang string) (*AdaptationSet, error) { |
There was a problem hiding this comment.
This is effectively making id a required field now, what's the reason for this?
There was a problem hiding this comment.
@id is mandatory on dynamic streams (I think)
AdaptationSet@id is an optional parameter
davemevans/dash.js@7043922
and was found from http://standards.iso.org/ittf/PubliclyAvailableStandards/MPEG-DASH_schema_files/DASH-MPD.xsd
search for
<xs:attribute name="id" type="xs:unsignedInt"/>
mpd/mpd.go
Outdated
| MimeType *string `xml:"mimeType,attr"` | ||
| SegmentProfiles *string `xml:"segmentProfiles,attr"` | ||
| Codecs *string `xml:"codecs,attr"` | ||
| MaximumSAPPeriod *string `xml:"MaximumSAPPeriod,attr"` |
There was a problem hiding this comment.
?? can you come point this out I'm being blind.
This lib needs quite a few more parameters to better model all the features of the ISO 23009-1:2014 spec and also DASH-IF guidelines.
This is a first pass in adding the CommonAttributesAndElements that exist across AdaptationSets and Representations.