Closed Bug 937204 Opened 6 years ago Closed 6 years ago

Remove the java GfxInfoThread that was used to gather OpenGL info on startup

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: bjacob, Assigned: bjacob)

References

Details

(Whiteboard: [qa-])

Attachments

(5 files, 3 obsolete files)

Attached patch Kill GfxInfoThread (obsolete) — Splinter Review
This was introduced in bug 766251. The idea was that we would probably need to have to evaluate blacklist rules very early, probably earlier than compositor startup. However:
 - we long gave up the notion of running on Android without GL compositing, which means that if GL layers are blacklisted, we won't run anyway.
 - another way that this could have been useful is if we had to create the compositor's GL context differently depending on blacklist rules. For example, at the time, we were using GL share groups to implement texture sharing across GL contexts (which we'd need for e.g. fast canvas compositing). That can have to be determined before creating GL contexts. But it later turned out that share groups were a bad idea anyway, and that we didn't need them to share just textures.

So that GfxInfoThread wasn't actually useful, and meanwhile, it made other code like GLController.java more complicated.

This patch removes GfxInfoThread; instead, GL strings are provided to GfxInfo by the first OpenGL context that we create, which will typically be the compositor's GL context. Special attention needs to be paid to thread-safety as this will typically happen off the main thread, while GfxInfo could typically be queried concurrently on the main thread.

R?:
 - kats for the java changes
 - jrmuizel for the GfxInfo/GLContext changes
Attachment #830286 - Flags: review?(jmuizelaar)
Attachment #830286 - Flags: review?(bugmail.mozilla)
Oh, what has happened in this patch... AndroidBridge is a huge diff for only a few trivial changes... how can I tell hg not to be stupid?
Comment on attachment 830286 [details] [diff] [review]
Kill GfxInfoThread

Review of attachment 830286 [details] [diff] [review]:
-----------------------------------------------------------------

I would rather we not have locking in GfxInfo.cpp. Can we use a condition variable to track when we have the information and block until then.

::: gfx/gl/GLContext.cpp
@@ +1107,5 @@
> +    if (++count == 1) {
> +      nsCOMPtr<nsIGfxInfo> gfxInfo = do_GetService("@mozilla.org/gfx/info;1");
> +      static_cast<widget::GfxInfo*>(gfxInfo.get())->GrabGLStrings(this);
> +    }
> +#endif

This should not happen automatically and should happen with a particular context.
Attachment #830286 - Flags: review?(jmuizelaar) → review-
Comment on attachment 830286 [details] [diff] [review]
Kill GfxInfoThread

