Skip to content

Implement to unmarshal PT duration #37

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

Merged
merged 2 commits into from
Oct 31, 2017

Conversation

miyukki
Copy link
Contributor

@miyukki miyukki commented Sep 28, 2017

Refer to #23

go-dash output the MPD with PT duration, but could not parse its notation.
This PR resolve an above issue.
However, It might be including the mistake due to I can not read the full specification of MPEG-DASH.

@philcluff
Copy link
Contributor

Thanks for the PR! I'll take a look today.

@thomshutt
Copy link
Contributor

thomshutt commented Oct 6, 2017

Hey @miyukki - sorry it's taken us a while to look at this!

Overall it looks good, but I noticed a few things:

  • Could we have some error test cases too please? There are some good examples at http://www.datypic.com/sc/xsd/t-xsd_duration.html

  • The above link also has valid cases for when this starts with things other than PT, which will currently be rejected by your parsing code

  • We also need test cases for when the field is an integer/float.

  • I think we probably want to guard against negative durations.

@miyukki miyukki force-pushed the fix-duration-unmarshal branch from 1d1a2ef to 1a12f99 Compare October 8, 2017 15:57
@miyukki
Copy link
Contributor Author

miyukki commented Oct 8, 2017

Thanks your review, and I saw the good examples about xsd:duration.

I changed the parsing code, but I got an issue about duration.
xsd:duration can be represent the duration with Years and Months, but it couldn't convert to golang time.Duration because Years and Months are not fixed length of days.
So I calculate the duration with beginning of unix time.
Please let me know if you have any specifications about it.

@thomshutt
Copy link
Contributor

@miyukki Good point - I can't find anything in the spec about this, so I guess we should just reject durations that contain Years/Months

@miyukki miyukki force-pushed the fix-duration-unmarshal branch from 1a12f99 to fb82bae Compare October 30, 2017 08:41
@miyukki
Copy link
Contributor Author

miyukki commented Oct 30, 2017

@thomshutt Sorry, it took so long. I fixed PR. Please review.

@thomshutt
Copy link
Contributor

Thanks @miyukki, looks great!

@thomshutt thomshutt merged commit 732362d into zencoder:master Oct 31, 2017
@miyukki miyukki deleted the fix-duration-unmarshal branch June 15, 2018 07:45
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.

3 participants