Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2669093004:
Remove codec thread from MediaCodecVideoEncoder. (Closed)
DescriptionRemove codec thread from MediaCodecVideoEncoder.
After this change, all calls to MediaCodecVideoEncoder must be made on
the same task queue. Removes OnCodecThread suffix from methods since it
is no longer meaningful.
BUG=webrtc:6290
Review-Url: https://codereview.webrtc.org/2669093004
Cr-Commit-Position: refs/heads/master@{#16792}
Committed: https://chromium.googlesource.com/external/webrtc/+/02f994b4e9ffdf6fddb0e52818785a1f28c4db26
Patch Set 1 : Update comments, add DCHECK to destructor. #
Total comments: 6
Patch Set 2 : Switch to SequenceTaskChecker. #Patch Set 3 : Minimize diff. #Patch Set 4 : Allow destruction on other thread. #Patch Set 5 : Call release in SimulcastAdapter, destroy WeakPtrFactory in Release. #
Total comments: 4
Patch Set 6 : Update comment, remove unncessary scope. #
Total comments: 6
Messages
Total messages: 45 (29 generated)
The CQ bit was checked by sakal@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== Remove codec thread from MediaCodecVideoEncoder. BUG=webrtc:6290 ========== to ========== Remove codec thread from MediaCodecVideoEncoder. BUG=webrtc:6290 ==========
sakal@webrtc.org changed reviewers: + magjed@webrtc.org
PTAL
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Remove codec thread from MediaCodecVideoEncoder. BUG=webrtc:6290 ========== to ========== Remove codec thread from MediaCodecVideoEncoder. After this change, all calls to MediaCodecVideoEncoder must be made on the same thread. Removes OnCodecThread extensions from methods since it is no longer meaningful. BUG=webrtc:6290 ==========
https://codereview.webrtc.org/2669093004/diff/60001/webrtc/sdk/android/src/jn... File webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc (left): https://codereview.webrtc.org/2669093004/diff/60001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc:423: RTC_FROM_HERE, Bind(&MediaCodecVideoEncoder::EncodeOnCodecThread, this, Can you try to reorder the functions in order to minimize the diff? This function should be moved to where EncodeOnCodecThread was. https://codereview.webrtc.org/2669093004/diff/60001/webrtc/sdk/android/src/jn... File webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2669093004/diff/60001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc:129: rtc::WeakPtr<MediaCodecVideoEncoder> encoder_; Why do we need a WeakPtr? https://codereview.webrtc.org/2669093004/diff/60001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc:190: rtc::ThreadChecker encoder_queue_checker_; Use SequencedTaskChecker instead. Also, update CL description to say that MediaCodecVideoEncoder must be used on a task queue, not "same thread".
Description was changed from ========== Remove codec thread from MediaCodecVideoEncoder. After this change, all calls to MediaCodecVideoEncoder must be made on the same thread. Removes OnCodecThread extensions from methods since it is no longer meaningful. BUG=webrtc:6290 ========== to ========== Remove codec thread from MediaCodecVideoEncoder. After this change, all calls to MediaCodecVideoEncoder must be made on the same task queueu. Removes OnCodecThread suffix from methods since it is no longer meaningful. BUG=webrtc:6290 ==========
PTAL https://codereview.webrtc.org/2669093004/diff/60001/webrtc/sdk/android/src/jn... File webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc (left): https://codereview.webrtc.org/2669093004/diff/60001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc:423: RTC_FROM_HERE, Bind(&MediaCodecVideoEncoder::EncodeOnCodecThread, this, On 2017/02/03 10:39:17, magjed_webrtc wrote: > Can you try to reorder the functions in order to minimize the diff? This > function should be moved to where EncodeOnCodecThread was. Done. Though, I often like to keep the order of declarations in sync with the order of definitions. https://codereview.webrtc.org/2669093004/diff/60001/webrtc/sdk/android/src/jn... File webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2669093004/diff/60001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc:129: rtc::WeakPtr<MediaCodecVideoEncoder> encoder_; On 2017/02/03 10:39:17, magjed_webrtc wrote: > Why do we need a WeakPtr? Discussed offline. (MediaCodecVideoEncoder can get deleted while the task is posted on the queue.) https://codereview.webrtc.org/2669093004/diff/60001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc:190: rtc::ThreadChecker encoder_queue_checker_; On 2017/02/03 10:39:17, magjed_webrtc wrote: > Use SequencedTaskChecker instead. Also, update CL description to say that > MediaCodecVideoEncoder must be used on a task queue, not "same thread". Done.
Description was changed from ========== Remove codec thread from MediaCodecVideoEncoder. After this change, all calls to MediaCodecVideoEncoder must be made on the same task queueu. Removes OnCodecThread suffix from methods since it is no longer meaningful. BUG=webrtc:6290 ========== to ========== Remove codec thread from MediaCodecVideoEncoder. After this change, all calls to MediaCodecVideoEncoder must be made on the same task queue. Removes OnCodecThread suffix from methods since it is no longer meaningful. BUG=webrtc:6290 ==========
lgtm
The CQ bit was checked by sakal@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_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
The CQ bit was checked by sakal@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...)
The CQ bit was checked by sakal@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/19560)
The CQ bit was checked by sakal@webrtc.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
sakal@webrtc.org changed reviewers: + stefan@webrtc.org
I have solved the problem of the encoder being created and destroyed on a different thread that it is operated. PTAL
lgtm % nits https://codereview.webrtc.org/2669093004/diff/140001/webrtc/sdk/android/src/j... File webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2669093004/diff/140001/webrtc/sdk/android/src/j... webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc:183: #if RTC_DCHECK_IS_ON It's apparent from '#if RTC_DCHECK_IS_ON', but please add a comment describing that this is only used to check correct thread usage on debug builds, not to affect the behaviour. https://codereview.webrtc.org/2669093004/diff/140001/webrtc/sdk/android/src/j... webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc:299: { The extra scope here is unnecessary.
https://codereview.webrtc.org/2669093004/diff/140001/webrtc/sdk/android/src/j... File webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2669093004/diff/140001/webrtc/sdk/android/src/j... webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc:183: #if RTC_DCHECK_IS_ON On 2017/02/13 16:10:18, magjed_webrtc wrote: > It's apparent from '#if RTC_DCHECK_IS_ON', but please add a comment describing > that this is only used to check correct thread usage on debug builds, not to > affect the behaviour. Done. https://codereview.webrtc.org/2669093004/diff/140001/webrtc/sdk/android/src/j... webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc:299: { On 2017/02/13 16:10:18, magjed_webrtc wrote: > The extra scope here is unnecessary. I just added it in case someone decides to add more code to the destructor later. I guess they can add it back if this ever happens.
ping
androidmediaencoder_jni.cc, so I can't lg that, but if magjed is an owner that should be enough. The description can be interpreted as if a thread is removed, but aren't we in fact still having a thread in the encoder as part of the task queue? https://codereview.webrtc.org/2669093004/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc (right): https://codereview.webrtc.org/2669093004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:152: encoder->Release(); This was a bug, right? We should add a unittest to ensure it doesn't regress. https://codereview.webrtc.org/2669093004/diff/160001/webrtc/sdk/android/src/j... File webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2669093004/diff/160001/webrtc/sdk/android/src/j... webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc:300: #if RTC_DCHECK_IS_ON I haven't seen this in many other places. Why is it needed here?
First sentence should've been: "I'm not the right person to review androidmediaencoder_jni.cc, so I can't lg that, but if magjed is an owner that should be enough." :)
A thread is removed in this CL. We reuse a task queue from the caller instead. https://codereview.webrtc.org/2669093004/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc (right): https://codereview.webrtc.org/2669093004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:152: encoder->Release(); On 2017/02/21 12:29:56, stefan-webrtc wrote: > This was a bug, right? We should add a unittest to ensure it doesn't regress. In practice not since all the encoders had an undocumented feature of releasing themselves in the destructor. This problem is caught by various instrumentation tests after this change. Specifically, it hits the DCHECK in the destructor of MediaCodecVideoEncoder. I believe it would be possible to make a more focused unit test if you find that important. https://codereview.webrtc.org/2669093004/diff/160001/webrtc/sdk/android/src/j... File webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2669093004/diff/160001/webrtc/sdk/android/src/j... webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc:300: #if RTC_DCHECK_IS_ON On 2017/02/21 12:29:56, stefan-webrtc wrote: > I haven't seen this in many other places. Why is it needed here? A lock is not normally needed. However, it is needed for this debug check. I didn't want to add it for release builds just to make a debug check.
lgtm https://codereview.webrtc.org/2669093004/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc (right): https://codereview.webrtc.org/2669093004/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/codecs/vp8/simulcast_encoder_adapter.cc:152: encoder->Release(); On 2017/02/21 12:53:50, sakal wrote: > On 2017/02/21 12:29:56, stefan-webrtc wrote: > > This was a bug, right? We should add a unittest to ensure it doesn't regress. > > In practice not since all the encoders had an undocumented feature of releasing > themselves in the destructor. > > This problem is caught by various instrumentation tests after this change. > Specifically, it hits the DCHECK in the destructor of MediaCodecVideoEncoder. I > believe it would be possible to make a more focused unit test if you find that > important. If it is hit by tests then I think it's good enough. https://codereview.webrtc.org/2669093004/diff/160001/webrtc/sdk/android/src/j... File webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc (right): https://codereview.webrtc.org/2669093004/diff/160001/webrtc/sdk/android/src/j... webrtc/sdk/android/src/jni/androidmediaencoder_jni.cc:300: #if RTC_DCHECK_IS_ON On 2017/02/21 12:53:50, sakal wrote: > On 2017/02/21 12:29:56, stefan-webrtc wrote: > > I haven't seen this in many other places. Why is it needed here? > > A lock is not normally needed. However, it is needed for this debug check. I > didn't want to add it for release builds just to make a debug check. ack
The CQ bit was checked by sakal@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org Link to the patchset: https://codereview.webrtc.org/2669093004/#ps160001 (title: "Update comment, remove unncessary scope.")
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": 160001, "attempt_start_ts": 1487842286332890,
"parent_rev": "df92c5cb8c34c7c43e9f0ec66914a86deeeda626", "commit_rev":
"02f994b4e9ffdf6fddb0e52818785a1f28c4db26"}
Message was sent while issue was closed.
Description was changed from ========== Remove codec thread from MediaCodecVideoEncoder. After this change, all calls to MediaCodecVideoEncoder must be made on the same task queue. Removes OnCodecThread suffix from methods since it is no longer meaningful. BUG=webrtc:6290 ========== to ========== Remove codec thread from MediaCodecVideoEncoder. After this change, all calls to MediaCodecVideoEncoder must be made on the same task queue. Removes OnCodecThread suffix from methods since it is no longer meaningful. BUG=webrtc:6290 Review-Url: https://codereview.webrtc.org/2669093004 Cr-Commit-Position: refs/heads/master@{#16792} Committed: https://chromium.googlesource.com/external/webrtc/+/02f994b4e9ffdf6fddb0e5281... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001) as https://chromium.googlesource.com/external/webrtc/+/02f994b4e9ffdf6fddb0e5281... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
