Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@Adlai-Holler
Copy link
Contributor

@Adlai-Holler Adlai-Holler commented Jul 22, 2020

This is part of a larger effort to expose the difference between GrDirectContext, which runs on the GPU thread and can directly perform operations like uploading textures, and GrRecordingContext, which can only queue up work to be delivered to the GrDirectContext later.

Over time, GrContext will be replaced by:

  • GrDirectContext in cases where we know that we have the GPU access / with functions that require it.
  • GrRecordingContext in cases where we aren't sure / with functions that do not need to perform such actions.
    • In practice, since Flutter plumbs the context around on its own (say, instead of using SkCanvas->recordingContext,) we pretty much have the direct context handy at all times.

Rough outline of what's going on is here. The key is SkDeferredDisplayLists – when dealing with them, which Flutter does not presently, the GrContext is actually incapable of talking to the GPU because it's not on the GPU's thread. Right now, the internal GrDDLContext you get just stubs off functionality that it can't perform, but this won't scale – we need to reveal this distinction to users so they can be clear about what's allowed and what's not.

@auto-assign auto-assign bot requested a review from jason-simmons July 22, 2020 21:04
@Adlai-Holler Adlai-Holler force-pushed the direct_context_factory branch 2 times, most recently from f77d83a to 7443b60 Compare July 23, 2020 16:05
@Adlai-Holler Adlai-Holler marked this pull request as draft July 23, 2020 16:25
@Adlai-Holler Adlai-Holler force-pushed the direct_context_factory branch 3 times, most recently from 61e43f6 to 65c67a6 Compare July 24, 2020 15:17
@Adlai-Holler
Copy link
Contributor Author

Adlai-Holler commented Jul 24, 2020

The remaining failures seem like a tooling issue? <vulkan.h> file not found.

@jason-simmons @gaaclarke thoughts on this? It's a big CL and I'm happy to talk more about the reason for it, but we'll have to do it eventually so we might as well rip the bandaid off. There's other API changes coming that are currently blocked by this (for instance, SkImage::MakeCrossContextFromPixmap should take a GrDirectContext argument instead of a GrContext one.

@Adlai-Holler Adlai-Holler marked this pull request as ready for review July 24, 2020 15:31
@auto-assign auto-assign bot requested a review from gaaclarke July 24, 2020 15:33
@jason-simmons
Copy link
Member

@chinmaygarde

@Adlai-Holler
Copy link
Contributor Author

Hi Chinmay! It's been about 6 years, I think!

@gaaclarke
Copy link
Member

Hey @Adlai-Holler , can you link to the documentation about the deprecation of GrContext in the description of the issue?

As for the vulkan.h not found problem. I'm not sure what the problem is there. It's probably best to file an issue about it and add // FLUTTER_NOLINT at the top of the file (directly below the copyright) for now until it can be debugged.

@Adlai-Holler
Copy link
Contributor Author

Thanks for the reply! I've added the NOLINT. Where should I file an issue? This repo doesn't seem to support them.

Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! small world.

Thanks for the background. This should only affect Flutter once it starts building its own DDLs.

@chinmaygarde
Copy link
Member

Where should I file an issue?

https://github.com/flutter/flutter/issues/new

@Adlai-Holler
Copy link
Contributor Author

Issue filed at flutter/flutter#62222

Looks like we have only infra failures remaining on the CI.

@gaaclarke
Copy link
Member

gaaclarke commented Jul 24, 2020

@Adlai-Holler I had those problems with CI yesterday. I haven't been able to verify if it is fixed now but rebasing your PR onto master should fix it.

edit: Don't even worry about it. Once this gets through review we can rebase before we land.

This is part of a larger effort to expose the difference between GrDirectContext,
which runs on the GPU thread and can directly perform operations like uploading
textures, and GrRecordingContext, which can only queue up work to be delivered
to the GrDirectContext later.
@Adlai-Holler
Copy link
Contributor Author

Rebased

@Adlai-Holler Adlai-Holler force-pushed the direct_context_factory branch from 82806bf to 73d37d1 Compare July 27, 2020 14:11
@Adlai-Holler
Copy link
Contributor Author

build_and_test_linux_unopt_debug failure seems like an infra flake

@Adlai-Holler
Copy link
Contributor Author

@gaaclarke @chinmaygarde Any chance we can get this in the bank today?

@chinmaygarde
Copy link
Member

The web stuff is unrelated and it seems to be referencing the framework which was broken at the time of the presubmit run. I am landing this now.

@chinmaygarde chinmaygarde merged commit c57aff1 into flutter:master Jul 28, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 28, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 29, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 29, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 29, 2020
@Adlai-Holler Adlai-Holler deleted the direct_context_factory branch July 29, 2020 15:57
pull bot pushed a commit to Mu-L/skia that referenced this pull request Jul 29, 2020
More recontexting for SkImage. Chrome flag in CL 2323135.
Flutter migration landed in flutter/engine#19962

Bug: skia:104662
Change-Id: Id725eb130310639457ba90f378ecdb334dd5f3cd
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/306182
Auto-Submit: Adlai Holler <[email protected]>
Commit-Queue: Brian Salomon <[email protected]>
Reviewed-by: Brian Salomon <[email protected]>
Adlai-Holler added a commit to Adlai-Holler/engine that referenced this pull request Aug 4, 2020
This is a followup to flutter#19962 to cover a few places where we
were still using GrContext. No functional impact.
jason-simmons pushed a commit that referenced this pull request Aug 5, 2020
* Migrate a few last places to GrDirectContext

This is a followup to #19962 to cover a few places where we
were still using GrContext. No functional impact.

* Formatting
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants