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

Conflicts: Fix conflicts with SVT-AV1 #429

Merged
merged 2 commits into from
Dec 10, 2019

Conversation

1480c1
Copy link
Member

@1480c1 1480c1 commented Nov 21, 2019

Should the prefix be EbHevc or eb_hevc_?

@tianjunwork tianjunwork added build Build related changes Clean up A cleaner implementation or improved functionality labels Nov 22, 2019
Copy link
Contributor

@intelmark intelmark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems OK. Some cleanup (tabs, spaces) changes included as well. Not sure why Travis failed though

@Austin-Hu
Copy link
Contributor

Hi @1480c1 ,

Thanks for your contribution on this to make the encoders aligned in the future! I believe you have some script or rapid way for these changes. So I prefer to hold this PR for some time, till the PR #70 is merged after v1.4.2 as it's a big change targetting for the CRF feature. After PR #70 is merged, you can amend this PR and we will make it go through the merging process. How do you think about it?

Thanks for understanding!

@1480c1
Copy link
Member Author

1480c1 commented Nov 22, 2019

I think it may be that the particular cpu that the job ran on was probably very slow.

Hi @1480c1 ,

Thanks for your contribution on this to make the encoders aligned in the future! I believe you have some script or rapid way for these changes. So I prefer to hold this PR for some time, till the PR #70 is merged after v1.4.2 as it's a big change targetting for the CRF feature. After PR #70 is merged, you can amend this PR and we will make it go through the merging process. How do you think about it?

Thanks for understanding!

I explicitly made sure that there will be no merge conflicts between #70

You can test this by

git clone https://github.com/OpenVisualCloud/SVT-HEVC.git
cd SVT-HEVC
git fetch origin pull/429/head
git checkout FETCH_HEAD
git fetch origin pull/70/head
git merge FETCH_HEAD

And if there is a merge conflict, git should tell you at the last command

Copy link
Contributor

@tianjunwork tianjunwork left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much Chris @1480c1 for your effort to fix these proactively. My main concern is the mismatch of naming e.g. EbHevccoeff_tbl_AVX2. Will it cause issue to formatting tool when later on if we would like to unify the naming convention as to vp9/av1?

@1480c1
Copy link
Member Author

1480c1 commented Nov 22, 2019

Although I do not think so, I can capitalize the first letter after Hevc to make sure they are PascalCase if wanted

@Austin-Hu
Copy link
Contributor

I think it may be that the particular cpu that the job ran on was probably very slow.

Hi @1480c1 ,
Thanks for your contribution on this to make the encoders aligned in the future! I believe you have some script or rapid way for these changes. So I prefer to hold this PR for some time, till the PR #70 is merged after v1.4.2 as it's a big change targetting for the CRF feature. After PR #70 is merged, you can amend this PR and we will make it go through the merging process. How do you think about it?
Thanks for understanding!

I explicitly made sure that there will be no merge conflicts between #70

You can test this by

git clone https://github.com/OpenVisualCloud/SVT-HEVC.git
cd SVT-HEVC
git fetch origin pull/429/head
git checkout FETCH_HEAD
git fetch origin pull/70/head
git merge FETCH_HEAD

And if there is a merge conflict, git should tell you at the last command

Thanks, @1480c1 ! I confirmed that there isn't any confliction between the 2 PRs. The PR looks good to me, unless you'd rename some with PascalCase.

Signed-off-by: Christopher Degawa <ccom@randomderp.com>
Signed-off-by: Christopher Degawa <ccom@randomderp.com>
@1480c1
Copy link
Member Author

1480c1 commented Nov 26, 2019

force-pushed because I forgot to add --signoff

@tianjunwork tianjunwork changed the title conflicts: Fix conflicts with SVT-AV1 Conflicts: Fix conflicts with SVT-AV1 Nov 26, 2019
@1480c1
Copy link
Member Author

1480c1 commented Dec 5, 2019

@hassount, is this PR okay?

@Austin-Hu, I can open a PR against the crf branch if there are any changed symbols/symbols that need to be changed after this is merged if you want

@Austin-Hu
Copy link
Contributor

@hassount, is this PR okay?

@Austin-Hu, I can open a PR against the crf branch if there are any changed symbols/symbols that need to be changed after this is merged if you want

Hi @1480c1 , the PR looks good to me, and we plan to merge it after v1.4.2 release soon.

@tianjunwork tianjunwork merged commit 0ba230d into OpenVisualCloud:master Dec 10, 2019
@1480c1 1480c1 deleted the conflicts branch December 10, 2019 05:32
1480c1 added a commit to 1480c1/media-autobuild_suite that referenced this pull request Dec 10, 2019
intelmark pushed a commit to intelmark/SVT-HEVC that referenced this pull request Dec 10, 2019
Signed-off-by: Christopher Degawa <ccom@randomderp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Build related changes Clean up A cleaner implementation or improved functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants