Skip to content
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

Dolby/ott #485

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Conversation

ShaoWeiguo
Copy link

Support Dolby Codecs in Bento4

muxer: adding support DD, DD+, AC-4 and Dolby Vision
fragment, mp4info, mp42hls etc: fix the issues for Dolby codecs
manifest: support Dolby codecs in DASH and HLS manifest

@lgtm-com
Copy link

lgtm-com bot commented Mar 27, 2020

This pull request introduces 7 alerts when merging 5eaad11 into 403a2c4 - view on LGTM.com

new alerts:

  • 3 for Multiplication result converted to larger type
  • 2 for Comparison result is always the same
  • 1 for Unused local variable
  • 1 for Unsigned comparison to zero

@krzemienski
Copy link

krzemienski commented Apr 5, 2020

this is awesome work @ShaoWeiguo ...if there is anything i can do to help this the work please let me know! I was going to try and work on manifest: support Dolby codecs in DASH and HLS manifest but seems like you have handled.

I have a decent amount of encoding ladder artifacts that were encoded by x265 and x264 that range from pq->sdr and also from 4k down that I can run this branch against if that is any help!

@barbibulle
Copy link
Contributor

Thanks for the PR. I will most likely merge the new features in separate discrete releases so that this can be tested incrementally.

@ShaoWeiguo
Copy link
Author

@krzemienski Sure, thanks.

@ShaoWeiguo
Copy link
Author

Dolby made some changes about Dolby Vision signaling in MP4. So please change the codes below in Mp4Mux.cpp.

  • line 1218, const AP4_UI32 dv_major_version = 2; -- > const AP4_UI32 dv_major_version = 1;
  • line 1219, const AP4_UI32 dv_minor_version = 1; --> const AP4_UI32 dv_minor_version = 0;
  • line 1757, const AP4_UI32 dv_major_version = 2; -- > const AP4_UI32 dv_major_version = 1;
  • line 1758, const AP4_UI32 dv_minor_version = 1; --> const AP4_UI32 dv_minor_version = 0;

@barbibulle
Copy link
Contributor

@ShaoWeiguo The PR was made before the code in the repo was updated to python3. Would it be possible to rebase and update your PR so that the conflicts can be resolved?

@ShaoWeiguo
Copy link
Author

I see. I will update the PR and make it base on latest commit. Need several days effort, and wish there is no big changes in master branch during this period.

# Conflicts:
#	Source/C++/Apps/Mp42Hls/Mp42Hls.cpp
#	Source/C++/Apps/Mp4Fragment/Mp4Fragment.cpp
#	Source/C++/Apps/Mp4Mux/Mp4Mux.cpp
#	Source/Python/utils/mp4-dash.py
#	Source/Python/utils/mp4-hls.py
@lgtm-com
Copy link

lgtm-com bot commented May 20, 2020

This pull request introduces 1 alert when merging ff590f0 into 17e26f7 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@ShaoWeiguo
Copy link
Author

@barbibulle I have updated the PR based on latest commit (17e26f7). And I also fixed the Dolby Vision version issue mentioned above. If anything need, please let me know.

@barbibulle
Copy link
Contributor

@barbibulle I have updated the PR based on latest commit (17e26f7). And I also fixed the Dolby Vision version issue mentioned above. If anything need, please let me know.

Thanks @ShaoWeiguo, I'll work on this ASAP.

@jimmymic
Copy link
Contributor

@barbibulle Is there anything we can do to assist getting this PR completed?

@barbibulle
Copy link
Contributor

@jimmymic I have started merging the changing in an integration branch. It is taking me some time because the PR is pretty large, so I have to check that merging it won't break something else. I'm 50% done, and will keep working on it next week, so I would expect something soon.

@ShaoWeiguo
Copy link
Author

@barbibulle Any update?

@barbibulle
Copy link
Contributor

Hi. Sorry about the delay. Most the merging is done but I have been delayed on finishing it. I will attend to it ASAP and let you know when the merged code is in the master branch.

@ShaoWeiguo
Copy link
Author

finish the merge pull request?

@barbibulle
Copy link
Contributor

Hi. I have merged all the changes to the C++ code. I have not yet been able to merge the python changes, because that's more difficult. The changes don't match the code style/architecture directly, so I'll likely will have to rewrite something equivalent that doesn't deviate so much. The issue I'm facing is that there's no test suite for the Dolby features, so I don't know how to test that my version would be equivalent to what's in the PR. Would it be possible to come up with a few files and scripts to have at least some basic checks/tests?

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.

4 participants