Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1682253005:
Fix SetRates for encoders with internal sources. (Closed)
|
Created:
4 years, 10 months ago by noahric Modified:
4 years, 8 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhengzhonghou_agora.io, video-team_agora.io, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix SetRates for encoders with internal sources.
An earlier change moved SetRates to happen on every input frame, but
encoders with internal sources don't receive input frames, so they
weren't getting updated bitrate (and framerate) signals.
BUG=
Committed: https://crrev.com/d4badbcb6d46fb268f2c9256701f207c6534012d
Cr-Commit-Position: refs/heads/master@{#12354}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Added a fake InternalSourceEncoder #Patch Set 3 : Use encoder_has_internal_source #Patch Set 4 : sync #
Total comments: 1
Patch Set 5 : Removed fake_encoder. #
Messages
Total messages: 23 (8 generated)
noahric@chromium.org changed reviewers: + pbos@chromium.org
Running trybots now to check for lock inversion in holding encoder_crit_ during SetChannelParameters. New unit test seems happy, though :)
Description was changed from ========== Fix SetRates for encoders with internal sources. An earlier change moved SetRates to happen on every input frame, but encoders with internal sources don't receive input frames, so they weren't getting updated bitrate (and framerate) signals. BUG= ========== to ========== Fix SetRates for encoders with internal sources. An earlier change moved SetRates to happen on every input frame, but encoders with internal sources don't receive input frames, so they weren't getting updated bitrate (and framerate) signals. BUG= ==========
noahric@chromium.org changed reviewers: + thaloun@chromium.org
Actually, will that even catch lock inversion? Is there anything besides the test I added that has a fuller integration setup that could test for such issues?
pbos@webrtc.org changed reviewers: + pbos@webrtc.org, stefan@webrtc.org
It'll have the test coverage of our end-to-end tests which will acquire this lock regardless. Can you see if you can implement a fake webrtc/test/fake_encoder.h that has internal_source and runs its own thread producing the content to have some end-to-end coverage? I believe this test should go into webrtc/video/end_to_end_tests.cc. Sync with me in person if this helps. Also, feel free to track a bug for this. https://codereview.webrtc.org/1682253005/diff/1/webrtc/modules/video_coding/v... File webrtc/modules/video_coding/video_sender.cc (right): https://codereview.webrtc.org/1682253005/diff/1/webrtc/modules/video_coding/v... webrtc/modules/video_coding/video_sender.cc:216: rtc::CritScope cs(&encoder_crit_); I believe that this lock could block (the network thread, perhaps?) during encoder use. Can you see if we can cache _encoder->InteralSource() under the params_crit_ so that we don't have to acquire this lock for non-internal-source?
On 2016/02/11 10:17:00, pbos-webrtc wrote: > It'll have the test coverage of our end-to-end tests which will acquire this > lock regardless. > > Can you see if you can implement a fake webrtc/test/fake_encoder.h that has > internal_source and runs its own thread producing the content to have some > end-to-end coverage? Added a fake encoder, but didn't use it anywhere yet. I started looking through the e2e tests, or modifying one of the existing ones (to template FakeEncoderT, changing the subclass relationship), but nothing was obviously simple. > I believe this test should go into webrtc/video/end_to_end_tests.cc. Sync with > me in person if this helps. > > Also, feel free to track a bug for this. > > https://codereview.webrtc.org/1682253005/diff/1/webrtc/modules/video_coding/v... > File webrtc/modules/video_coding/video_sender.cc (right): > > https://codereview.webrtc.org/1682253005/diff/1/webrtc/modules/video_coding/v... > webrtc/modules/video_coding/video_sender.cc:216: rtc::CritScope > cs(&encoder_crit_); > I believe that this lock could block (the network thread, perhaps?) during > encoder use. Can you see if we can cache _encoder->InteralSource() under the > params_crit_ so that we don't have to acquire this lock for non-internal-source? There's already a field for it, turns out, so I switched over to that. Let me know how that looks.
Ah, I added that field for another optimization elsewhere, code looks good to me, just needs testing. Maybe we could take a look at a test together Monday or so, I think I'll be off quite early tomorrow.
isheriff@chromium.org changed reviewers: + isheriff@chromium.org
Can we get this in ? Chromoting is beginning to depend on this. Thanks!
On 2016/04/06 21:32:15, Irfan wrote: > Can we get this in ? Chromoting is beginning to depend on this. > > Thanks! If pbos LGTMs without the integration test hookup, them I'm fine getting it in. Peter? Just did a sync+upload and I'm doing a new git cl try. If that's green, I can submit tomorrow with peter's blessing.
lg overall, don't see the purpose of the internal source decoder, is there something missing "git add"? https://codereview.chromium.org/1682253005/diff/50001/webrtc/test/fake_encoder.h File webrtc/test/fake_encoder.h (right): https://codereview.chromium.org/1682253005/diff/50001/webrtc/test/fake_encode... webrtc/test/fake_encoder.h:87: // An encoder that is marked as internal source, so ignores everything but This class just looks like dead code, if it's not used in tests I think you can remove it?
On 2016/04/07 12:20:22, pbos wrote: > lg overall, don't see the purpose of the internal source decoder, is there > something missing "git add"? > > https://codereview.chromium.org/1682253005/diff/50001/webrtc/test/fake_encoder.h > File webrtc/test/fake_encoder.h (right): > > https://codereview.chromium.org/1682253005/diff/50001/webrtc/test/fake_encode... > webrtc/test/fake_encoder.h:87: // An encoder that is marked as internal source, > so ignores everything but > This class just looks like dead code, if it's not used in tests I think you can > remove it? Here's the relevant part of the conversation history. Sorry I let this sit for so long :( I can remove it if you want, but I still think someone should be adding tests for internal source, and this is at least part of the way there. pbos: Can you see if you can implement a fake webrtc/test/fake_encoder.h that has internal_source and runs its own thread producing the content to have some end-to-end coverage? noahric: Added a fake encoder, but didn't use it anywhere yet. I started looking through the e2e tests, or modifying one of the existing ones (to template FakeEncoderT, changing the subclass relationship), but nothing was obviously simple. pbos: Maybe we could take a look at a test together Monday or so, I think I'll be off quite early tomorrow.
On 2016/04/08 17:15:36, noahric wrote: > On 2016/04/07 12:20:22, pbos wrote: > > lg overall, don't see the purpose of the internal source decoder, is there > > something missing "git add"? > > > > > https://codereview.chromium.org/1682253005/diff/50001/webrtc/test/fake_encoder.h > > File webrtc/test/fake_encoder.h (right): > > > > > https://codereview.chromium.org/1682253005/diff/50001/webrtc/test/fake_encode... > > webrtc/test/fake_encoder.h:87: // An encoder that is marked as internal > source, > > so ignores everything but > > This class just looks like dead code, if it's not used in tests I think you > can > > remove it? > > Here's the relevant part of the conversation history. Sorry I let this sit for > so long :( > > I can remove it if you want, but I still think someone should be adding tests > for internal source, and this is at least part of the way there. > > pbos: > Can you see if you can implement a fake webrtc/test/fake_encoder.h that has > internal_source and runs its own thread producing the content to have some > end-to-end coverage? > > noahric: > Added a fake encoder, but didn't use it anywhere yet. I started looking through > the e2e tests, or modifying one of the existing ones (to template FakeEncoderT, > changing the subclass relationship), but nothing was obviously simple. > > pbos: > Maybe we could take a look at a test together Monday or so, I think I'll be off > quite early tomorrow. Yeah, maybe we can skip that for now to get this in. Can you revert the fake_encoder changes since they're dead code? If you wanna add a TODO to implement them then you can refer to this CL where they are present. internal_source might not be the way going forward so it may be made redundant by future refactoring that separates the video-producing part and transports/packetization.
On 2016/04/11 14:28:50, pbos wrote: > On 2016/04/08 17:15:36, noahric wrote: > > On 2016/04/07 12:20:22, pbos wrote: > > > lg overall, don't see the purpose of the internal source decoder, is there > > > something missing "git add"? > > > > > > > > > https://codereview.chromium.org/1682253005/diff/50001/webrtc/test/fake_encoder.h > > > File webrtc/test/fake_encoder.h (right): > > > > > > > > > https://codereview.chromium.org/1682253005/diff/50001/webrtc/test/fake_encode... > > > webrtc/test/fake_encoder.h:87: // An encoder that is marked as internal > > source, > > > so ignores everything but > > > This class just looks like dead code, if it's not used in tests I think you > > can > > > remove it? > > > > Here's the relevant part of the conversation history. Sorry I let this sit for > > so long :( > > > > I can remove it if you want, but I still think someone should be adding tests > > for internal source, and this is at least part of the way there. > > > > pbos: > > Can you see if you can implement a fake webrtc/test/fake_encoder.h that has > > internal_source and runs its own thread producing the content to have some > > end-to-end coverage? > > > > noahric: > > Added a fake encoder, but didn't use it anywhere yet. I started looking > through > > the e2e tests, or modifying one of the existing ones (to template > FakeEncoderT, > > changing the subclass relationship), but nothing was obviously simple. > > > > pbos: > > Maybe we could take a look at a test together Monday or so, I think I'll be > off > > quite early tomorrow. > > Yeah, maybe we can skip that for now to get this in. Can you revert the > fake_encoder changes since they're dead code? If you wanna add a TODO to > implement them then you can refer to this CL where they are present. > internal_source might not be the way going forward so it may be made redundant > by future refactoring that separates the video-producing part and > transports/packetization. Ok, removed fake_encoder. PTAL.
lgtm
The CQ bit was checked by noahric@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1682253005/70001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1682253005/70001
Message was sent while issue was closed.
Description was changed from ========== Fix SetRates for encoders with internal sources. An earlier change moved SetRates to happen on every input frame, but encoders with internal sources don't receive input frames, so they weren't getting updated bitrate (and framerate) signals. BUG= ========== to ========== Fix SetRates for encoders with internal sources. An earlier change moved SetRates to happen on every input frame, but encoders with internal sources don't receive input frames, so they weren't getting updated bitrate (and framerate) signals. BUG= ==========
Message was sent while issue was closed.
Committed patchset #5 (id:70001)
Message was sent while issue was closed.
Description was changed from ========== Fix SetRates for encoders with internal sources. An earlier change moved SetRates to happen on every input frame, but encoders with internal sources don't receive input frames, so they weren't getting updated bitrate (and framerate) signals. BUG= ========== to ========== Fix SetRates for encoders with internal sources. An earlier change moved SetRates to happen on every input frame, but encoders with internal sources don't receive input frames, so they weren't getting updated bitrate (and framerate) signals. BUG= Committed: https://crrev.com/d4badbcb6d46fb268f2c9256701f207c6534012d Cr-Commit-Position: refs/heads/master@{#12354} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d4badbcb6d46fb268f2c9256701f207c6534012d Cr-Commit-Position: refs/heads/master@{#12354} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
