Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(21)

Issue 1561703002: media/vpx: Add support for VP9 alpha channel (Closed)

Created:
4 years, 6 months ago by vignesh
Modified:
4 years, 5 months ago
CC:
chromium-reviews, feature-media-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

media/vpx: Add support for VP9 alpha channel Not supporting VP9 alpha channel in WebM is an artificial restriction. This patch removes that restriction and adds a test. TEST=<media pipeline integration test should pass> Committed: https://crrev.com/4863f9bda1800e1da901588ffd995364aaa70d8e Cr-Commit-Position: refs/heads/master@{#370147}

Patch Set 1 #

Total comments: 9

Patch Set 2 : marked tests as kClockless #

Patch Set 3 : avoid copying y,u and v. #

Total comments: 2

Patch Set 4 : move alpha decoding to its own function #

Total comments: 10

Patch Set 5 : addressing comments #

Total comments: 4

Patch Set 6 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+185 lines, -63 lines) Patch
M media/base/video_frame.h View 1 2 1 chunk +17 lines, -0 lines 0 comments Download
M media/base/video_frame.cc View 1 2 3 4 5 1 chunk +42 lines, -0 lines 0 comments Download
M media/filters/vpx_video_decoder.h View 1 2 3 4 2 chunks +14 lines, -0 lines 0 comments Download
M media/filters/vpx_video_decoder.cc View 1 2 3 4 5 7 chunks +87 lines, -54 lines 0 comments Download
A media/test/data/bear-vp9a.webm View Binary file 0 comments Download
A media/test/data/bear-vp9a-odd-dimensions.webm View Binary file 0 comments Download
M media/test/pipeline_integration_test.cc View 1 2 3 4 5 7 chunks +25 lines, -9 lines 0 comments Download

Messages

