Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2467623002:
media: Only add more threads for codecs that benefit from it (Closed)
|
Created:
4 years, 1 month ago by hubbe Modified:
4 years, 1 month ago Reviewers:
DaleCurtis CC:
chromium-reviews, feature-media-reviews_chromium.org, posciak+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Only add more threads for codecs that benefit from it
Adding more threads tends to increase seek time. For high resolution
videos this is probably a reasonable tradeoff as we get smoother
playback. However, some codecs don't use threads for decoding at all,
and for those codecs adding more threads does nothing except increase
decoding latency and seek time.
BUG=660900
Committed: https://crrev.com/d4cc08f819e66335d32a72bd500781d2d36be99f
Cr-Commit-Position: refs/heads/master@{#428940}
Patch Set 1 #
Total comments: 2
Patch Set 2 : use switch statement, add vp8/9 #
Total comments: 2
Patch Set 3 : comments addressed #
Total comments: 2
Patch Set 4 : NOTREACHED -> comment #Messages
Total messages: 28 (14 generated)
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
hubbe@chromium.org changed reviewers: + dalecurtis@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2467623002/diff/1/media/filters/ffmpeg_video_... File media/filters/ffmpeg_video_decoder.cc (right): https://codereview.chromium.org/2467623002/diff/1/media/filters/ffmpeg_video_... media/filters/ffmpeg_video_decoder.cc:53: // Some ffmpeg codecs don't actually benefit from using more threads. vp8 should benefit? I'd just exclude THEORA?
The CQ bit was checked by hubbe@chromium.org to run a CQ dry run
https://codereview.chromium.org/2467623002/diff/1/media/filters/ffmpeg_video_... File media/filters/ffmpeg_video_decoder.cc (right): https://codereview.chromium.org/2467623002/diff/1/media/filters/ffmpeg_video_... media/filters/ffmpeg_video_decoder.cc:53: // Some ffmpeg codecs don't actually benefit from using more threads. On 2016/10/31 21:26:33, DaleCurtis wrote: > vp8 should benefit? I'd just exclude THEORA? Replaced the if statement with a switch to remind people in the future to add new codecs here. Added VP8/9 to the list that benefit from more threads. From looking at the ffmpeg code, it seems like VP1 doesn't support threads, so I left it off the list.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2467623002/diff/20001/media/filters/ffmpeg_vi... File media/filters/ffmpeg_video_decoder.cc (right): https://codereview.chromium.org/2467623002/diff/20001/media/filters/ffmpeg_vi... media/filters/ffmpeg_video_decoder.cc:67: case kCodecHEVC: Unknown, VC1, MPEG2, and HEVC should all be NOTREACHED(). Probably VP9 should be NOTREACHED as well to avoid being misleading.
https://codereview.chromium.org/2467623002/diff/20001/media/filters/ffmpeg_vi... File media/filters/ffmpeg_video_decoder.cc (right): https://codereview.chromium.org/2467623002/diff/20001/media/filters/ffmpeg_vi... media/filters/ffmpeg_video_decoder.cc:67: case kCodecHEVC: On 2016/10/31 22:07:57, DaleCurtis wrote: > Unknown, VC1, MPEG2, and HEVC should all be NOTREACHED(). Probably VP9 should be > NOTREACHED as well to avoid being misleading. Done.
The CQ bit was checked by hubbe@chromium.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.chromium.or...
lgtm https://codereview.chromium.org/2467623002/diff/40001/media/filters/ffmpeg_vi... File media/filters/ffmpeg_video_decoder.cc (right): https://codereview.chromium.org/2467623002/diff/40001/media/filters/ffmpeg_vi... media/filters/ffmpeg_video_decoder.cc:61: NOTREACHED(); Actually, sorry we should drop the NOTREACHED(). In the event of an error condition GpuVideoDecoder/VpxVideoDecoder will fall through to FFmpegVideoDecoder with some of these codecs, this shouldn't DCHECK() in that case. I would still keep this block separate from kCodecTheora though with a comment that these codecs are not supported by ffmpeg.
The CQ bit was checked by hubbe@chromium.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.chromium.or...
https://codereview.chromium.org/2467623002/diff/40001/media/filters/ffmpeg_vi... File media/filters/ffmpeg_video_decoder.cc (right): https://codereview.chromium.org/2467623002/diff/40001/media/filters/ffmpeg_vi... media/filters/ffmpeg_video_decoder.cc:61: NOTREACHED(); On 2016/10/31 22:26:49, DaleCurtis wrote: > Actually, sorry we should drop the NOTREACHED(). In the event of an error > condition GpuVideoDecoder/VpxVideoDecoder will fall through to > FFmpegVideoDecoder with some of these codecs, this shouldn't DCHECK() in that > case. I would still keep this block separate from kCodecTheora though with a > comment that these codecs are not supported by ffmpeg. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hubbe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2467623002/#ps60001 (title: "NOTREACHED -> comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== media: Only add more threads for codecs that benefit from it Adding more threads tends to increase seek time. For high resolution videos this is probably a reasonable tradeoff as we get smoother playback. However, some codecs don't use threads for decoding at all, and for those codecs adding more threads does nothing except increase decoding latency and seek time. BUG=660900 ========== to ========== media: Only add more threads for codecs that benefit from it Adding more threads tends to increase seek time. For high resolution videos this is probably a reasonable tradeoff as we get smoother playback. However, some codecs don't use threads for decoding at all, and for those codecs adding more threads does nothing except increase decoding latency and seek time. BUG=660900 Committed: https://crrev.com/d4cc08f819e66335d32a72bd500781d2d36be99f Cr-Commit-Position: refs/heads/master@{#428940} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d4cc08f819e66335d32a72bd500781d2d36be99f Cr-Commit-Position: refs/heads/master@{#428940}
Message was sent while issue was closed.
Hmm, how did you determine that theora doesn't benefit from extra threads? Looks like we have conflicting graphs: https://chromeperf.appspot.com/report?sid=e3626002d3460da01f087e058fc1bad48cc...
Message was sent while issue was closed.
On 2016/11/03 00:40:53, DaleCurtis wrote: > Hmm, how did you determine that theora doesn't benefit from extra threads? Looks > like we have conflicting graphs: > > https://chromeperf.appspot.com/report?sid=e3626002d3460da01f087e058fc1bad48cc... I tried playing the ogv on my machine with and without the extra threads and noticed little or no difference.
Message was sent while issue was closed.
You mean the telemetry test showed no difference locally?
Message was sent while issue was closed.
On 2016/11/03 18:13:55, DaleCurtis wrote: > You mean the telemetry test showed no difference locally? No, I mean that I tried to play the OGV file in a browser. The OGG/theora file is high resolution and does not play smoothly on my z620 with or without the extra threads. I then looked at the code and found no mentioning of threading in the theora part of ffmpeg, while the other codecs we care about do. (VC1 was the only other codec that didn't have threads mentioned somewhere in the code.) |
