Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1824833002:
Remove redefined macros from BitrateAdjuster (Closed)
|
Created:
4 years, 9 months ago by emircan Modified:
4 years, 9 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhengzhonghou_agora.io, video-team_agora.io, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRemove redefined macros from BitrateAdjuster
This CL removes the include that redefines Chrome macros, so Chrome
can include this header in https://codereview.chromium.org/1818903004/.
Committed: https://crrev.com/2df29cb673d38a91c6a08326ae3009aba4feae9a
Cr-Commit-Position: refs/heads/master@{#12095}
Patch Set 1 #
Messages
Total messages: 21 (10 generated)
Description was changed from ========== Remove redefined macro. BUG= ========== to ========== Remove redefined macros from BitrateAdjuster ==========
emircan@chromium.org changed reviewers: + tkchin@chromium.org
Description was changed from ========== Remove redefined macros from BitrateAdjuster ========== to ========== Remove redefined macros from BitrateAdjuster This CL removes the include that redefines Chrome macros, so Chrome can include this header in https://codereview.chromium.org/1818903004/. ==========
PTAL.
tkchin@webrtc.org changed reviewers: + stefan@webrtc.org
tkchin@webrtc.org changed reviewers: + tkchin@webrtc.org
lgtm but I'm not an owner so adding stefan
On 2016/03/22 00:26:10, tkchin_webrtc wrote: > lgtm but I'm not an owner so adding stefan Android failures caused by https://codereview.chromium.org/1818083003/ which has now been reverted. Feel free to ignore those.
This specific change lgtm. However, do we really want Chrome to depend directly on webrtc::BitrateAdjuster? It seems like an interface that we shouldn't expose...
On 2016/03/22 10:29:05, stefan-webrtc (holmer) wrote: > This specific change lgtm. > > However, do we really want Chrome to depend directly on webrtc::BitrateAdjuster? > It seems like an interface that we shouldn't expose... Uncertain. If we don't then they will likely need to write something of their own. They have a different way of plumbing frames to their encoder, so they can't use our encoder (afaict) Also, ours does not compile for Mac due to some c++/stdc++ configuration issues in webrtc at the moment. The longer term fix would probably be to remove the need for bitrateadjuster by modifying the pipeline not to drop frames immediately but first instead to reduce bitrate after send side bwe is default behavior? (e.g. telling encoder to move bitrate "up" or "down").
I see your concerns. Let's discuss these in the Chromium CL that actually reuses code, or in offline email thread. AFAIU, this is a small dependency issue that shouldn't effect other moving parts.
The CQ bit was checked by emircan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1824833002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1824833002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by emircan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1824833002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1824833002/1
Message was sent while issue was closed.
Description was changed from ========== Remove redefined macros from BitrateAdjuster This CL removes the include that redefines Chrome macros, so Chrome can include this header in https://codereview.chromium.org/1818903004/. ========== to ========== Remove redefined macros from BitrateAdjuster This CL removes the include that redefines Chrome macros, so Chrome can include this header in https://codereview.chromium.org/1818903004/. ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Remove redefined macros from BitrateAdjuster This CL removes the include that redefines Chrome macros, so Chrome can include this header in https://codereview.chromium.org/1818903004/. ========== to ========== Remove redefined macros from BitrateAdjuster This CL removes the include that redefines Chrome macros, so Chrome can include this header in https://codereview.chromium.org/1818903004/. Committed: https://crrev.com/2df29cb673d38a91c6a08326ae3009aba4feae9a Cr-Commit-Position: refs/heads/master@{#12095} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/2df29cb673d38a91c6a08326ae3009aba4feae9a Cr-Commit-Position: refs/heads/master@{#12095} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
