Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1060433002:
VDA: Don't wait for the VSyncProvider in headless mode (Closed)
|
Created:
5 years, 8 months ago by dnicoara Modified:
5 years, 8 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@fix-vda2 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionVDA: Don't wait for the VSyncProvider in headless mode
The VSyncProvider does not guarantee that the callback is ever called.
On Ozone-GBM, the VSyncProvider relies on CRTC information to determine
when the vsync deadline will occurr. Since we're headless, that
information cannot be use, so the callback will never be called.
BUG=471550
Committed: https://crrev.com/42d7f4ea43b42a3bc1405a2b9561e15b3fb37d77
Cr-Commit-Position: refs/heads/master@{#326786}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add comment & rebase #
Total comments: 8
Patch Set 3 : . #Patch Set 4 : . #
Total comments: 2
Patch Set 5 : nits #
Messages
Total messages: 27 (7 generated)
dnicoara@chromium.org changed reviewers: + posciak@chromium.org
I got VDA working on a headless panther with this patch (and the window size one). Did we ever have VDA tests running on headless devices before on X11? Just reading the documentation of the GLX sync_control extension (https://www.opengl.org/registry/specs/OML/glx_sync_control.txt) it makes me think this wouldn't have worked on X11 either since the counters are values taken from the CRTC and there wouldn't be any CRTC in headless mode.
posciak@chromium.org changed reviewers: + owenlin@chromium.org
lgtm % a nit
On 2015/04/07 05:06:35, Owen Lin wrote: > lgtm % a nit Did you forget the nit?
Oops, it is still in my draft. https://codereview.chromium.org/1060433002/diff/1/content/common/gpu/media/re... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/1060433002/diff/1/content/common/gpu/media/re... content/common/gpu/media/rendering_helper.cc:481: if (!is_headless_ && vsync_provider && frame_duration_ != base::TimeDelta()) Please add a comment.
https://codereview.chromium.org/1060433002/diff/1/content/common/gpu/media/re... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/1060433002/diff/1/content/common/gpu/media/re... content/common/gpu/media/rendering_helper.cc:481: if (!is_headless_ && vsync_provider && frame_duration_ != base::TimeDelta()) On 2015/04/15 02:57:53, Owen Lin wrote: > Please add a comment. Done.
Pawel, would you mind taking a look for owners review?
https://codereview.chromium.org/1060433002/diff/20001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/1060433002/diff/20001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:246: is_headless_ = true; This is confusing, since only on ozone we may set this to true, however we could be headless somewhere else as well. I understand it's only useful on ozone, but it's still confusing. But since this is only used for vsync provider, and it's vsync provider that doesn't work well in this case, should we instead make vsync provider handle this, or even not create one at all in case of headless? https://codereview.chromium.org/1060433002/diff/20001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:269: LOG(WARNING) << "Running in headless mode"; Should this be a WARNING? This is normal on some devices, so I'd prefer something like DVLOG(1)... https://codereview.chromium.org/1060433002/diff/20001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:484: // mode the surface isn't associated with a CRTC so the vsync provider can not Should GetVSyncProvider() return != NULL if there is no CRTC at all? Does having one make sense in that case?
https://codereview.chromium.org/1060433002/diff/20001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/1060433002/diff/20001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:246: is_headless_ = true; On 2015/04/17 00:33:25, Pawel Osciak wrote: > This is confusing, since only on ozone we may set this to true, however we could > be headless somewhere else as well. I understand it's only useful on ozone, but > it's still confusing. But since this is only used for vsync provider, and it's > vsync provider that doesn't work well in this case, should we instead make vsync > provider handle this, or even not create one at all in case of headless? I don't think this is Ozone specific, but I haven't seen any X11 test runs in headless mode so I can't tell how the test would execute on X11. Based on reading the X11 Intel driver I believe it would end up never calling the callback as well. It would end up in https://code.google.com/p/chromium/codesearch#chromium/src/ui/gl/sync_control... always returning without the callback since the MSC would be 0 since there wouldn't be a CRTC. The vsync provider does work as expected. The interface explicitly mentions that the callback may never be called if there is no data source. https://codereview.chromium.org/1060433002/diff/20001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:269: LOG(WARNING) << "Running in headless mode"; On 2015/04/17 00:33:25, Pawel Osciak wrote: > Should this be a WARNING? This is normal on some devices, so I'd prefer > something like DVLOG(1)... Done. https://codereview.chromium.org/1060433002/diff/20001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:484: // mode the surface isn't associated with a CRTC so the vsync provider can not On 2015/04/17 00:33:25, Pawel Osciak wrote: > Should GetVSyncProvider() return != NULL if there is no CRTC at all? Does having > one make sense in that case? VSync providers are created based on the supported features of the underlying system rather than what the surface supports at the specific point in time. Also VSync providers are created on a per-surface basis. Since surfaces can be moved around, it is possible to have the surface associated with a CRTC at one point in time and disassociated at another point in time. In my opinion it would have been better had the GLSurface implementations would always require a valid VSync provider, even if it has to be a stub.
https://codereview.chromium.org/1060433002/diff/20001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/1060433002/diff/20001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:246: is_headless_ = true; On 2015/04/17 14:45:20, dnicoara wrote: > On 2015/04/17 00:33:25, Pawel Osciak wrote: > > This is confusing, since only on ozone we may set this to true, however we > could > > be headless somewhere else as well. I understand it's only useful on ozone, > but > > it's still confusing. But since this is only used for vsync provider, and it's > > vsync provider that doesn't work well in this case, should we instead make > vsync > > provider handle this, or even not create one at all in case of headless? > > I don't think this is Ozone specific, but I haven't seen any X11 test runs in > headless mode so I can't tell how the test would execute on X11. Based on > reading the X11 Intel driver I believe it would end up never calling the > callback as well. > > It would end up in > https://code.google.com/p/chromium/codesearch#chromium/src/ui/gl/sync_control... > always returning without the callback since the MSC would be 0 since there > wouldn't be a CRTC. > > The vsync provider does work as expected. The interface explicitly mentions that > the callback may never be called if there is no data source. Hm, personally I think it would be ideal for this to be a queryable property of VSyncProvider whether we should expect vsync info. Otherwise we are making assumptions here on internal workings of VSyncProvider impls, which may change if VSyncProvider impls change, without changing this. But if you feel that's not feasible, I understand that. If that's the case. Could we refactor this a bit though please? First of all, we call gl_surface_->GetVSyncProvider() a few times in this class, could we instead call it once on initialization here and only if framebuffer_size.IsEmpty()? Then, we could get rid of the is_headless_ and just check for if (vsync_provider_). Would that make sense?
https://codereview.chromium.org/1060433002/diff/20001/content/common/gpu/medi... File content/common/gpu/media/rendering_helper.cc (right): https://codereview.chromium.org/1060433002/diff/20001/content/common/gpu/medi... content/common/gpu/media/rendering_helper.cc:246: is_headless_ = true; On 2015/04/21 01:39:49, Pawel Osciak wrote: > On 2015/04/17 14:45:20, dnicoara wrote: > > On 2015/04/17 00:33:25, Pawel Osciak wrote: > > > This is confusing, since only on ozone we may set this to true, however we > > could > > > be headless somewhere else as well. I understand it's only useful on ozone, > > but > > > it's still confusing. But since this is only used for vsync provider, and > it's > > > vsync provider that doesn't work well in this case, should we instead make > > vsync > > > provider handle this, or even not create one at all in case of headless? > > > > I don't think this is Ozone specific, but I haven't seen any X11 test runs in > > headless mode so I can't tell how the test would execute on X11. Based on > > reading the X11 Intel driver I believe it would end up never calling the > > callback as well. > > > > It would end up in > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/gl/sync_control... > > always returning without the callback since the MSC would be 0 since there > > wouldn't be a CRTC. > > > > The vsync provider does work as expected. The interface explicitly mentions > that > > the callback may never be called if there is no data source. > > Hm, personally I think it would be ideal for this to be a queryable property of > VSyncProvider whether we should expect vsync info. Otherwise we are making > assumptions here on internal workings of VSyncProvider impls, which may change > if VSyncProvider impls change, without changing this. But if you feel that's not > feasible, I understand that. If that's the case. Could we refactor this a bit > though please? > I agree with you, the VSyncProvider should be simpler, though I don't think that is a trivial change and I'm weary of making changes that would affect assumptions and timings in the Compositor. > First of all, we call gl_surface_->GetVSyncProvider() a few times in this class, > could we instead call it once on initialization here and only if > framebuffer_size.IsEmpty()? Then, we could get rid of the is_headless_ and just > check for if (vsync_provider_). Would that make sense? This is too early as we don't have a GLSurface at this point. RenderingHelper::Setup() is the "browser" side initialization. The GLSurface is initialized later on in RenderingHelper::Initialize() on a different thread. I didn't want to access DisplayConfigurator from another thread since that would add assumptions on DisplayConfigurator.I felt that keeping track of the state locally would make this more resilient to future changes.
On 2015/04/21 14:21:31, dnicoara wrote: > https://codereview.chromium.org/1060433002/diff/20001/content/common/gpu/medi... > File content/common/gpu/media/rendering_helper.cc (right): > > https://codereview.chromium.org/1060433002/diff/20001/content/common/gpu/medi... > content/common/gpu/media/rendering_helper.cc:246: is_headless_ = true; > On 2015/04/21 01:39:49, Pawel Osciak wrote: > > On 2015/04/17 14:45:20, dnicoara wrote: > > > On 2015/04/17 00:33:25, Pawel Osciak wrote: > > > > This is confusing, since only on ozone we may set this to true, however we > > > could > > > > be headless somewhere else as well. I understand it's only useful on > ozone, > > > but > > > > it's still confusing. But since this is only used for vsync provider, and > > it's > > > > vsync provider that doesn't work well in this case, should we instead make > > > vsync > > > > provider handle this, or even not create one at all in case of headless? > > > > > > I don't think this is Ozone specific, but I haven't seen any X11 test runs > in > > > headless mode so I can't tell how the test would execute on X11. Based on > > > reading the X11 Intel driver I believe it would end up never calling the > > > callback as well. > > > > > > It would end up in > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/gl/sync_control... > > > always returning without the callback since the MSC would be 0 since there > > > wouldn't be a CRTC. > > > > > > The vsync provider does work as expected. The interface explicitly mentions > > that > > > the callback may never be called if there is no data source. > > > > Hm, personally I think it would be ideal for this to be a queryable property > of > > VSyncProvider whether we should expect vsync info. Otherwise we are making > > assumptions here on internal workings of VSyncProvider impls, which may change > > if VSyncProvider impls change, without changing this. But if you feel that's > not > > feasible, I understand that. If that's the case. Could we refactor this a bit > > though please? > > > > I agree with you, the VSyncProvider should be simpler, though I don't think that > is a trivial change and I'm weary of making changes that would affect > assumptions and timings in the Compositor. > > > First of all, we call gl_surface_->GetVSyncProvider() a few times in this > class, > > could we instead call it once on initialization here and only if > > framebuffer_size.IsEmpty()? Then, we could get rid of the is_headless_ and > just > > check for if (vsync_provider_). Would that make sense? > > This is too early as we don't have a GLSurface at this point. > RenderingHelper::Setup() is the "browser" side initialization. The GLSurface is > initialized later on in RenderingHelper::Initialize() on a different thread. I > didn't want to access DisplayConfigurator from another thread since that would > add assumptions on DisplayConfigurator.I felt that keeping track of the state > locally would make this more resilient to future changes. Ok. If so, could we change the name of is_headless_ to ingore_vsync_ or something like that please? Also, we seem to check for is_headless_ only in on place where we call GetVSyncParameters(), but there is another call to it in RenderContent(), should we have it in there as well?
On 2015/04/22 09:06:53, Pawel Osciak wrote: > On 2015/04/21 14:21:31, dnicoara wrote: > > > https://codereview.chromium.org/1060433002/diff/20001/content/common/gpu/medi... > > File content/common/gpu/media/rendering_helper.cc (right): > > > > > https://codereview.chromium.org/1060433002/diff/20001/content/common/gpu/medi... > > content/common/gpu/media/rendering_helper.cc:246: is_headless_ = true; > > On 2015/04/21 01:39:49, Pawel Osciak wrote: > > > On 2015/04/17 14:45:20, dnicoara wrote: > > > > On 2015/04/17 00:33:25, Pawel Osciak wrote: > > > > > This is confusing, since only on ozone we may set this to true, however > we > > > > could > > > > > be headless somewhere else as well. I understand it's only useful on > > ozone, > > > > but > > > > > it's still confusing. But since this is only used for vsync provider, > and > > > it's > > > > > vsync provider that doesn't work well in this case, should we instead > make > > > > vsync > > > > > provider handle this, or even not create one at all in case of headless? > > > > > > > > I don't think this is Ozone specific, but I haven't seen any X11 test runs > > in > > > > headless mode so I can't tell how the test would execute on X11. Based on > > > > reading the X11 Intel driver I believe it would end up never calling the > > > > callback as well. > > > > > > > > It would end up in > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/gl/sync_control... > > > > always returning without the callback since the MSC would be 0 since there > > > > wouldn't be a CRTC. > > > > > > > > The vsync provider does work as expected. The interface explicitly > mentions > > > that > > > > the callback may never be called if there is no data source. > > > > > > Hm, personally I think it would be ideal for this to be a queryable property > > of > > > VSyncProvider whether we should expect vsync info. Otherwise we are making > > > assumptions here on internal workings of VSyncProvider impls, which may > change > > > if VSyncProvider impls change, without changing this. But if you feel that's > > not > > > feasible, I understand that. If that's the case. Could we refactor this a > bit > > > though please? > > > > > > > I agree with you, the VSyncProvider should be simpler, though I don't think > that > > is a trivial change and I'm weary of making changes that would affect > > assumptions and timings in the Compositor. > > > > > First of all, we call gl_surface_->GetVSyncProvider() a few times in this > > class, > > > could we instead call it once on initialization here and only if > > > framebuffer_size.IsEmpty()? Then, we could get rid of the is_headless_ and > > just > > > check for if (vsync_provider_). Would that make sense? > > > > This is too early as we don't have a GLSurface at this point. > > RenderingHelper::Setup() is the "browser" side initialization. The GLSurface > is > > initialized later on in RenderingHelper::Initialize() on a different thread. I > > didn't want to access DisplayConfigurator from another thread since that would > > add assumptions on DisplayConfigurator.I felt that keeping track of the state > > locally would make this more resilient to future changes. > > Ok. If so, could we change the name of is_headless_ to ingore_vsync_ or > something like that please? > Done. > Also, we seem to check for is_headless_ only in on place where we call > GetVSyncParameters(), but there is another call to it in RenderContent(), should > we have it in there as well? Added for completeness. It isn't needed in RenderContent() since it doesn't wait for the call to get the vsync parameters.
The CQ bit was checked by dnicoara@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from owenlin@chromium.org Link to the patchset: https://codereview.chromium.org/1060433002/#ps60001 (title: ".")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1060433002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % nit https://chromiumcodereview.appspot.com/1060433002/diff/60001/content/common/g... File content/common/gpu/media/rendering_helper.cc (right): https://chromiumcodereview.appspot.com/1060433002/diff/60001/content/common/g... content/common/gpu/media/rendering_helper.cc:244: // Assume headless mode by default. On ChromeOS this will be set accordingly Please update comment.
https://chromiumcodereview.appspot.com/1060433002/diff/60001/content/common/g... File content/common/gpu/media/rendering_helper.cc (right): https://chromiumcodereview.appspot.com/1060433002/diff/60001/content/common/g... content/common/gpu/media/rendering_helper.cc:244: // Assume headless mode by default. On ChromeOS this will be set accordingly On 2015/04/24 01:08:56, Pawel Osciak wrote: > Please update comment. Done.
The CQ bit was checked by dnicoara@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from owenlin@chromium.org, posciak@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1060433002/#ps80001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1060433002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/42d7f4ea43b42a3bc1405a2b9561e15b3fb37d77 Cr-Commit-Position: refs/heads/master@{#326786} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
