New issue
Advanced search Search tips

Issue 701450 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug



Sign in to add a comment

Resolution change causes crash in MediaRecorder using VEA

Project Member Reported by emir...@chromium.org, Mar 14 2017

Issue description

Project Member

Comment 1 by bugdroid1@chromium.org, Mar 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/98557de3bc84264df66670ca5baa7e0765f2186f

commit 98557de3bc84264df66670ca5baa7e0765f2186f
Author: emircan <emircan@chromium.org>
Date: Tue Mar 14 21:53:49 2017

Avoid calling VEA::Destroy() in VideoTrackRecorder

The default deleter for VEA calls Destroy() by default[0]. Running it multiple times can
cause a crash, see bug.
[0] https://cs.chromium.org/chromium/src/media/video/video_encode_accelerator.h?rcl=575512843787644524196d04076a59ec9415cb89&l=186

BUG= 701450 
TEST=Fixes the crash described in the bug.

Review-Url: https://codereview.chromium.org/2753593002
Cr-Commit-Position: refs/heads/master@{#456851}

[modify] https://crrev.com/98557de3bc84264df66670ca5baa7e0765f2186f/content/renderer/media_recorder/video_track_recorder.cc

Labels: Merge-Request-58
Project Member

Comment 3 by sheriffbot@chromium.org, Mar 15 2017

Labels: -Merge-Request-58 Hotlist-Merge-Approved Merge-Approved-58
Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 4 by bugdroid1@chromium.org, Mar 15 2017

Labels: -merge-approved-58 merge-merged-3029
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/813c97a414763063352914dd35fe9ea426852acf

commit 813c97a414763063352914dd35fe9ea426852acf
Author: emircan <emircan@chromium.org>
Date: Wed Mar 15 22:08:25 2017

Merge 58: Avoid calling VEA::Destroy() in VideoTrackRecorder

The default deleter for VEA calls Destroy() by default[0]. Running it multiple times can
cause a crash, see bug.
[0] https://cs.chromium.org/chromium/src/media/video/video_encode_accelerator.h?rcl=575512843787644524196d04076a59ec9415cb89&l=186

BUG= 701450 
TEST=Fixes the crash described in the bug.

Review-Url: https://codereview.chromium.org/2753593002
Cr-Commit-Position: refs/heads/master@{#456851}
(cherry picked from commit 98557de3bc84264df66670ca5baa7e0765f2186f)
NOTRY=true
NOPRESUBMIT=true
TBR=mcasas@chromium.org

Review-Url: https://codereview.chromium.org/2753513008
Cr-Commit-Position: refs/branch-heads/3029@{#219}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/813c97a414763063352914dd35fe9ea426852acf/content/renderer/media_recorder/video_track_recorder.cc

Labels: Merge-Request-57
Status: Fixed (was: Started)
Adding Merge-request-57 as well. Only 6 crashes were seen on 57, but we know that this can be reliably reproduced by resizing tab capture while MediaRecorder is running on Mac. The fix has a very specific purpose and touches only MediaRecorder
Project Member

Comment 6 by sheriffbot@chromium.org, Mar 16 2017

Labels: -Merge-Request-57 Hotlist-Merge-Review Merge-Review-57
This bug requires manual review: Request affecting a post-stable build
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 7 by gov...@chromium.org, Mar 16 2017

M57 for stable is already out and at the moment there is no plan for respin. Even if we decide to respin, this merge doesn't justify for stable channel merge (per comment #5 only 6 crashes reported on M57 so far). I'm going to reject this merge for M57. Please let me know if there is any concern here. 

https://crash.corp.google.com/browse?q=custom_data.ChromeCrashProto.magic_signature_1.name%3D%27content%3A%3A%60anonymous%20namespace%5C%27%3A%3AVEAEncoder%3A%3AEncodeOnEncodingTaskRunner%27&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D#samplereports:5,productversion

57.0.2987.98: 1
57.0.2987.21: 1
57.0.2986.0: 4
Project Member

Comment 8 by bugdroid1@chromium.org, Mar 16 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6d0510b49f3fa532b29378bf9d45b08374ce1885

commit 6d0510b49f3fa532b29378bf9d45b08374ce1885
Author: emircan <emircan@chromium.org>
Date: Thu Mar 16 23:44:51 2017

Add resized video input test for WebRtcMediaRecorderBrowserTest

This CL adds a new test where canvas input resizes during the recording.

BUG= 701450 

Review-Url: https://codereview.chromium.org/2750373003
Cr-Commit-Position: refs/heads/master@{#457607}

[modify] https://crrev.com/6d0510b49f3fa532b29378bf9d45b08374ce1885/content/browser/webrtc/webrtc_media_recorder_browsertest.cc
[modify] https://crrev.com/6d0510b49f3fa532b29378bf9d45b08374ce1885/content/test/data/media/mediarecorder_test.html

Comment 9 by gov...@chromium.org, Mar 20 2017

Cc: pbomm...@chromium.org
Labels: -Merge-Review-57 Merge-Rejected-57
Rejecting merge to M57 based on comment #7.

Sign in to add a comment