Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2892423004:
cc: make texture draw quad of video overlay candidate. (Closed)
Descriptioncc: make texture draw quad of video overlay candidate.
|overlay_resources.size_in_pixels| in TextureDrawQuad must have a value to
be promoted as overlay candidate.
BUG=683347
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2892423004
Cr-Commit-Position: refs/heads/master@{#477760}
Committed: https://chromium.googlesource.com/chromium/src/+/16c230301025a487d69f2233c4b1bf305367bb96
Patch Set 1 #Patch Set 2 : add unittests #
Messages
Total messages: 30 (16 generated)
Description was changed from ========== cc: make texture draw quad of video overable. |overlay_resources.size_in_pixels| in TextureDrawQuad must have a value to promote overlay. Check DrmOverlayManager::CanHandleCandidate() and DrmOverlayValidator::TestPageFlip(). BUG=683347 ========== to ========== cc: make texture draw quad of video overable. |overlay_resources.size_in_pixels| in TextureDrawQuad must have a value to promote overlay. Check DrmOverlayManager::CanHandleCandidate() and DrmOverlayValidator::TestPageFlip(). BUG=683347 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dongseong.hwang@intel.com changed reviewers: + dcastagna@chromium.org, reveman@chromium.org
dcastagna@, could you review? as we fixed dma-buf issue, let me resume video overlay work.
Description was changed from ========== cc: make texture draw quad of video overable. |overlay_resources.size_in_pixels| in TextureDrawQuad must have a value to promote overlay. Check DrmOverlayManager::CanHandleCandidate() and DrmOverlayValidator::TestPageFlip(). BUG=683347 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: make texture draw quad of video overable. |overlay_resources.size_in_pixels| in TextureDrawQuad must have a value to promote overlay. Check DrmOverlayManager::CanHandleCandidate() and DrmOverlayValidator::TestPageFlip(). BUG=683347 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/23 at 02:40:11, dongseong.hwang wrote: > dcastagna@, could you review? > as we fixed dma-buf issue, let me resume video overlay work. I'm afraid this change would break video playback on some devices where we use V4L2SliceVideoDecodeAccelerator. V4L2SliceVideoDecodeAccelerator produces textures that don't have a GLImage associated. I'm working on changing that, let's hold on this change until that part lands.
On 2017/05/23 04:00:43, Daniele Castagna wrote: > On 2017/05/23 at 02:40:11, dongseong.hwang wrote: > > dcastagna@, could you review? > > as we fixed dma-buf issue, let me resume video overlay work. > > I'm afraid this change would break video playback on some devices where we use > V4L2SliceVideoDecodeAccelerator. > V4L2SliceVideoDecodeAccelerator produces textures that don't have a GLImage > associated. > > I'm working on changing that, let's hold on this change until that part lands. Sounds good, but I have a question. In V4L2 case, resource_provider->IsOverlayCandidate() returns false, right? It's already guarded, so I think it's safe? https://cs.chromium.org/chromium/src/cc/output/overlay_candidate.cc?q=Overlay...
On 2017/05/23 at 04:47:55, dongseong.hwang wrote: > On 2017/05/23 04:00:43, Daniele Castagna wrote: > > On 2017/05/23 at 02:40:11, dongseong.hwang wrote: > > > dcastagna@, could you review? > > > as we fixed dma-buf issue, let me resume video overlay work. > > > > I'm afraid this change would break video playback on some devices where we use > > V4L2SliceVideoDecodeAccelerator. > > V4L2SliceVideoDecodeAccelerator produces textures that don't have a GLImage > > associated. > > > > I'm working on changing that, let's hold on this change until that part lands. > > Sounds good, but I have a question. > > In V4L2 case, resource_provider->IsOverlayCandidate() returns false, right? It's already guarded, so I think it's safe? > https://cs.chromium.org/chromium/src/cc/output/overlay_candidate.cc?q=Overlay... Last time I checked it was returning true. We could change that temporarily to not return true. Considering we won't be able to have overlays on Intel until we get atomic support in i915, I'd prefer to wait to have an actual use case before landing this patch.
On 2017/05/23 06:48:04, Daniele Castagna wrote: > On 2017/05/23 at 04:47:55, dongseong.hwang wrote: > > On 2017/05/23 04:00:43, Daniele Castagna wrote: > > > On 2017/05/23 at 02:40:11, dongseong.hwang wrote: > > > > dcastagna@, could you review? > > > > as we fixed dma-buf issue, let me resume video overlay work. > > > > > > I'm afraid this change would break video playback on some devices where we > use > > > V4L2SliceVideoDecodeAccelerator. > > > V4L2SliceVideoDecodeAccelerator produces textures that don't have a GLImage > > > associated. > > > > > > I'm working on changing that, let's hold on this change until that part > lands. > > > > Sounds good, but I have a question. > > > > In V4L2 case, resource_provider->IsOverlayCandidate() returns false, right? > It's already guarded, so I think it's safe? > > > https://cs.chromium.org/chromium/src/cc/output/overlay_candidate.cc?q=Overlay... > > Last time I checked it was returning true. We could change that temporarily to > not return true. I checked it returning false. It's because V4L2SliceVideoDecodeAccelerator is hardcoded to create non-overlayable Picture. |allow_overlay| is false in the below code. https://cs.chromium.org/chromium/src/media/gpu/v4l2_slice_video_decode_accele... > Considering we won't be able to have overlays on Intel until we get atomic > support in i915, I'd prefer to wait to have an actual use case before landing > this patch. Tarun in Intel ChromeOS integration team is backporting all necessary patches for nuclear page flip to v4.4. https://chromium-review.googlesource.com/q/owner:tarun.vyas%40intel.com We will be able to enable hardware overlay on Kabylake and Apollolake chromebook soon. I think it's good to make it ready from chromium side meanwhile. What do you think?
Daniele, V4L2 decoder never produce overlayable video frame, so we can land it. What do you think?
On 2017/05/31 at 21:14:30, dongseong.hwang wrote: > Daniele, V4L2 decoder never produce overlayable video frame, so we can land it. What do you think? How did you check that that V4L2 never produces a video frame that can be used as overlay? Btw, on rockchip we are using V4L2 slice, not V4L2 video decoder. Should we add a test for this? Also, please notice that we're back using StreamVideoDrawQuad momentarily: https://codereview.chromium.org/2920893003/
On 2017/06/03 19:27:49, Daniele Castagna wrote: > On 2017/05/31 at 21:14:30, dongseong.hwang wrote: > > Daniele, V4L2 decoder never produce overlayable video frame, so we can land > it. What do you think? > > How did you check that that V4L2 never produces a video frame that can be used > as overlay? Btw, on rockchip we are using V4L2 slice, not V4L2 video decoder. as I mentioned earlier, V4L2SliceVideoDecodeAccelerator::OutputSurface() always produce a Picture(allow_overlay = false). https://cs.chromium.org/chromium/src/media/gpu/v4l2_slice_video_decode_accele... > Should we add a test for this? |allow_overlay| depends on backend decoder implementation, but the unittest could not know what is exact subclass of GpuVideoDecodeAccelerator, right? > Also, please notice that we're back using StreamVideoDrawQuad momentarily: > https://codereview.chromium.org/2920893003/ StreamVideoDrawQuad also check |IsOverlayCandidate|, and V4L2SliceVideoDecodeAccelerator always mark |allow_overlay| to false. So it cannot be broken in terms of overlay. https://cs.chromium.org/chromium/src/cc/output/overlay_candidate.cc?type=cs&l...
On 2017/06/05 at 17:38:54, dongseong.hwang wrote: > On 2017/06/03 19:27:49, Daniele Castagna wrote: > > On 2017/05/31 at 21:14:30, dongseong.hwang wrote: > > > Daniele, V4L2 decoder never produce overlayable video frame, so we can land > > it. What do you think? > > > > How did you check that that V4L2 never produces a video frame that can be used > > as overlay? Btw, on rockchip we are using V4L2 slice, not V4L2 video decoder. > > as I mentioned earlier, V4L2SliceVideoDecodeAccelerator::OutputSurface() always produce a Picture(allow_overlay = false). > https://cs.chromium.org/chromium/src/media/gpu/v4l2_slice_video_decode_accele... You said V4L2 never produces video frames with ALLOW_OVERLAY metadata set to true, I was pointing out I was talking about the other GPU video decoder. Good to know it doesn't right now, but let's keep in mind it will soon. > > > Should we add a test for this? > > |allow_overlay| depends on backend decoder implementation, but the unittest could not know what is exact subclass of GpuVideoDecodeAccelerator, right? > I was asking if you should *unit* test this. You'd be testing just your change, not the interaction with the rest of the system. We could just check if resource_size_in_pixels is set when we have a native video frame in video_layer_impl_unittest.cc I think this check is worth adding if it doesn't require too much work. Otherwise, this CL LGTM. > > Also, please notice that we're back using StreamVideoDrawQuad momentarily: > > https://codereview.chromium.org/2920893003/ > > StreamVideoDrawQuad also check |IsOverlayCandidate|, and V4L2SliceVideoDecodeAccelerator always mark |allow_overlay| to false. So it cannot be broken in terms of overlay. > https://cs.chromium.org/chromium/src/cc/output/overlay_candidate.cc?type=cs&l... Can you also change the title of the CL, "overable" shuould be "overlayable" or "overlay candidate". I'm also not sure what you mean with "Check DrmOverlayManager::CanHandleCandidate() and DrmOverlayValidator::TestPageFlip()."
The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== cc: make texture draw quad of video overable. |overlay_resources.size_in_pixels| in TextureDrawQuad must have a value to promote overlay. Check DrmOverlayManager::CanHandleCandidate() and DrmOverlayValidator::TestPageFlip(). BUG=683347 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: make texture draw quad of video overlay candidate. |overlay_resources.size_in_pixels| in TextureDrawQuad must have a value to be promoted as overlay candidate. BUG=683347 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Hi Daniele, I added unittests for TextureDrawQuad as well as StreamVideoDrawQuad. When you will change stream video, you need to change this new test. :) Could you review again?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
reveman@, could you review it as owner?
lgtm
The CQ bit was checked by dongseong.hwang@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from dcastagna@chromium.org Link to the patchset: https://codereview.chromium.org/2892423004/#ps20001 (title: "add unittests")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496867504365890,
"parent_rev": "7c8bd685a9baeff5169fd4cc63422a69d56b1046", "commit_rev":
"16c230301025a487d69f2233c4b1bf305367bb96"}
Message was sent while issue was closed.
Description was changed from ========== cc: make texture draw quad of video overlay candidate. |overlay_resources.size_in_pixels| in TextureDrawQuad must have a value to be promoted as overlay candidate. BUG=683347 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: make texture draw quad of video overlay candidate. |overlay_resources.size_in_pixels| in TextureDrawQuad must have a value to be promoted as overlay candidate. BUG=683347 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2892423004 Cr-Commit-Position: refs/heads/master@{#477760} Committed: https://chromium.googlesource.com/chromium/src/+/16c230301025a487d69f2233c4b1... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/16c230301025a487d69f2233c4b1... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
