Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2573453002:
Add multithreaded fake encoder and corresponding FlexFEC VideoSendStreamTest. (Closed)
|
Created:
4 years ago by brandtr Modified:
4 years ago Reviewers:
stefan-webrtc CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd multithreaded fake encoder and corresponding FlexFEC VideoSendStreamTest.
This test would have found the issue that was fixed in
https://codereview.webrtc.org/2562983002/.
BUG=webrtc:5654
Review-Url: https://codereview.webrtc.org/2573453002
Cr-Commit-Position: refs/heads/master@{#15675}
Committed: https://chromium.googlesource.com/external/webrtc/+/696c9c6b64234a34631bb39c758996bb08449fe4
Patch Set 1 #Patch Set 2 : Add the test here. #
Total comments: 9
Patch Set 3 : holmer response 1. #Patch Set 4 : holmer response 2. #Patch Set 5 : Fix android. #
Messages
Total messages: 22 (9 generated)
Move test to https://codereview.webrtc.org/2573453002/
Patchset #2 (id:20001) has been deleted
Add the test here.
stefan@webrtc.org changed reviewers: + stefan@webrtc.org
https://codereview.webrtc.org/2573453002/diff/40001/webrtc/test/fake_encoder.cc File webrtc/test/fake_encoder.cc (right): https://codereview.webrtc.org/2573453002/diff/40001/webrtc/test/fake_encoder.... webrtc/test/fake_encoder.cc:282: return FakeEncoder::Encode(input_image, codec_specific_info, frame_types); Doesn't this mean that the encoder will produce twice the bitrate it's been configured to produce? https://codereview.webrtc.org/2573453002/diff/40001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2573453002/diff/40001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:368: if (codec == "MTFE") { I think this is a bit ugly. Can we pass in the objects instead and have the caller create them?
https://codereview.webrtc.org/2573453002/diff/40001/webrtc/test/fake_encoder.cc File webrtc/test/fake_encoder.cc (right): https://codereview.webrtc.org/2573453002/diff/40001/webrtc/test/fake_encoder.... webrtc/test/fake_encoder.cc:282: return FakeEncoder::Encode(input_image, codec_specific_info, frame_types); On 2016/12/12 15:18:28, stefan-webrtc (holmer) wrote: > Doesn't this mean that the encoder will produce twice the bitrate it's been > configured to produce? Not sure why it would do that? MultiThreadedFakeEncoder::Encode posts a task to some queue that when run will call MultiThreadedFakeEncoder::EncodeCallback, which in its turn will run FakeEncoder::Encode. That is, the MultiThreadedFakeEncoder just distributes calls to ::Encode onto two different queues. https://codereview.webrtc.org/2573453002/diff/40001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2573453002/diff/40001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:368: if (codec == "MTFE") { On 2016/12/12 15:18:28, stefan-webrtc (holmer) wrote: > I think this is a bit ugly. Can we pass in the objects instead and have the > caller create them? We could, but it seems that I would still need to pass in the codec name string, to be set here: https://cs.chromium.org/chromium/src/third_party/webrtc/video/video_send_stre.... This string is used when creating the coder, and DCHECKS if I don't set it.
https://codereview.webrtc.org/2573453002/diff/40001/webrtc/test/fake_encoder.cc File webrtc/test/fake_encoder.cc (right): https://codereview.webrtc.org/2573453002/diff/40001/webrtc/test/fake_encoder.... webrtc/test/fake_encoder.cc:282: return FakeEncoder::Encode(input_image, codec_specific_info, frame_types); On 2016/12/13 11:32:23, brandtr wrote: > On 2016/12/12 15:18:28, stefan-webrtc (holmer) wrote: > > Doesn't this mean that the encoder will produce twice the bitrate it's been > > configured to produce? > > Not sure why it would do that? > > MultiThreadedFakeEncoder::Encode posts a task to some queue that when run will > call MultiThreadedFakeEncoder::EncodeCallback, which in its turn will run > FakeEncoder::Encode. That is, the MultiThreadedFakeEncoder just distributes > calls to ::Encode onto two different queues. Yes, you are right, I misunderstood the code. https://codereview.webrtc.org/2573453002/diff/40001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2573453002/diff/40001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:368: if (codec == "MTFE") { On 2016/12/13 11:32:23, brandtr wrote: > On 2016/12/12 15:18:28, stefan-webrtc (holmer) wrote: > > I think this is a bit ugly. Can we pass in the objects instead and have the > > caller create them? > > We could, but it seems that I would still need to pass in the codec name string, > to be set here: > https://cs.chromium.org/chromium/src/third_party/webrtc/video/video_send_stre.... > This string is used when creating the coder, and DCHECKS if I don't set it. Right, but you can then just call it H264, instead of something unknown as MTFE.
https://codereview.webrtc.org/2573453002/diff/40001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2573453002/diff/40001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:368: if (codec == "MTFE") { On 2016/12/13 13:04:46, stefan-webrtc (holmer) wrote: > On 2016/12/13 11:32:23, brandtr wrote: > > On 2016/12/12 15:18:28, stefan-webrtc (holmer) wrote: > > > I think this is a bit ugly. Can we pass in the objects instead and have the > > > caller create them? > > > > We could, but it seems that I would still need to pass in the codec name > string, > > to be set here: > > > https://cs.chromium.org/chromium/src/third_party/webrtc/video/video_send_stre.... > > This string is used when creating the coder, and DCHECKS if I don't set it. > > Right, but you can then just call it H264, instead of something unknown as MTFE. Yep. I also made it a FakeH264Encoder, so I'm not lying to the underlying code about what type of encoder it is supposed to be.
https://codereview.webrtc.org/2573453002/diff/40001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2573453002/diff/40001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:368: if (codec == "MTFE") { On 2016/12/14 10:13:50, brandtr wrote: > On 2016/12/13 13:04:46, stefan-webrtc (holmer) wrote: > > On 2016/12/13 11:32:23, brandtr wrote: > > > On 2016/12/12 15:18:28, stefan-webrtc (holmer) wrote: > > > > I think this is a bit ugly. Can we pass in the objects instead and have > the > > > > caller create them? > > > > > > We could, but it seems that I would still need to pass in the codec name > > string, > > > to be set here: > > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/video/video_send_stre.... > > > This string is used when creating the coder, and DCHECKS if I don't set it. > > > > Right, but you can then just call it H264, instead of something unknown as > MTFE. > > Yep. I also made it a FakeH264Encoder, so I'm not lying to the underlying code > about what type of encoder it is supposed to be. This works, but instead of only passing in the payload name you could pass in the payload name and the encoder object and avoid the string comparisons and the odd codec name
Patchset #4 (id:80001) has been deleted
https://codereview.webrtc.org/2573453002/diff/40001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2573453002/diff/40001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:368: if (codec == "MTFE") { On 2016/12/14 11:42:31, stefan-webrtc (holmer) wrote: > On 2016/12/14 10:13:50, brandtr wrote: > > On 2016/12/13 13:04:46, stefan-webrtc (holmer) wrote: > > > On 2016/12/13 11:32:23, brandtr wrote: > > > > On 2016/12/12 15:18:28, stefan-webrtc (holmer) wrote: > > > > > I think this is a bit ugly. Can we pass in the objects instead and have > > the > > > > > caller create them? > > > > > > > > We could, but it seems that I would still need to pass in the codec name > > > string, > > > > to be set here: > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/video/video_send_stre.... > > > > This string is used when creating the coder, and DCHECKS if I don't set > it. > > > > > > Right, but you can then just call it H264, instead of something unknown as > > MTFE. > > > > Yep. I also made it a FakeH264Encoder, so I'm not lying to the underlying code > > about what type of encoder it is supposed to be. > > This works, but instead of only passing in the payload name you could pass in > the payload name and the encoder object and avoid the string comparisons and the > odd codec name Done. This made the test instantiations more verbose, however.
lgtm
The CQ bit was checked by brandtr@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_mips_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_mips_db...) android_compile_x86_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...)
The CQ bit was checked by brandtr@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2573453002/#ps120001 (title: "Fix android.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1482150743908800,
"parent_rev": "b78306a7d339bfcc3c4a6043023ab57d95444b99", "commit_rev":
"696c9c6b64234a34631bb39c758996bb08449fe4"}
Message was sent while issue was closed.
Description was changed from ========== Add multithreaded fake encoder and corresponding FlexFEC VideoSendStreamTest. This test would have found the issue that was fixed in https://codereview.webrtc.org/2562983002/. BUG=webrtc:5654 ========== to ========== Add multithreaded fake encoder and corresponding FlexFEC VideoSendStreamTest. This test would have found the issue that was fixed in https://codereview.webrtc.org/2562983002/. BUG=webrtc:5654 Review-Url: https://codereview.webrtc.org/2573453002 Cr-Commit-Position: refs/heads/master@{#15675} Committed: https://chromium.googlesource.com/external/webrtc/+/696c9c6b64234a34631bb39c7... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/696c9c6b64234a34631bb39c7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
