Closed
Bug 1497650
Opened 6 years ago
Closed 6 years ago
Upstream our local changes to vp8_impl.cc
Categories
(Core :: WebRTC: Audio/Video, enhancement, P3)
Tracking
()
RESOLVED
FIXED
People
(Reporter: dminor, Assigned: dminor)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
No description provided.
|
Assignee | |
Comment 1•6 years ago
|
||
Upstream has refactored vp8_impl.cc substantially, so here is the diff between what we have in tree and upstream branch 64.
|
|
Assignee | |
Comment 2•6 years ago
|
||
Bug 919979 added:
437a439
> // Reserve 100 extra bytes for overhead at small resolutions.
439c441
< CalcBufferSize(VideoType::kI420, codec_.width, codec_.height);
---
> CalcBufferSize(VideoType::kI420, codec_.width, codec_.height) + 100;
543c545,551
< stream_bitrates[stream_idx], inst->maxBitrate, inst->maxFramerate);
It looks like the unit test that was added with Bug 919979 is no longer present. I'll add a similar test and see if this change is still required. I doubt upstream would take it in its current form.
|
|
Assignee | |
Updated•6 years ago
|
Assignee: nobody → dminor
Status: NEW → ASSIGNED
|
|
Assignee | |
Comment 3•6 years ago
|
||
Bug 1453740 added:
369c370
< if (inst->width <= 1 || inst->height <= 1) {
---
> if (inst->width < 1 || inst->height < 1) {
The 1x1 images come from here [1]. It's probably worth retesting this now that the 64 update has landed to see if this still fails before we try to upstream this. We could either upstream this change or attempt to change [1] to return 2x2 black images.
[1] https://searchfox.org/mozilla-central/rev/3fdc51e66c9487b39220ad58dcee275fca070ccd/media/webrtc/trunk/webrtc/modules/desktop_capture/window_capturer_win.cc#238
|
|
Assignee | |
Comment 4•6 years ago
|
||
I filed https://bugs.chromium.org/p/webrtc/issues/detail?id=10099 upstream for the changes from Bug 1453740.
The remaining changes in the diff are from Bug 1429219, for which I had already filed https://bugs.chromium.org/p/webrtc/issues/detail?id=8753 upstream.
|
|
Assignee | |
Comment 5•6 years ago
|
||
Turns out I filed Bug 1498322 to handle the temporal layer initialization changes in vp8_impl.cc. I'll work on that over there.
|
||
Comment 6•6 years ago
|
||
> The 1x1 images come from here [1]. It's probably worth retesting this now
> that the 64 update has landed to see if this still fails before we try to
> upstream this. We could either upstream this change or attempt to change [1]
> to return 2x2 black images.
I think it's best to ensure that a 1x1 image (or 1xN or Nx1) don't fail, so I wouldn't switch to 2x2 when minimized. I'd think this would be non-controversial
|
|
Assignee | |
Comment 7•6 years ago
|
||
In Bug 919979 we added 100 bytes to the size returned by CalcBufferSize
to work around an error with the calculated buffer size with small
resolutions. I verified that this extra buffer is no longer required with
a modified mochitest. Given the age of the bug this was working around,
I don't think a permanent test is required to prevent regressions from
upstream.
|
|
Assignee | |
Updated•6 years ago
|
Keywords: leave-open
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b20329b1ec05
Remove 100 bytes added to CalcBufferSize in vp8_impl.cc; r=ng
|
||
Comment 9•6 years ago
|
||
| bugherder | ||
|
|
Assignee | |
Comment 10•6 years ago
|
||
(In reply to Dan Minor [:dminor] from comment #3)
Bug 1453740 added:
369c370
< if (inst->width <= 1 || inst->height <= 1) {if (inst->width < 1 || inst->height < 1) {
The 1x1 images come from here [1]. It's probably worth retesting this now
that the 64 update has landed to see if this still fails before we try to
upstream this. We could either upstream this change or attempt to change [1]
to return 2x2 black images.[1]
https://searchfox.org/mozilla-central/rev/
3fdc51e66c9487b39220ad58dcee275fca070ccd/media/webrtc/trunk/webrtc/modules/
desktop_capture/window_capturer_win.cc#238
This landed in https://webrtc-review.googlesource.com/c/src/+/120422.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
|
||
Updated•6 years ago
|
Keywords: leave-open
You need to log in
before you can comment on or make changes to this bug.
Description
•