This bug requires manual review: M61 has already been promoted to the beta branch, so this requires manual review
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

govind@, PTAL at my merge request.

Before we approve merge to M61, please answer followings:
* Is this M61 regression? Is it critical to take in for M61?
* Is the change well baked/verified in Canary, having enough automation tests coverage and safe to merge to M61?
* Any other important details to justify the merge.

Please note M61 is already in Beta, so merge bar is very high. Thank you.

The regression happened in M58, and it would be really nice to have, but not critical. Unfortunately it was not rolled into chrome before the weekend  so it has not yet been built/verified in canary.

Thank you  philipel@.
Rejecting merge to M61 as this bug has been exists since M58 and not critical. 

With all respect, how is buggy WebRTC-internals producing corrupted video from a perfectly good stream not critical?

Deleted: corruption.png 1.2 MB

	corruption.png 1.2 MB View Download

I agree that this is bad enough to be considered critical from a quality perspective. I think what philipel referred to is that it isn't critical from a security perspective. Given that it's particularly likely to hit with screensharing applications, I think it's important to get it fixed.

Can we please reconsider this for M61?

Philip, you mentioned that there's a much simpler workaround which we could merge to M61 if we want to make the smallest change possible. Not sure if that's worth considering?

This bug requires manual review: M61 has already been promoted to the beta branch, so this requires manual review
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

Yes, there is a trivial and effective way to mitigate this problem. Since the bug only occurs when a frame is the same size as the packet buffer we can increase the startsize to something large, like 512 and the bug should never trigger in practice.

This workaround would only require us to change the 32 to 512 on this line:
https://cs.chromium.org/chromium/src/third_party/webrtc/video/rtp_video_stream_receiver.cc?l=46

If simple workaround listed #9 works and safe, then we will take the merge in for M61.

It works and it is safe, I uploaded the CL here: https://codereview.chromium.org/3000653002/

Pls wait until change listed at #11 is baked/verified in Canary and update result here. If canary result looks good, I will approve merge to M61.

The CL in #11 is a merge to M61, but I'll land the equivalent CL in ToT.

CL for WebRTC ToT: https://codereview.chromium.org/2994093002/

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/3bf97cf0602f150e05b5424735cced520aa0cfef

commit 3bf97cf0602f150e05b5424735cced520aa0cfef
Author: philipel <philipel@webrtc.org>
Date: Thu Aug 10 16:11:04 2017

Workaround for PacketBuffer bug.

There exist a bug in the video_coding::PacketBuffer which triggers when a
frame is the same size as the buffer. A trivial workaround is to increase
the start size to something big so that this never happens in practice.

The bug has been fixed but we still want to test the workaround in ToT,
which is why this CL exist.

BUG= webrtc:8028 ,  chromium:752886 
R=stefan@webrtc.org

Review-Url: https://codereview.webrtc.org/2994093002 .
Cr-Commit-Position: refs/heads/master@{#19308}

[modify] https://crrev.com/3bf97cf0602f150e05b5424735cced520aa0cfef/webrtc/video/rtp_video_stream_receiver.cc

Thank you  philipel@ for landing cl to ToT. Please update the bug with Canary result. 

How is this change looking in Canary?

The change in #15 was first built with version 62.0.3183.0, and we have around 1250 calls made so far and it looks good.

Approving merge to M61 branch 3163 for CL listed at #15 based on comment #18. Please merge ASAP. Thank you.

The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/371062bf35921ebeb33b3a66cdf58d1028d8e9a9

commit 371062bf35921ebeb33b3a66cdf58d1028d8e9a9
Author: philipel <philipel@webrtc.org>
Date: Mon Aug 14 14:51:57 2017

Workaround for PacketBuffer bug.

This is a merge to M61.

There exist a bug in the video_coding::PacketBuffer which triggers when a
frame is the same size as the buffer. A trivial workaround is to increase
the start size to something big so that this never happens in practice.

The bug has been fixed but to avoid non-trivial merges we use this
workaround instead for M61.

BUG= webrtc:8028 ,  chromium:752886 
R=stefan@webrtc.org

Review-Url: https://codereview.webrtc.org/3000653002 .
Cr-Commit-Position: refs/branch-heads/61@{#4}
Cr-Branched-From: 83dc6b6f5380d7c03f5ebfe2553bf4a3147e2d1c-refs/heads/master@{#19063}

[modify] https://crrev.com/371062bf35921ebeb33b3a66cdf58d1028d8e9a9/webrtc/video/rtp_video_stream_receiver.cc

Per comment #20, this is already merged to M61.

Shall we close this now?

Any update on the TODO comment https://chromium.googlesource.com/external/webrtc/+/master/video/rtp_video_stream_receiver.cc#45 ?

// TODO(philipel): Change kPacketBufferStartSize back to 32 in M63 see:
//                  crbug.com/752886 

Thx!