Total messages: 28 (4 generated)
vignesh
4 years, 6 months ago (2016-01-05 19:11:42 UTC) #2
DaleCurtis
https://codereview.chromium.org/1561703002/diff/1/media/filters/vpx_video_decoder.cc File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/1561703002/diff/1/media/filters/vpx_video_decoder.cc#newcode403 media/filters/vpx_video_decoder.cc:403: // Configure non-YV12A VP9 to decode on our buffers ...
4 years, 6 months ago (2016-01-05 21:22:21 UTC) #3
vignesh
https://codereview.chromium.org/1561703002/diff/1/media/filters/vpx_video_decoder.cc File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/1561703002/diff/1/media/filters/vpx_video_decoder.cc#newcode403 media/filters/vpx_video_decoder.cc:403: // Configure non-YV12A VP9 to decode on our buffers ...
4 years, 6 months ago (2016-01-06 00:07:49 UTC) #4
DaleCurtis
lgtm https://codereview.chromium.org/1561703002/diff/1/media/test/pipeline_integration_test.cc File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/1561703002/diff/1/media/test/pipeline_integration_test.cc#newcode1838 media/test/pipeline_integration_test.cc:1838: ASSERT_EQ(PIPELINE_OK, Start("bear-vp9a.webm")); On 2016/01/06 00:07:49, vignesh wrote: > ...
4 years, 6 months ago (2016-01-06 00:16:49 UTC) #5
DaleCurtis
https://codereview.chromium.org/1561703002/diff/1/media/filters/vpx_video_decoder.cc File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/1561703002/diff/1/media/filters/vpx_video_decoder.cc#newcode403 media/filters/vpx_video_decoder.cc:403: // Configure non-YV12A VP9 to decode on our buffers ...
4 years, 6 months ago (2016-01-06 00:18:02 UTC) #6
vignesh
https://codereview.chromium.org/1561703002/diff/1/media/filters/vpx_video_decoder.cc File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/1561703002/diff/1/media/filters/vpx_video_decoder.cc#newcode403 media/filters/vpx_video_decoder.cc:403: // Configure non-YV12A VP9 to decode on our buffers ...
4 years, 6 months ago (2016-01-08 17:53:40 UTC) #7
vignesh
https://codereview.chromium.org/1561703002/diff/1/media/filters/vpx_video_decoder.cc File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/1561703002/diff/1/media/filters/vpx_video_decoder.cc#newcode403 media/filters/vpx_video_decoder.cc:403: // Configure non-YV12A VP9 to decode on our buffers ...
4 years, 6 months ago (2016-01-08 19:21:33 UTC) #8
DaleCurtis
https://codereview.chromium.org/1561703002/diff/1/media/filters/vpx_video_decoder.cc File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/1561703002/diff/1/media/filters/vpx_video_decoder.cc#newcode403 media/filters/vpx_video_decoder.cc:403: // Configure non-YV12A VP9 to decode on our buffers ...
4 years, 6 months ago (2016-01-08 19:29:03 UTC) #9
vignesh
took a shot at avoiding copying of Y, U and V planes. The code ended ...
4 years, 6 months ago (2016-01-12 00:57:29 UTC) #10
DaleCurtis
Aside from some minor cleanup it doesn't look that bad to me. What part irks ...
4 years, 6 months ago (2016-01-12 01:18:20 UTC) #11
vignesh
On 2016/01/12 01:18:20, DaleCurtis wrote: > Aside from some minor cleanup it doesn't look that ...
4 years, 6 months ago (2016-01-12 01:27:13 UTC) #12
vignesh
I have changed the behavior in case where the file claims it has alpha but ...
4 years, 5 months ago (2016-01-13 01:26:04 UTC) #13
DaleCurtis
https://codereview.chromium.org/1561703002/diff/60001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/1561703002/diff/60001/media/base/video_frame.cc#newcode402 media/base/video_frame.cc:402: DLOG(ERROR) << "Not expecting alpha for the video format."; ...
4 years, 5 months ago (2016-01-13 01:32:28 UTC) #14
vignesh
https://codereview.chromium.org/1561703002/diff/60001/media/filters/vpx_video_decoder.cc File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/1561703002/diff/60001/media/filters/vpx_video_decoder.cc#newcode513 media/filters/vpx_video_decoder.cc:513: const struct vpx_image** vpx_image_alpha, On 2016/01/13 01:32:28, DaleCurtis wrote: ...
4 years, 5 months ago (2016-01-13 04:01:10 UTC) #15
DaleCurtis
https://codereview.chromium.org/1561703002/diff/60001/media/filters/vpx_video_decoder.cc File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/1561703002/diff/60001/media/filters/vpx_video_decoder.cc#newcode513 media/filters/vpx_video_decoder.cc:513: const struct vpx_image** vpx_image_alpha, On 2016/01/13 04:01:10, vignesh wrote: ...
4 years, 5 months ago (2016-01-13 18:59:55 UTC) #16
vignesh
https://codereview.chromium.org/1561703002/diff/60001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/1561703002/diff/60001/media/base/video_frame.cc#newcode402 media/base/video_frame.cc:402: DLOG(ERROR) << "Not expecting alpha for the video format."; ...
4 years, 5 months ago (2016-01-15 18:07:13 UTC) #17
DaleCurtis
lgtm https://codereview.chromium.org/1561703002/diff/80001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/1561703002/diff/80001/media/base/video_frame.cc#newcode395 media/base/video_frame.cc:395: DLOG(ERROR) << __FUNCTION__ << " Invalid config." I ...
4 years, 5 months ago (2016-01-15 18:31:09 UTC) #18
DaleCurtis
lgtm lgtm https://codereview.chromium.org/1561703002/diff/80001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/1561703002/diff/80001/media/base/video_frame.cc#newcode395 media/base/video_frame.cc:395: DLOG(ERROR) << __FUNCTION__ << " Invalid config." ...
4 years, 5 months ago (2016-01-15 18:31:10 UTC) #19
vignesh
https://codereview.chromium.org/1561703002/diff/80001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/1561703002/diff/80001/media/base/video_frame.cc#newcode395 media/base/video_frame.cc:395: DLOG(ERROR) << __FUNCTION__ << " Invalid config." On 2016/01/15 ...
4 years, 5 months ago (2016-01-15 19:05:14 UTC) #20
fgalligan1
On 2016/01/15 19:05:14, vignesh wrote: > https://codereview.chromium.org/1561703002/diff/80001/media/base/video_frame.cc > File media/base/video_frame.cc (right): > > https://codereview.chromium.org/1561703002/diff/80001/media/base/video_frame.cc#newcode395 > ...
4 years, 5 months ago (2016-01-15 23:42:57 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1561703002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1561703002/100001
4 years, 5 months ago (2016-01-19 17:21:29 UTC) #24
commit-bot: I haz the power
Committed patchset #6 (id:100001)
4 years, 5 months ago (2016-01-19 18:35:24 UTC) #25
commit-bot: I haz the power
Patchset 6 (id:??) landed as https://crrev.com/4863f9bda1800e1da901588ffd995364aaa70d8e Cr-Commit-Position: refs/heads/master@{#370147}
4 years, 5 months ago (2016-01-19 18:36:26 UTC) #27
benwells
4 years, 5 months ago (2016-01-20 02:17:32 UTC) #28
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1608113002/ by benwells@chromium.org.

The reason for reverting is: The new test is timing out on the ChromeOS valgrind
bot. It isn't clear if this is because the test is too slow for that bot (tests
on valgrind bots typically take longer to run) or if there is a real problem.

See log here:
https://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20OS%20%28...
.

Powered by Google App Engine
This is Rietveld 408576698