Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1782043003:
MediaRecorder: produce keyframes more often (Closed)
|
Created:
4 years, 9 months ago by thembrown Modified:
4 years, 9 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionForce keyframes every 100 frames.
Old behavior forced keyframes every 30000 frames. This makes it prohibitively
expensive to seek in a resulting video (without re-encoding), as this might
require decoding up to 30000 frames. Forcing more keyframes makes seeking much
more efficient.
BUG=594241
Committed: https://crrev.com/10fd187c1de8dff86eb165deaecec7a5b5f6cc36
Cr-Commit-Position: refs/heads/master@{#381090}
Patch Set 1 #Patch Set 2 : #
Total comments: 3
Messages
Total messages: 18 (7 generated)
Description was changed from ========== Force keyframes every 150 frames Old behavior forced keyframes every 30000 frames. This makes it prohibitively expensive to seek in a resulting video (without re-encoding), as this might require decoding up to 30000 frames. Forcing more keyframes makes seeking much more efficient. BUG=262211 ========== to ========== Force keyframes every 150 frames Old behavior forced keyframes every 30000 frames. This makes it prohibitively expensive to seek in a resulting video (without re-encoding), as this might require decoding up to 30000 frames. Forcing more keyframes makes seeking much more efficient. BUG=262211 ==========
thembrown@gmail.com changed reviewers: + mcasas@chromium.org, miu@chromium.org, sandersd@chromium.org
Description was changed from ========== Force keyframes every 150 frames Old behavior forced keyframes every 30000 frames. This makes it prohibitively expensive to seek in a resulting video (without re-encoding), as this might require decoding up to 30000 frames. Forcing more keyframes makes seeking much more efficient. BUG=262211 ========== to ========== Force keyframes every 100 frames. Old behavior forced keyframes every 30000 frames. This makes it prohibitively expensive to seek in a resulting video (without re-encoding), as this might require decoding up to 30000 frames. Forcing more keyframes makes seeking much more efficient. BUG=262211 ==========
Side note: To see performance difference, record something on https://www.cam-recorder.com/ and try to seek in resulting file with and without this patch. It writes MediaRecorder output, but adds missing Cues/Duration elements to the WebM file to allow seeking.
Could you please a) file a bug to commit against (262211 is closed) b) give a bit more context in the CL one liner, e.g. "MediaRecorder: produce keyframes more often."
I'd like to get miu@s take on this, but this in principle el-gee-tee-em. I wonder if you got some performance metrics/differences? You could very well try [1] with a test of your own devising, or at least get some idea as to the CPU impact, WDYT? [1] https://www.chromium.org/developers/telemetry/run_locally
https://codereview.chromium.org/1782043003/diff/20001/content/renderer/media/... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1782043003/diff/20001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:333: codec_config_.kf_max_dist = 100; It seems that your goal is to generate a key frame once every 3 seconds of video. Adjusting this here will only work if the actual frame rate is near 30 FPS. Instead of making this change here, I'd suggest adding some simple logic to EncodeOnEncodingThread() to force a KF after 3 seconds have elapsed on the video timeline. Something like: // Force a key frame for every three seconds of video. This is to allow video players to efficiently seek around in the video stream. bool force_key_frame = false; if (last_key_frame_at_timestamp_.is_null() || ((video_frame->timestamp() - last_key_frame_at_timestamp_) > base::TimeDelta::FromSeconds(3)) { force_key_frame = true; } ... const vpx_codec_err_t ret = vpx_codec_encode( encoder_.get(), &vpx_image, 0 /* pts */, duration.InMicroseconds(), force_key_frame ? VPX_EFLAG_FORCE_KF : kNoFlags, VPX_DL_REALTIME); ... while ((pkt = vpx_codec_get_cx_data(...)) { ... } if (keyframe) last_key_frame_at_timestamp_ = video_frame->timestamp();
On 2016/03/11 19:46:07, mcasas wrote: > Could you please > a) file a bug to commit against (262211 is > closed) > b) give a bit more context in the CL one > liner, e.g. "MediaRecorder: produce keyframes > more often." ACK, I'll file a separate bug.
https://codereview.chromium.org/1782043003/diff/20001/content/renderer/media/... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1782043003/diff/20001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:333: codec_config_.kf_max_dist = 100; On 2016/03/11 20:04:36, miu wrote: > It seems that your goal is to generate a key frame once every 3 seconds of > video. Adjusting this here will only work if the actual frame rate is near 30 > FPS. My goal was mainly to introduce a upper bound for the number of frames that need to be decoded when seeking. So it'd have to decode at most 100 frames (~ 3sec @ 30fps, or 20 sec @ 5fps). For that purpose I don't see the advantage of producing keyframes in a fixed time-interval instead, as the presentation timestamps should not have any impact on decoding performance. A fixed time-interval between keyframes would work too, but might produce more keyframes than necessary at low frame rates. What do you think? (Firefox produces keyframes at fixed time-intervals IIRC.) > > Instead of making this change here, I'd suggest adding some simple logic to > EncodeOnEncodingThread() to force a KF after 3 seconds have elapsed on the video > timeline. Something like: > > // Force a key frame for every three seconds of video. This is to allow video > players to efficiently seek around in the video stream. > bool force_key_frame = false; > if (last_key_frame_at_timestamp_.is_null() || > ((video_frame->timestamp() - last_key_frame_at_timestamp_) > > base::TimeDelta::FromSeconds(3)) { > force_key_frame = true; > } > > ... > > const vpx_codec_err_t ret = vpx_codec_encode( > encoder_.get(), &vpx_image, 0 /* pts */, duration.InMicroseconds(), > force_key_frame ? VPX_EFLAG_FORCE_KF : kNoFlags, VPX_DL_REALTIME); > > ... > > while ((pkt = vpx_codec_get_cx_data(...)) { > ... > } > > if (keyframe) > last_key_frame_at_timestamp_ = video_frame->timestamp();
On 2016/03/11 19:48:21, mcasas wrote: > I'd like to get miu@s take on this, but this in > principle el-gee-tee-em. > > I wonder if you got some performance > metrics/differences? You could very well > try [1] with a test of your own devising, or > at least get some idea as to the CPU impact, > WDYT? > > [1] https://www.chromium.org/developers/telemetry/run_locally I did not run a real benchmark yet. The difference was quite obvious when compared manually though, especially with longer videos (~10 minutes). With regular keyframes seeking feels instantaneous, without it takes tens of seconds on my machine. Which I think makes sense, as without any keyframes, seeking forward 5 minutes @ 30 fps requires decoding 5 * 60 * 30 = 7500 frames. With a keyframe every 100 frames, it just requires decoding 50 frames on average.
Description was changed from ========== Force keyframes every 100 frames. Old behavior forced keyframes every 30000 frames. This makes it prohibitively expensive to seek in a resulting video (without re-encoding), as this might require decoding up to 30000 frames. Forcing more keyframes makes seeking much more efficient. BUG=262211 ========== to ========== Force keyframes every 100 frames. Old behavior forced keyframes every 30000 frames. This makes it prohibitively expensive to seek in a resulting video (without re-encoding), as this might require decoding up to 30000 frames. Forcing more keyframes makes seeking much more efficient. BUG=594241 ==========
lgtm https://codereview.chromium.org/1782043003/diff/20001/content/renderer/media/... File content/renderer/media/video_track_recorder.cc (right): https://codereview.chromium.org/1782043003/diff/20001/content/renderer/media/... content/renderer/media/video_track_recorder.cc:333: codec_config_.kf_max_dist = 100; On 2016/03/11 20:32:51, thembrown wrote: > My goal was mainly to introduce a upper bound for the number of frames that need > to be decoded when seeking. So it'd have to decode at most 100 frames (~ 3sec @ > 30fps, or 20 sec @ 5fps). For that purpose I don't see the advantage of > producing keyframes in a fixed time-interval instead, as the presentation > timestamps should not have any impact on decoding performance. A fixed SGTM.
The CQ bit was checked by thembrown@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1782043003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1782043003/20001
Message was sent while issue was closed.
Description was changed from ========== Force keyframes every 100 frames. Old behavior forced keyframes every 30000 frames. This makes it prohibitively expensive to seek in a resulting video (without re-encoding), as this might require decoding up to 30000 frames. Forcing more keyframes makes seeking much more efficient. BUG=594241 ========== to ========== Force keyframes every 100 frames. Old behavior forced keyframes every 30000 frames. This makes it prohibitively expensive to seek in a resulting video (without re-encoding), as this might require decoding up to 30000 frames. Forcing more keyframes makes seeking much more efficient. BUG=594241 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Force keyframes every 100 frames. Old behavior forced keyframes every 30000 frames. This makes it prohibitively expensive to seek in a resulting video (without re-encoding), as this might require decoding up to 30000 frames. Forcing more keyframes makes seeking much more efficient. BUG=594241 ========== to ========== Force keyframes every 100 frames. Old behavior forced keyframes every 30000 frames. This makes it prohibitively expensive to seek in a resulting video (without re-encoding), as this might require decoding up to 30000 frames. Forcing more keyframes makes seeking much more efficient. BUG=594241 Committed: https://crrev.com/10fd187c1de8dff86eb165deaecec7a5b5f6cc36 Cr-Commit-Position: refs/heads/master@{#381090} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/10fd187c1de8dff86eb165deaecec7a5b5f6cc36 Cr-Commit-Position: refs/heads/master@{#381090} |
