Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1411283003:
Add workaround for PPB_VideoDecoder not setting visible_rect. (Closed)
|
Created:
5 years, 2 months ago by Sergey Ulanov Modified:
5 years, 2 months ago CC:
chromium-reviews, chromoting-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd workaround for PPB_VideoDecoder not setting visible_rect.
Depending on the decoder being used PPB_VideoDecoder API may not set
visible_rect in the returned pictures. Work around that problem by
copying frame size from VideoPacket.
BUG=542945
Committed: https://crrev.com/83e7195f4794df0549112b177429ee8b204874fd
Cr-Commit-Position: refs/heads/master@{#355115}
Patch Set 1 #Patch Set 2 : #
Total comments: 5
Patch Set 3 : #
Total comments: 6
Patch Set 4 : #
Total comments: 3
Messages
Total messages: 18 (5 generated)
sergeyu@chromium.org changed reviewers: + kelvinp@chromium.org
https://codereview.chromium.org/1411283003/diff/20001/remoting/client/plugin/... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/1411283003/diff/20001/remoting/client/plugin/... remoting/client/plugin/pepper_video_renderer_3d.cc:374: CHECK(picture.texture_size.width > 0 && picture.texture_size.height > 0); These CHECKs will help to diagnose similar issues in the future.
wez@chromium.org changed reviewers: + wez@chromium.org
https://codereview.chromium.org/1411283003/diff/20001/remoting/client/plugin/... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/1411283003/diff/20001/remoting/client/plugin/... remoting/client/plugin/pepper_video_renderer_3d.cc:30: // stable channel. nit: Is there a bug filed for that work? If so, reference it here? https://codereview.chromium.org/1411283003/diff/20001/remoting/client/plugin/... File remoting/client/plugin/pepper_video_renderer_3d.h (right): https://codereview.chromium.org/1411283003/diff/20001/remoting/client/plugin/... remoting/client/plugin/pepper_video_renderer_3d.h:105: scoped_ptr<FrameSizesMap> frame_sizes_; Do you need something this complicated? Frame sizes don't change very often, so surely you can set the size to the minimum of the texture dimensions and the most-recent frame size from ProcessVideoPacket? You might then get artefacts on resize, but only for a single frame.
https://codereview.chromium.org/1411283003/diff/20001/remoting/client/plugin/... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/1411283003/diff/20001/remoting/client/plugin/... remoting/client/plugin/pepper_video_renderer_3d.cc:30: // stable channel. On 2015/10/19 21:56:16, Wez wrote: > nit: Is there a bug filed for that work? If so, reference it here? Added reference to the bug to get 1.1 API enabled on stable. https://codereview.chromium.org/1411283003/diff/20001/remoting/client/plugin/... File remoting/client/plugin/pepper_video_renderer_3d.h (right): https://codereview.chromium.org/1411283003/diff/20001/remoting/client/plugin/... remoting/client/plugin/pepper_video_renderer_3d.h:105: scoped_ptr<FrameSizesMap> frame_sizes_; On 2015/10/19 21:56:16, Wez wrote: > Do you need something this complicated? Frame sizes don't change very often, so > surely you can set the size to the minimum of the texture dimensions and the > most-recent frame size from ProcessVideoPacket? You might then get artefacts on > resize, but only for a single frame. Yes, good point. Done.
Patchset #3 (id:40001) has been deleted
posciak@chromium.org changed reviewers: + posciak@chromium.org
https://codereview.chromium.org/1411283003/diff/60001/remoting/client/plugin/... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/1411283003/diff/60001/remoting/client/plugin/... remoting/client/plugin/pepper_video_renderer_3d.cc:316: // Workaround crbug.com/542945 by filling in visible_rect if it isn't set. If you wouldn't mind, I would prefer not calling this a workaround. This is an intended behavior for decoders that cannot provide this size. https://codereview.chromium.org/1411283003/diff/60001/remoting/client/plugin/... remoting/client/plugin/pepper_video_renderer_3d.cc:326: std::min(frame_size_.width(), picture.texture_size.width); Wouldn't using frame_size_ be a problem is resolution changed and we were still decoding pictures from before the change? If ProcessVideoPacket() with new frame_size_ comes while we have previous pictures still decoding, then frame_size_ will change to the new size, but then we may get a OnPictureReady() for a picture for the old frame_size_? Could we store frame_size_ for each picture like GVD and RTCVD do? https://codereview.chromium.org/1411283003/diff/60001/remoting/client/plugin/... remoting/client/plugin/pepper_video_renderer_3d.cc:328: std::min(frame_size_.height(), picture.texture_size.width); s/picture.texture_size.width/picture.texture_size.height/
https://codereview.chromium.org/1411283003/diff/60001/remoting/client/plugin/... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/1411283003/diff/60001/remoting/client/plugin/... remoting/client/plugin/pepper_video_renderer_3d.cc:316: // Workaround crbug.com/542945 by filling in visible_rect if it isn't set. On 2015/10/20 00:41:36, Pawel Osciak wrote: > If you wouldn't mind, I would prefer not calling this a workaround. This is an > intended behavior for decoders that cannot provide this size. I still think this is a bug that should be fixed in the decoder API implementation. This behavior is not documented anywhere and there is no good reason API cannot provide correct visible_rect. Ideally I would prefer if it wasn't necessary to duplicate frame dimensions in the container. https://codereview.chromium.org/1411283003/diff/60001/remoting/client/plugin/... remoting/client/plugin/pepper_video_renderer_3d.cc:326: std::min(frame_size_.width(), picture.texture_size.width); On 2015/10/20 00:41:36, Pawel Osciak wrote: > Wouldn't using frame_size_ be a problem is resolution changed and we were still > decoding pictures from before the change? If ProcessVideoPacket() with new > frame_size_ comes while we have previous pictures still decoding, then > frame_size_ will change to the new size, but then we may get a OnPictureReady() > for a picture for the old frame_size_? > > Could we store frame_size_ for each picture like GVD and RTCVD do? Yes. Please see the previous version (patch set 2) of this CL and Wez's comment there. https://codereview.chromium.org/1411283003/diff/60001/remoting/client/plugin/... remoting/client/plugin/pepper_video_renderer_3d.cc:328: std::min(frame_size_.height(), picture.texture_size.width); On 2015/10/20 00:41:36, Pawel Osciak wrote: > s/picture.texture_size.width/picture.texture_size.height/ Done. Thanks for catching this.
https://codereview.chromium.org/1411283003/diff/80001/remoting/client/plugin/... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/1411283003/diff/80001/remoting/client/plugin/... remoting/client/plugin/pepper_video_renderer_3d.cc:321: LOG(WARNING) << "PPB_VideoDecoder doesn't set visible_rect."; What's the point of logging a warning in this case?
https://codereview.chromium.org/1411283003/diff/80001/remoting/client/plugin/... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/1411283003/diff/80001/remoting/client/plugin/... remoting/client/plugin/pepper_video_renderer_3d.cc:321: LOG(WARNING) << "PPB_VideoDecoder doesn't set visible_rect."; On 2015/10/20 04:57:44, Wez wrote: > What's the point of logging a warning in this case? To make it easy to verify if the problem is still there.
lgtm https://codereview.chromium.org/1411283003/diff/80001/remoting/client/plugin/... File remoting/client/plugin/pepper_video_renderer_3d.cc (right): https://codereview.chromium.org/1411283003/diff/80001/remoting/client/plugin/... remoting/client/plugin/pepper_video_renderer_3d.cc:321: LOG(WARNING) << "PPB_VideoDecoder doesn't set visible_rect."; On 2015/10/20 at 14:37:52, Sergey Ulanov wrote: > On 2015/10/20 04:57:44, Wez wrote: > > What's the point of logging a warning in this case? > > To make it easy to verify if the problem is still there. You mean easy to see whether the PPAPI has been fixed? Gotcha.
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411283003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411283003/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/83e7195f4794df0549112b177429ee8b204874fd Cr-Commit-Position: refs/heads/master@{#355115} |
