Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1561703002:
media/vpx: Add support for VP9 alpha channel (Closed)
Descriptionmedia/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 #
Messages
Total messages: 28 (4 generated)
vigneshv@chromium.org changed reviewers: + dalecurtis@chromium.org, tomfinegan@chromium.org
https://codereview.chromium.org/1561703002/diff/1/media/filters/vpx_video_dec... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/1561703002/diff/1/media/filters/vpx_video_dec... media/filters/vpx_video_decoder.cc:403: // Configure non-YV12A VP9 to decode on our buffers to skip a data copy on I guess we can't copy into one of the buffers since each buffer may be reused for a future frame? https://codereview.chromium.org/1561703002/diff/1/media/test/pipeline_integra... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/1561703002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test.cc:1838: ASSERT_EQ(PIPELINE_OK, Start("bear-vp9a.webm")); Can you create these as kClockless type tests? We're starting to get too many of these and they take #media_duration in real-time to complete.
https://codereview.chromium.org/1561703002/diff/1/media/filters/vpx_video_dec... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/1561703002/diff/1/media/filters/vpx_video_dec... media/filters/vpx_video_decoder.cc:403: // Configure non-YV12A VP9 to decode on our buffers to skip a data copy on On 2016/01/05 21:22:21, DaleCurtis wrote: > I guess we can't copy into one of the buffers since each buffer may be reused > for a future frame? The thing is that the Y, U and V planes come from one instance of the decoder and the Alpha plane comes from another instance of the decoder. It seems a lot simpler to just do a copy of all the 4 planes. https://codereview.chromium.org/1561703002/diff/1/media/test/pipeline_integra... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/1561703002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test.cc:1838: ASSERT_EQ(PIPELINE_OK, Start("bear-vp9a.webm")); On 2016/01/05 21:22:21, DaleCurtis wrote: > Can you create these as kClockless type tests? We're starting to get too many > of these and they take #media_duration in real-time to complete. I have marked these two as kClockless. Would you like me to change the other BasicPlayback_VP{8,9}_* tests as well?
lgtm https://codereview.chromium.org/1561703002/diff/1/media/test/pipeline_integra... File media/test/pipeline_integration_test.cc (right): https://codereview.chromium.org/1561703002/diff/1/media/test/pipeline_integra... media/test/pipeline_integration_test.cc:1838: ASSERT_EQ(PIPELINE_OK, Start("bear-vp9a.webm")); On 2016/01/06 00:07:49, vignesh wrote: > On 2016/01/05 21:22:21, DaleCurtis wrote: > > Can you create these as kClockless type tests? We're starting to get too many > > of these and they take #media_duration in real-time to complete. > > I have marked these two as kClockless. Would you like me to change the other > BasicPlayback_VP{8,9}_* tests as well? Sure, if they all still pass.
https://codereview.chromium.org/1561703002/diff/1/media/filters/vpx_video_dec... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/1561703002/diff/1/media/filters/vpx_video_dec... media/filters/vpx_video_decoder.cc:403: // Configure non-YV12A VP9 to decode on our buffers to skip a data copy on On 2016/01/06 00:07:49, vignesh wrote: > On 2016/01/05 21:22:21, DaleCurtis wrote: > > I guess we can't copy into one of the buffers since each buffer may be reused > > for a future frame? > > The thing is that the Y, U and V planes come from one instance of the decoder > and the Alpha plane comes from another instance of the decoder. It seems a lot > simpler to just do a copy of all the 4 planes. Hmm, could you over allocate the first buffer and copy in the A plane? If the non-alpha decoder isn't using that plane, seems we could avoid a copy of the main plane (most expensive) and keep only the copy of the A plane.
https://codereview.chromium.org/1561703002/diff/1/media/filters/vpx_video_dec... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/1561703002/diff/1/media/filters/vpx_video_dec... media/filters/vpx_video_decoder.cc:403: // Configure non-YV12A VP9 to decode on our buffers to skip a data copy on On 2016/01/06 00:18:02, DaleCurtis wrote: > On 2016/01/06 00:07:49, vignesh wrote: > > On 2016/01/05 21:22:21, DaleCurtis wrote: > > > I guess we can't copy into one of the buffers since each buffer may be > reused > > > for a future frame? > > > > The thing is that the Y, U and V planes come from one instance of the decoder > > and the Alpha plane comes from another instance of the decoder. It seems a lot > > simpler to just do a copy of all the 4 planes. > > Hmm, could you over allocate the first buffer and copy in the A plane? If the > non-alpha decoder isn't using that plane, seems we could avoid a copy of the > main plane (most expensive) and keep only the copy of the A plane. Good point, i will try it out and update the CL.
https://codereview.chromium.org/1561703002/diff/1/media/filters/vpx_video_dec... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/1561703002/diff/1/media/filters/vpx_video_dec... media/filters/vpx_video_decoder.cc:403: // Configure non-YV12A VP9 to decode on our buffers to skip a data copy on On 2016/01/06 00:18:02, DaleCurtis wrote: > On 2016/01/06 00:07:49, vignesh wrote: > > On 2016/01/05 21:22:21, DaleCurtis wrote: > > > I guess we can't copy into one of the buffers since each buffer may be > reused > > > for a future frame? > > > > The thing is that the Y, U and V planes come from one instance of the decoder > > and the Alpha plane comes from another instance of the decoder. It seems a lot > > simpler to just do a copy of all the 4 planes. > > Hmm, could you over allocate the first buffer and copy in the A plane? If the > non-alpha decoder isn't using that plane, seems we could avoid a copy of the > main plane (most expensive) and keep only the copy of the A plane. ok, i looked into this and i'm a little confused. can you please explain a little more about what you mean by "over allocate the first buffer" ? as it is, we are using VideoFrame::WrapExternalYuvData for the Y,U and V planes. Which means VideoFrame doesn't own any of those buffers. And to copy the Alpha planes, we would need VideoFrame's own buffer. So it ends up being confusing.
https://codereview.chromium.org/1561703002/diff/1/media/filters/vpx_video_dec... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/1561703002/diff/1/media/filters/vpx_video_dec... media/filters/vpx_video_decoder.cc:403: // Configure non-YV12A VP9 to decode on our buffers to skip a data copy on On 2016/01/08 19:21:33, vignesh wrote: > On 2016/01/06 00:18:02, DaleCurtis wrote: > > On 2016/01/06 00:07:49, vignesh wrote: > > > On 2016/01/05 21:22:21, DaleCurtis wrote: > > > > I guess we can't copy into one of the buffers since each buffer may be > > reused > > > > for a future frame? > > > > > > The thing is that the Y, U and V planes come from one instance of the > decoder > > > and the Alpha plane comes from another instance of the decoder. It seems a > lot > > > simpler to just do a copy of all the 4 planes. > > > > Hmm, could you over allocate the first buffer and copy in the A plane? If the > > non-alpha decoder isn't using that plane, seems we could avoid a copy of the > > main plane (most expensive) and keep only the copy of the A plane. > > ok, i looked into this and i'm a little confused. can you please explain a > little more about what you mean by "over allocate the first buffer" ? > > as it is, we are using VideoFrame::WrapExternalYuvData for the Y,U and V planes. > Which means VideoFrame doesn't own any of those buffers. And to copy the Alpha > planes, we would need VideoFrame's own buffer. So it ends up being confusing. Yes, you'd need to add a WrapExternalYUVA method and then change the memory pool to allocate the space for the extra plane. You would then copy the A plane into the buffer owned by the memory pool -- which since the first decoder is only YUV it would never know about.
took a shot at avoiding copying of Y, U and V planes. The code ended up looking somewhat unnecessarily complicated. PTAL. IMO, alpha is a rarely used feature and i think it might be okay to stick to patch set 2 to avoid the complications to gain a small amount of performance. Tom/Frank/Dale, WDYT?
Aside from some minor cleanup it doesn't look that bad to me. What part irks you? https://codereview.chromium.org/1561703002/diff/40001/media/filters/vpx_video... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/1561703002/diff/40001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:475: const vpx_image_t* vpx_image_alpha = nullptr; Seems like this should go in its own function? DecodeAlphaPlane or something.
On 2016/01/12 01:18:20, DaleCurtis wrote: > Aside from some minor cleanup it doesn't look that bad to me. What part irks > you? > It just seemed like too many paths and too many combinations (VP8, VP9 x Alpha and non-alpha). Anyway, we can stick to it if you think it's okay. Will address the other comments in a bit. > https://codereview.chromium.org/1561703002/diff/40001/media/filters/vpx_video... > File media/filters/vpx_video_decoder.cc (right): > > https://codereview.chromium.org/1561703002/diff/40001/media/filters/vpx_video... > media/filters/vpx_video_decoder.cc:475: const vpx_image_t* vpx_image_alpha = > nullptr; > Seems like this should go in its own function? DecodeAlphaPlane or something.
I have changed the behavior in case where the file claims it has alpha but the alpha plane is actually invalid (side_data_id != 1 or some such): instead of setting alpha to be opaque, i am setting the colorspace of the videoframe to be YV12. Other than that, all old behavior should remain unchanged. PTAL. https://codereview.chromium.org/1561703002/diff/40001/media/filters/vpx_video... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/1561703002/diff/40001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:475: const vpx_image_t* vpx_image_alpha = nullptr; On 2016/01/12 01:18:20, DaleCurtis wrote: > Seems like this should go in its own function? DecodeAlphaPlane or something. Done.
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.... media/base/video_frame.cc:402: DLOG(ERROR) << "Not expecting alpha for the video format."; Fix message. Expecting YUVA planes, etc. https://codereview.chromium.org/1561703002/diff/60001/media/filters/vpx_video... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/1561703002/diff/60001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:513: const struct vpx_image** vpx_image_alpha, possible to avoid passing **? https://codereview.chromium.org/1561703002/diff/60001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:516: if (vpx_codec_alpha_ && buffer->side_data_size() >= 8) { return early instead of indenting the whole function, ditto for a few of the conditionals below too. https://codereview.chromium.org/1561703002/diff/60001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:541: return kNoAlphaPlaneData; It's a bit confusing that kOk is returned for non-alpha video. Not sure of a better suggestion though.
https://codereview.chromium.org/1561703002/diff/60001/media/filters/vpx_video... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/1561703002/diff/60001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:513: const struct vpx_image** vpx_image_alpha, On 2016/01/13 01:32:28, DaleCurtis wrote: > possible to avoid passing **? i can swap this with the return value. that would mean passing a AlphaDecodeStatus*, would that be ok? https://codereview.chromium.org/1561703002/diff/60001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:541: return kNoAlphaPlaneData; On 2016/01/13 01:32:28, DaleCurtis wrote: > It's a bit confusing that kOk is returned for non-alpha video. Not sure of a > better suggestion though. I defined the function semantic as follows: * returns kOk if there are no fatal errors. In this case vpx_image_alpha will be populated if a valid alpha plane exists, otherwise it will be null. * return kFailure on a fatal error. in this case vpx_image_alpha will be null. * return kNoAlphaPlane if a non-fatal error occurred. vpx_image_alpha will be null. Thoughts on changing this for better?
https://codereview.chromium.org/1561703002/diff/60001/media/filters/vpx_video... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/1561703002/diff/60001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:513: const struct vpx_image** vpx_image_alpha, On 2016/01/13 04:01:10, vignesh wrote: > On 2016/01/13 01:32:28, DaleCurtis wrote: > > possible to avoid passing **? > > i can swap this with the return value. that would mean passing a > AlphaDecodeStatus*, would that be ok? It's no different semantically, so what you have is fine. https://codereview.chromium.org/1561703002/diff/60001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:541: return kNoAlphaPlaneData; On 2016/01/13 04:01:10, vignesh wrote: > On 2016/01/13 01:32:28, DaleCurtis wrote: > > It's a bit confusing that kOk is returned for non-alpha video. Not sure of a > > better suggestion though. > > I defined the function semantic as follows: > * returns kOk if there are no fatal errors. In this case vpx_image_alpha will be > populated if a valid alpha plane exists, otherwise it will be null. > * return kFailure on a fatal error. in this case vpx_image_alpha will be null. > * return kNoAlphaPlane if a non-fatal error occurred. vpx_image_alpha will be > null. > > Thoughts on changing this for better? Nope, if nothing sticks out lets just go with this.
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.... media/base/video_frame.cc:402: DLOG(ERROR) << "Not expecting alpha for the video format."; On 2016/01/13 01:32:28, DaleCurtis wrote: > Fix message. Expecting YUVA planes, etc. Done. https://codereview.chromium.org/1561703002/diff/60001/media/filters/vpx_video... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/1561703002/diff/60001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:516: if (vpx_codec_alpha_ && buffer->side_data_size() >= 8) { On 2016/01/13 01:32:28, DaleCurtis wrote: > return early instead of indenting the whole function, ditto for a few of the > conditionals below too. Done.
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.... media/base/video_frame.cc:395: DLOG(ERROR) << __FUNCTION__ << " Invalid config." I believe there's another CL which changed these messages to type LOG(DFATAL) -- can you sync and rebase (and change these two)? https://codereview.chromium.org/1561703002/diff/80001/media/filters/vpx_video... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/1561703002/diff/80001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:570: libyuv::CopyPlane((*vpx_image_alpha)->planes[VPX_PLANE_Y], Out of curiosity, would it be possible to avoid the copy by giving libvpx the memory pool's alpha buffer? I'd worry about providing the wrong frame. If you think it's possible it might be worth putting a TODO here for future optimizations.
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.... media/base/video_frame.cc:395: DLOG(ERROR) << __FUNCTION__ << " Invalid config." I believe there's another CL which changed these messages to type LOG(DFATAL) -- can you sync and rebase (and change these two)? https://codereview.chromium.org/1561703002/diff/80001/media/filters/vpx_video... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/1561703002/diff/80001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:570: libyuv::CopyPlane((*vpx_image_alpha)->planes[VPX_PLANE_Y], Out of curiosity, would it be possible to avoid the copy by giving libvpx the memory pool's alpha buffer? I'd worry about providing the wrong frame. If you think it's possible it might be worth putting a TODO here for future optimizations.
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.... media/base/video_frame.cc:395: DLOG(ERROR) << __FUNCTION__ << " Invalid config." On 2016/01/15 18:31:09, DaleCurtis wrote: > I believe there's another CL which changed these messages to type LOG(DFATAL) -- > can you sync and rebase (and change these two)? Done. https://codereview.chromium.org/1561703002/diff/80001/media/filters/vpx_video... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/1561703002/diff/80001/media/filters/vpx_video... media/filters/vpx_video_decoder.cc:570: libyuv::CopyPlane((*vpx_image_alpha)->planes[VPX_PLANE_Y], On 2016/01/15 18:31:09, DaleCurtis wrote: > Out of curiosity, would it be possible to avoid the copy by giving libvpx the > memory pool's alpha buffer? I'd worry about providing the wrong frame. If you > think it's possible it might be worth putting a TODO here for future > optimizations. It's theoretically possible but there are a couple of issues. 1) Libvpx only accepts data buffers as a whole (y, u and v planes combined). We don't care about the u and v planes of the alpha decoder. 2) There is one more thing to keep track of since the alpha decoder may release the particular piece of memory earlier or later than the regular decoder. Not putting a TODO since it's unlikely that we will ever do this.
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.... > media/base/video_frame.cc:395: DLOG(ERROR) << __FUNCTION__ << " Invalid config." > On 2016/01/15 18:31:09, DaleCurtis wrote: > > I believe there's another CL which changed these messages to type LOG(DFATAL) > -- > > can you sync and rebase (and change these two)? > > Done. > > https://codereview.chromium.org/1561703002/diff/80001/media/filters/vpx_video... > File media/filters/vpx_video_decoder.cc (right): > > https://codereview.chromium.org/1561703002/diff/80001/media/filters/vpx_video... > media/filters/vpx_video_decoder.cc:570: > libyuv::CopyPlane((*vpx_image_alpha)->planes[VPX_PLANE_Y], > On 2016/01/15 18:31:09, DaleCurtis wrote: > > Out of curiosity, would it be possible to avoid the copy by giving libvpx the > > memory pool's alpha buffer? I'd worry about providing the wrong frame. If you > > think it's possible it might be worth putting a TODO here for future > > optimizations. > > It's theoretically possible but there are a couple of issues. > > 1) Libvpx only accepts data buffers as a whole (y, u and v planes combined). We > don't care about the u and v planes of the alpha decoder. > > 2) There is one more thing to keep track of since the alpha decoder may release > the particular piece of memory earlier or later than the regular decoder. > > Not putting a TODO since it's unlikely that we will ever do this. lgtm We don't have to add a todo here, but we can put it on our radar as a very low priority.
The CQ bit was checked by vigneshv@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1561703002/#ps100001 (title: "rebase")
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
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== 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> ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/4863f9bda1800e1da901588ffd995364aaa70d8e Cr-Commit-Position: refs/heads/master@{#370147}
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... . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