Review of attachment 830286 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/gfx/GLController.java
@@ -139,5 @@
> -                // If we haven't yet created the compositor, and the GfxInfoThread
> -                // isn't done it's data gathering activities, then postpone creating
> -                // the compositor a little bit more. Don't block though, since this is
> -                // the UI thread we're running on.
> -                if (!mCompositorCreated && !GfxInfoThread.hasData()) {

Do we still need the mCompositorCreated check here? I think as part of this patch it probably needs to still stay here but maybe your future patches get rid of it?

::: widget/android/GfxInfo.cpp
@@ +482,5 @@
>  
>  /* void spoofVendorID (in DOMString aVendorID); */
>  NS_IMETHODIMP GfxInfo::SpoofVendorID(const nsAString & aVendorID)
>  {
> +  EnsureInitialized(); // initialization from GfxInfo data overwrites mVendor

Does the comment need updating? Ditto for the next few hunks
Attachment #830286 - Flags: review?(bugmail.mozilla)
You see what happens when you r- a patch from me? Boom, you get THREE patches to review.

This is needed here because while this d3d9 was trivially failing outside of Windows, it was deadlocking with GfxInfo initialization, as it was run before we got a compositor, which would provide GL strings to GfxInfo. In the main patch that follows here, I special-cased OPENGL_LAYERS as always succeeding on Android so that at least that doesn't deadlock, but I didn't want to special case all the crazy features that don't matter on Android.
Attachment #831242 - Flags: review?(jmuizelaar)
Sad consequence: had to drop some const-qualification. The GfxInfoBase-inherited getters aren't const-qualified anyway.

The bug that this fixes is that otherwise, with the next patch, about:support misses some fields the first time it's loaded.
Attachment #831243 - Flags: review?(jmuizelaar)
I still amn't sure what's going on with GeneratedJNIWrappers but 'Generated' sounds like I don't need to worry.
Attachment #831245 - Flags: review?(jmuizelaar)
Attachment #831245 - Flags: review?(bugmail.mozilla)
Attachment #830286 - Flags: review-
Attachment #831245 - Attachment is obsolete: true
Attachment #831245 - Flags: review?(jmuizelaar)
Attachment #831245 - Flags: review?(bugmail.mozilla)
Attachment #831246 - Flags: review?(jmuizelaar)
Attachment #831246 - Flags: review?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> Comment on attachment 830286 [details] [diff] [review]
> Kill GfxInfoThread
> 
> Review of attachment 830286 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/gfx/GLController.java
> @@ -139,5 @@
> > -                // If we haven't yet created the compositor, and the GfxInfoThread
> > -                // isn't done it's data gathering activities, then postpone creating
> > -                // the compositor a little bit more. Don't block though, since this is
> > -                // the UI thread we're running on.
> > -                if (!mCompositorCreated && !GfxInfoThread.hasData()) {
> 
> Do we still need the mCompositorCreated check here? I think as part of this
> patch it probably needs to still stay here but maybe your future patches get
> rid of it?

Things seem to be running fine without it (Nexus 10, Android 4.3) and yes the patches on bug 925608 would remove that anyway.

> 
> ::: widget/android/GfxInfo.cpp
> @@ +482,5 @@
> >  
> >  /* void spoofVendorID (in DOMString aVendorID); */
> >  NS_IMETHODIMP GfxInfo::SpoofVendorID(const nsAString & aVendorID)
> >  {
> > +  EnsureInitialized(); // initialization from GfxInfo data overwrites mVendor
> 
> Does the comment need updating? Ditto for the next few hunks

Removed these comments, they weren't useful anyway.
Attachment #830286 - Attachment is obsolete: true
Attachment #831246 - Flags: review?(bugmail.mozilla) → review+
Attachment #831243 - Flags: review?(jmuizelaar) → review+
Attachment #831242 - Flags: review?(jmuizelaar) → review+
Comment on attachment 831246 [details] [diff] [review]
3/3. Kill GfxInfoThread (now with a Monitor)

Review of attachment 831246 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/android/GfxInfo.h
@@ +87,5 @@
>    nsCString mVendor;
>    nsCString mRenderer;
>    nsCString mVersion;
> +  bool mHaveRecordedGLStrings;
> +  Monitor mGLStringsMonitor;

It would be nice we could encapsulate the shared information in it's own struct.
Now with the gl strings and monitor stuff taken to a separate nested class.
Attachment #831713 - Flags: review?(jmuizelaar)
Attachment #831246 - Flags: review?(jmuizelaar)
Comment on attachment 831713 [details] [diff] [review]
3/3. Kill GfxInfoThread (now with a Monitor) (v2)

Review of attachment 831713 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/android/GfxInfo.cpp
@@ +26,5 @@
> +  nsCString mVendor;
> +  nsCString mRenderer;
> +  nsCString mVersion;
> +  bool mReady;
> +  Monitor mMonitor;

You should add a bit of documentation about how this class is used.
Attachment #831713 - Flags: review?(jmuizelaar) → review+
Depends on: 938393
Oh, and I know that 4/3 may seem surprising... but that's very well explained by Marius:

 http://www.youtube.com/watch?v=oZZJFfk_Oh8

 " - You put one third Curaçao... but that's a very little third. Then one third lemon, and one big third Picon, and one very big third water."
 " - But that's four thirds?!"
 " - Dummy! It all depends on the size of each third!"
Attachment #831869 - Flags: review?(ehsan)
Comment on attachment 831869 [details] [diff] [review]
4/3. Avoid stalling in xpcshell or anywhere we don't have a WindowCreator

Review of attachment 831869 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/android/GfxInfo.cpp
@@ +30,5 @@
>    nsCString mVersion;
>    bool mReady;
>    Monitor mMonitor;
>  
> +  static bool ExpectToEverGetInitialized();

I don't think this method should be a member of this class.  Just make it a static global function please.

@@ +125,5 @@
> +  if (!wwatch) {
> +    return false;
> +  }
> +  bool hasWindowCreator = false;
> +  nsresult rv = wwatch->HasWindowCreator(&hasWindowCreator);

Where is HasWindowCreator defined?  I can't find it in this patch.
Attachment #831869 - Flags: review?(ehsan) → review-
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #14)
> Comment on attachment 831869 [details] [diff] [review]
> 4/3. Avoid stalling in xpcshell or anywhere we don't have a WindowCreator
> 
> Review of attachment 831869 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/android/GfxInfo.cpp
> @@ +30,5 @@
> >    nsCString mVersion;
> >    bool mReady;
> >    Monitor mMonitor;
> >  
> > +  static bool ExpectToEverGetInitialized();
> 
> I don't think this method should be a member of this class.  Just make it a
> static global function please.

OK, new patch coming.

> 
> @@ +125,5 @@
> > +  if (!wwatch) {
> > +    return false;
> > +  }
> > +  bool hasWindowCreator = false;
> > +  nsresult rv = wwatch->HasWindowCreator(&hasWindowCreator);
> 
> Where is HasWindowCreator defined?  I can't find it in this patch.

In the patch on the blocker bug 938393 which I'm getting Benjamin to review.
Comment on attachment 832302 [details] [diff] [review]
4/3. Avoid stalling in xpcshell or anywhere we don't have a WindowCreator

Review of attachment 832302 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/android/GfxInfo.cpp
@@ +23,5 @@
>  
>  namespace mozilla {
>  namespace widget {
>  
> +static bool ExpectGLStringsToEverGetInitialized() {

Nit: brace on a new line, please.
Attachment #832302 - Flags: review?(ehsan) → review+
Assignee: nobody → bjacob
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.