Avoid flushing gfx buffers more frequently than refresh rate (with Cocoa widgets)

VERIFIED FIXED in mozilla1.8alpha4

Status

Core Graveyard
GFX: Mac
VERIFIED FIXED
12 years ago
9 years ago

People

(Reporter: krmathis@gmail.com, Assigned: Mark Mentovai)

Tracking

Trunk
mozilla1.8alpha4
PowerPC
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

12 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050722 Camino/0.9a2+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050722 Camino/0.9a2+

After the bug 280982 checkin my build compiled with 10.4u SDK have became very
slow at redrawing when I type in text boxes on webpages with animated gif images.

Testcase in link above ( from bug 272954 ).

The official build (10.2.8 SDK) and a 10.3.9 SDK build (only sdk difference to
my build) dont have this slowness problem.

Reproducible: Always

Steps to Reproduce:
1. Build Camino with 10.4u SDK.
2. Visit testpage above.
3. Type some text in the text box.

Actual Results:  
Browser slows down. Gif images slowly animates, while it tries to catch up what
I write. Its "beach balling" as well.

Expected Results:  
"Smooth" typing and gif animation (at least on par with the 10.2.8 and 10.3.9
SDK builds)

My .mozconfig: http://homepage.mac.com/krmathis/Trunk/mozconfig.txt
(Assignee)

Comment 1

12 years ago
I see this with the 10.4.0 SDK.  Backing out the patch from bug 280982 doesn't
avoid the issue.  Take a look at your own builds from a few days ago, and you'll
probably find that the testcase was always worse with a 10.4 SDK than the
official builds are.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Slow redrawing when typing in text boxes on pages with animated gif images (10.4u SDK) → SDK >= 10.4: Slow redrawing when typing in text boxes on pages with animated gif images

Comment 2

12 years ago
So they must change something in Cocoa depending on what versions of the libs
you link with. Does Tiger have further NSView drawing optimizations?
(Assignee)

Comment 3

12 years ago
(In reply to comment #2)
> So they must change something in Cocoa depending on what versions of the libs
> you link with.

Wouldn't be the first time.  You most likely won't see this behavior if you
build with a 10.4 SDK and run on 10.3.  If you do, it's a problem with the SDK
itself.

> Does Tiger have further NSView drawing optimizations?

Can we take advantage of the stuff they added to allow drawing into a cache? 
http://developer.apple.com/releasenotes/Cocoa/AppKit.html, sorry, no internal
link, scroll to "NSView Drawing Redirection".

The change that caused this bug SHOULD be mentioned on that page, but a quick
search on the word "linked" doesn't turn up anything that jumps out as the
obvious candidate.

Comment 4

12 years ago
Perhaps you are running into issues with the new coalesced updates behavior:
http://developer.apple.com/documentation/Performance/Conceptual/Drawing/Articles/FlushingContent.html
It's Tiger and Tiger-built only.
(Reporter)

Comment 5

12 years ago
(In reply to comment #1)
You are correct. Even before the 280982 checkin, my 10.4u SDK build are
noticable slower than the official build. Sorry for not giving accurate
information in my first post.

Comment 6

12 years ago
Camino *does* have exceptionally poor typing performance with coalesced updates
enabled for it, so that's a likely explanation for what you are seeing. You
probably just need to throttle back your flushing.

Comment 7

12 years ago
That sounds plausible. So this is heavily dependent on bug 54153.
Assignee: pinkerton → sfraser_bugs
Depends on: 54153
(Assignee)

Comment 8

12 years ago
Created attachment 190269 [details] [diff] [review]
The beginning of a fix

Simon, I'm not gonna let your having taken this bug get in my way!

I'm typing this in Camino, and for the first time in forever, it doesn't
completely suck.  I'm even linked against the 10.4 libraries.

The attached patch is in no way complete.  It's a proof-of-concept type of
thing.

What I'm doing here is rate-limiting back buffer flushes.  If a flush is
requested too soon, a timer is set with a callback that flushes at a more
appropriate time.  I've done this with icky static variables because the caret
doesn't reuse rendering contexts, and because I can get away with it as long as
the caret's the only thing that calls FlushRect and the port stays the same. 
It is and it does.  That doesn't make this any less hacky.  The math is also
hackish if not altogether wrong, and you can still get caret droppings by
moving around with the cursor keys (this may not be a new problem?)  I know
it's flawed, it's just a starting point.

Where to go from here?	I'm envisioning some sort of global flush manager to
replace and rate-limit QDFlushPortBuffer calls.
(Assignee)

Comment 9

12 years ago
Created attachment 190290 [details] [diff] [review]
v0.2

In the previous patch, I was flushing at a maximum of 3Hz because that provided
the only thing resembling a rational blink rate on the test page.  That problem
is purely bug 54153 and causes problems with non-Cocoa widgets too, so I won't
work around it here.  I'm now using a sane, but still hardcoded, 30Hz.	This
may change to be display-dependent based on the guidance in Apple's doc.

Drawing issues were caused by resetting the dirty region instead of adding to
it.  The caret droppings should be gone now.

I also fixed the case that would allow a pair of flushes instead of a single
flush if the timer was set but didn't fire until after a FlushRect call.

And warning cleanup too.  That's always healthy.

This patch should be suitable for testing.  From the user's standpoint, it
should be equivalent to whatever final result is produced here.  From the
developer's standpoint, it's not, but hey, it's Saturday night and I just got
home.
Attachment #190269 - Attachment is obsolete: true
(Reporter)

Comment 10

12 years ago
(In reply to comment #9)
> Created an attachment (id=190290) [edit]
> v0.2

I just compiled a 10.4u SDK build and I must say this is working a LOT better
than it has for a long time. I even think its smoother than the official build...

Ok, I know its not a final solution. But you definatelly seem to be on the right
track. Great!

Comment 11

12 years ago
I just tested this witha custom build and the attached patch make a HUGE
difference on page with a bunc of animations. This is defenitly the way to go.

Comment 12

12 years ago
This is certainly one approach, and if we don't get to fixing bug 54153, is a
good one. Note also that bug 287813 will make this obsolete.
Depends on: 287813
We are going to land 287813 as early as possible in 1.9. So you should do what
you can for 1.8 but don't bother trying to fix it yourself for 1.9.
(Assignee)

Comment 14

12 years ago
Repurposing bug to match what's being done here.  Old summary: "SDK >= 10.4:
Slow redrawing when typing in text boxes on pages with animated gif images"

I'll have a clean patch later or tomorrow.  This will be nice to have for
anything that might like to flush frequently, although it seems that the caret's
the only thing that falls into that category at the moment.
Assignee: sfraser_bugs → mark
Component: General → GFX: Mac
Product: Camino → Core
Summary: SDK >= 10.4: Slow redrawing when typing in text boxes on pages with animated gif images → Avoid flushing gfx buffers more frequently than refresh rate (with Cocoa widgets)
Target Milestone: --- → mozilla1.8alpha4
Version: unspecified → Trunk
(Assignee)

Comment 15

12 years ago
Created attachment 190366 [details] [diff] [review]
Rate-limited QD buffer flushes, v1.0

Here it is.  This improves the snots out of typing performance with
koko-widgets, nicely implemented in a neat little globally-available flush
manager.
Attachment #190290 - Attachment is obsolete: true
Attachment #190366 - Flags: superreview?(sfraser_bugs)
Attachment #190366 - Flags: review?(pinkerton)
(Assignee)

Comment 16

12 years ago
Comment on attachment 190366 [details] [diff] [review]
Rate-limited QD buffer flushes, v1.0

This:
+  if (Hz == 0)
+  {
+    // Initialize the refresh rate.

is a relic and a bad idea.  Initialization won't be conditional on
uninitialized values.
+// Static globally-available flush manager
+NS_EXPORT nsQDFlushManager sQDFlushManager;

rather than exporting the global, you should wrap it in a static function that
returns a ptr to the global. Helps enforce it as a singleton.

+  PRUintn                Hz;                 // Maximum number of updates per
+                                             // second

i'd say add a getter for this to enforce that it's not changable by clients.

+    nsQDFlushPort* port;
+    nsQDFlushPort* next;

don't declare these until you're ready to use them. eg

+    for (port = mPortList ; port ; port = next)

should be |for (nsQDFlushPort* port = mPortList....)|

+  nsQDFlushPort** portPtr;

initialize this to nil for safety...

+  AbsoluteTime now;
+  Nanoseconds elapsed;
+  PRInt64 timeUntilFlush;

same as above, defer declaration until you can initialize.

+  if (!mFlushTimerRunning && timeUntilFlush < 0)
+  {
+    // If past the time for the next acceptable flush, flush now.
+    ::QDFlushPortBuffer(mPort, aRegion);
+    mLastFlushTime = now;
+  }

what happens if the machine goes to sleep while this is happening? will it wake
up and flush a hell of a lot of times, or under-flush? just curious if you've
tested that aspect.

the other question is, does this really need a lock and threadsafety? who else
besides the main thread is going to be doing any drawing? Seems like overhead
that's not needed.
(Assignee)

Comment 18

12 years ago
Created attachment 190757 [details] [diff] [review]
Rate-limited QD buffer flushes, v2

> rather than exporting the global, you should wrap it in a static function
> that returns a ptr to the global. Helps enforce it as a singleton.

How about this?  I've just made the methods and members static.  I'm still
keeping the "s" object around just to get the destructor to fire.  I guess even
that's not necessary since it's at quit time, but I like cleaning up after
myself.  If you think it's better without, I'll get rid of it too.

> i'd say add a getter for this to enforce that it's not changable by clients.

I stuck it in the port object as a static const.

(in CreateOrGet)
> +   nsQDFlushPort** portPtr;
> initialize this to nil for safety...

I'll initialize it to &sPortList instead and take the initialization out of the
for.

> same as above, defer declaration until you can initialize.

Yeah, pre-C99 habit.  I prefer all declarations up top, but I'll go with the
prevailing style.

> what happens if the machine goes to sleep while this is happening? will it
> wake up and flush a hell of a lot of times, or under-flush? just curious if
> you've tested that aspect.

It should flush just right.  ::UpTime stops counting when the machine is
sleeping.  Even if it didn't, with only a single instance per port, we'd see at
most one potentially (but probably not) unnecessary flush.

> the other question is, does this really need a lock and threadsafety? who
> else besides the main thread is going to be doing any drawing? Seems like
> overhead that's not needed.

I tossed that in at the last minute to make it more general.  You're right,
it's not needed here.

Other changes:
Added a TimeUntilFlush method to the port object.  This is used instead of
calculating the time directly in FlushPortBuffer.  The new method also supports
a new check in CreateOrGetPort that will repurpose existing stale port objects
once they're no longer necessary.  Now, in the most common case, the list
should remain only one entry long.  That's nice.

This will now only be part of the build if Cocoa widgets are selected, which
makes it Camino-only and npotdb.  And you know what that means.
Attachment #190366 - Attachment is obsolete: true
Attachment #190757 - Flags: superreview?(sfraser_bugs)
Attachment #190757 - Flags: review?(pinkerton)
(Assignee)

Updated

12 years ago
Attachment #190366 - Flags: superreview?(sfraser_bugs)
Attachment #190366 - Flags: review?(pinkerton)

Comment 19

12 years ago
Comment on attachment 190757 [details] [diff] [review]
Rate-limited QD buffer flushes, v2

Header first...

>--- /dev/null	2005-07-27 16:11:21.000000000 -0400
>+++ mozilla/gfx/src/mac/nsQDFlushManager.h	2005-07-27 16:21:14.000000000 -0400

>+#include <MacTypes.h>
>+#include <Quickdraw.h>

Shouldn't we just be using Carbon/Carbon.h these days?


>+class NS_EXPORT nsQDFlushManager
>+{
>+public:
>+  nsQDFlushManager            ();
>+  ~nsQDFlushManager           ();

This spacing is unconventional. Choose either:

    nsQDFlushManager();
    ~nsQDFlushManager();
    void			Foopy();

or

				nsQDFlushManager();
				~nsQDFlushManager();
    void			Foopy();

>+
>+  static void                 FlushPortBuffer(CGrafPtr, RgnHandle);

I like to see params named in the header too -- "inPort", "inRegion".

>+  class nsQDFlushPort : public nsITimerCallback
>+  {
>+    friend class nsQDFlushManager;
>+
>+  public:
>+                              nsQDFlushPort(CGrafPtr);
>+                              ~nsQDFlushPort();
>+
>+  protected:
>+    void                      Init(CGrafPtr);
>+    void                      Destroy();
>+    void                      FlushPortBuffer(RgnHandle);
>+    PRInt64                   TimeUntilFlush(AbsoluteTime);
>+    NS_DECL_ISUPPORTS
>+    NS_DECL_NSITIMERCALLBACK
>+
>+    // To use the display's refresh rate as the update Hz, see
>+    // http://developer.apple.com/documentation/Performance/Conceptual/Drawing/Articles/FlushingContent.html .
>+    // Here, use a hard-coded 30 Hz instead.
>+    static const PRUintn      Hz = 30;            // Maximum number of updates

The size of PRUintn depends on the compilation options; it's not a good idea
to use this here. PRUint32 will do fine. We also prefer consts to be prepended
by 'k', so kHz, or, better, kRefreshRateHz.

>+    nsQDFlushPort*            mNext;              // Next object in list
>+    CGrafPtr                  mPort;              // Associated port
>+    AbsoluteTime              mLastFlushTime;     // Last QDFlushPortBuffer call
>+    nsCOMPtr<nsITimer>        mFlushTimer;        // Timer for scheduled flush
>+    PRPackedBool              mFlushTimerRunning; // Is it?
>+  };

I'm not a big fan of nested classes; I'd prefer this be declared and
implemented
in the .cpp file, and just forward-declared here. Someone looking at how to use
this thing shouldn't have to wade through implementation classes.


>--- /dev/null	2005-07-27 16:11:21.000000000 -0400
>+++ mozilla/gfx/src/mac/nsQDFlushManager.cpp	2005-07-27 16:21:43.000000000 -0400

>+// nsQDFlushManager
>+
>+// This sets up a dummy object so the destructor will fire.
>+static nsQDFlushManager sQDFlushManager;

Eh? Is this to prevent dead-stripping or something?

>+nsQDFlushManager::~nsQDFlushManager()
>+{
>+  nsQDFlushPort* next;
>+
>+  for (nsQDFlushPort* port = sPortList ; port ; port = next)
>+  {
>+    next = port->mNext;
>+    port->Destroy();
>+    NS_RELEASE(port);

I find while loops easier to grok:

    nsQDFlushPort* port = sPortList;
    while (port)
    {
	nsQDFlushPort* next = port->mNext;
	port->Destroy();
	NS_RELEASE(port);
	port = next;
    }


>+// CreateOrGetPort(aPort)
>+//
>+// Walks through the list of port objects and returns the one corresponding to
>+// aPort if it exists.  If it doesn't exist, the port object is created, added
>+// to the list, and returned.

Say when the port object will be destroyed.

>+//
>+// protected static
>+nsQDFlushManager::nsQDFlushPort*
>+nsQDFlushManager::CreateOrGetPort(CGrafPtr aPort)
>+{
>+  AbsoluteTime now = ::UpTime();
>+  nsQDFlushPort** portPtr = &sPortList;
>+
>+  // Don't be frightened.  The pointer-pointer business isn't so confusing,
>+  // and it eases maintenance of the linked list.
>+  // First, go through the list and see if an object is already associated
>+  // with this port.
>+  for ( ; *portPtr ; portPtr = &((*portPtr)->mNext))
>+  {
>+    if (aPort == (*portPtr)->mPort)
>+    {
>+      return *portPtr;
>+    }
>+  }

Again, i think this would be clearer with a while loop (and without the weird
double deref):

    nsQDFlushPort* port = sPortList;
    while (port)
    {
	if (aPort == port->mPort)
	    return port;
	port = port->mNext;
    }

>+  // If no port object exists yet, try to find an object that's not in use.
>+  // If there is one, repurpose it.
>+  for (portPtr = &sPortList ; *portPtr ; portPtr = &((*portPtr)->mNext))
>+  {
>+    if (!(*portPtr)->mFlushTimerRunning && (*portPtr)->TimeUntilFlush(now) < 0)
>+    {
>+      // If the flush timer is not running and it's past the time during which
>+      // a flush would be postponed, the object is no longer needed.  Future
>+      // flushes for (*portPtr)->mPort would occur immediately.  Since there's
>+      // no longer any state to track, the object can be reused for another
>+      // port.  This keeps the size of the list manageable.
>+      (*portPtr)->Init(aPort);
>+      return *portPtr;
>+    }
>+  }

Another thing more clearly written with a while loop, and without the extra
deref.

>+  *portPtr = new nsQDFlushPort(aPort);
>+  NS_ADDREF(*portPtr);
>+  return *portPtr;
>+}

>+
>+// RemovePort(aPort)
>+//
>+// Walks through the list of port objects and removes the one corresponding to
>+// aPort, if it exists.
>+//
>+// Presently unused.
>+//
>+// protected static
>+void
>+nsQDFlushManager::RemovePort(CGrafPtr aPort)
>+{
>+  // Traversal is as in CreateOrGetPort.
>+  for (nsQDFlushPort** portPtr = &sPortList ; *portPtr ;
>+       portPtr = &((*portPtr)->mNext))
>+  {
>+    if (aPort == (*portPtr)->mPort)
>+    {
>+      nsQDFlushPort* next = (*portPtr)->mNext;
>+      (*portPtr)->Destroy();
>+      NS_RELEASE(*portPtr);
>+      *portPtr = next;
>+      return;
>+    }
>+  }

Isn't this broken? You have to hook up mNext of the previous port object in the
list to the next one, no?

>+nsQDFlushManager::nsQDFlushPort::nsQDFlushPort(CGrafPtr aPort)
>+: mNext(nsnull)
>+, mPort(aPort)
>+, mLastFlushTime((AbsoluteTime){0, 0})
>+, mFlushTimer(nsnull)
>+, mFlushTimerRunning(PR_FALSE)
>+{
>+}
>+
>+nsQDFlushManager::nsQDFlushPort::~nsQDFlushPort()
>+{
>+  // Everything should have been taken care of by Destroy().
>+}
>+
>+// Init(aPort)
>+//
>+// (Re)initialize object.
>+//
>+// protected
>+void
>+nsQDFlushManager::nsQDFlushPort::Init(CGrafPtr aPort)
>+{
>+  mPort = aPort;
>+  return;
>+}

No need for 'return'.

>+// FlushPortBuffer(aRegion)
>+//
>+// Flushes, dirties, and schedules, as appropriate.  Public access is from
>+// nsQDFlushManager::FlushPortBuffer(CGrafPtr aPort, RgnHandle aRegion).
>+//
>+// protected
>+void
>+nsQDFlushManager::nsQDFlushPort::FlushPortBuffer(RgnHandle aRegion)
>+{
>+  AbsoluteTime now = ::UpTime();
>+  PRInt64 timeUntilFlush = TimeUntilFlush(now);
>+
>+  if (!mFlushTimerRunning && timeUntilFlush < 0)
>+  {
>+    // If past the time for the next acceptable flush, flush now.
>+    ::QDFlushPortBuffer(mPort, aRegion);
>+    mLastFlushTime = now;
>+  }
>+  else
>+  {
>+    // If it's not time for the next flush yet, or if the timer is running
>+    // indicating that an update is pending, just mark the dirty region.
>+    ::QDAddRegionToDirtyRegion(mPort, aRegion);

How does QDAddRegionToDirtyRegion() interplay with QDFlushPortBuffer()?
Have you checked to see that we're just flushging the union of the flush
regions, and not the entire port?

>+
>+    if (!mFlushTimerRunning)
>+    {
>+      // No flush scheduled?  No problem.
>+      if (!mFlushTimer)
>+      {
>+        // No timer object?  No problem.
>+        nsresult err;
>+        mFlushTimer = do_CreateInstance("@mozilla.org/timer;1", &err);
>+        NS_ASSERTION(NS_SUCCEEDED(err), "Could not instantiate flush timer.");
>+      }
>+      if (mFlushTimer)
>+      {
>+        // Start the clock, with the timer firing at the already-calculated
>+        // time until the next flush.  Nanoseconds (1E-9) were used above,
>+        // but nsITimer is big on milliseconds (1E-3), so divide by 1E6.
>+        // Any time that was consumed between the ::UpTime call and now
>+        // will be lost.  That's not so bad in the usual case, it's a tiny
>+        // bit less not so bad if a timer object didn't exist yet and was
>+        // created.  It's better to update slightly less frequently than
>+        // the target than slightly more frequently.
>+        mFlushTimer->InitWithCallback(this, (PRUint32)(timeUntilFlush/1E6),
>+                                      nsITimer::TYPE_ONE_SHOT);
>+        mFlushTimerRunning = PR_TRUE;
>+      }
>+    }
>+  }
>+  return;

Nuke return.


>Index: mozilla/gfx/src/mac/nsRenderingContextMac.cpp
>===================================================================

>-    RgnHandle rgn = ::NewRgn();
>+    RgnHandle rgn = sNativeRegionPool.GetNewRegion();

StRegionFromPool ?
Attachment #190757 - Flags: superreview?(sfraser_bugs) → superreview-
(Assignee)

Comment 20

12 years ago
(In reply to comment #19)
> The size of PRUintn depends on the compilation options; it's not a good idea
> to use this here. PRUint32 will do fine. We also prefer consts to be prepended
> by 'k', so kHz, or, better, kRefreshRateHz.

Yeah, I didn't want to use kHz for obvious reasons.  I'll be verbose to work the
k in.

> >+static nsQDFlushManager sQDFlushManager;
> 
> Eh? Is this to prevent dead-stripping or something?

It makes sure the destructor fires while avoiding dead-stripping problems.  Like
I said, the destructor's not really strictly necessary if everything's static
anyway.

> Again, i think this would be clearer with a while loop (and without the weird
> double deref):

(in CreateOrGetPort:)
I can get rid of the pointer-to-pointer in the first loop for sure.  It's
vestigal.  I'm more attached to it in the second pass, though, so that portPtr
points to either the last mNext or, if none, sPortList.  That lets me just stuff
the new object into *portPtr.  I find the alternative to pointer fun ugly and
unnecessary:

  if (sPortList == nsnull)
    sPortList = the_new_object;
  else
    port->mNext = the_new_object;

> Isn't this broken? You have to hook up mNext of the previous port object in 
> the list to the next one, no?

(in RemovePort:)
As above, *portPtr is either sPortList if removing the first or mNext of the
previous if removing anything else.  In this case, the alternative is even uglier.

> How does QDAddRegionToDirtyRegion() interplay with QDFlushPortBuffer()?
> Have you checked to see that we're just flushging the union of the flush
> regions, and not the entire port?

The documented behavior is that QDFlushPortBuffer() flushes only the dirty
region which I'm setting with QDAddRegionToDirtyRegion() plus whatever region is
passed directly to QDFlushPortBuffer().  If that directly-passed region is NULL,
only the dirty region is flushed.

> >-    RgnHandle rgn = ::NewRgn();
> >+    RgnHandle rgn = sNativeRegionPool.GetNewRegion();
> 
> StRegionFromPool ?

(in nsRenderingContextMac.cpp:)
OK, and I'll clean up other occurrences in that file too.
(Assignee)

Comment 21

12 years ago
Is there a preferred style to use in preference to static ctors and dtors?

I'm thinking of taking all of the static methods and members in nsQDFlushManager
back to non-static and providing a static nsQDFlushManager *sQDFlushManager in
the class, along with a static sFlushPortBuffer that instantiates
sQDFlushManager if necessary and calls sQDFlushManager->FlushPortBuffer().  The
dummy object to get the dtor firing is stupid, and I'd rather not leak.
(Assignee)

Comment 22

12 years ago
Created attachment 190997 [details] [diff] [review]
Rate-limited QD buffer flushes, v3

Style and readability changes made.  Things that were brought up that I haven't
already commented on are integrated as-is.  I'm still doing pointer-pointer
where it's advantageous to do so, but it should be much more readable now
because I've added explicit shadow derefs and comments that should do a better
job of explaining what portPtr is about.  I've modified the manager class to be
non-static while carrying a static member and associated static flush method as
proposed in comment 21.  The static method is responsible for instantiation,
and arranges for destruction at shutdown via a shutdown observer.  This is all
explained in the comments.
Attachment #190757 - Attachment is obsolete: true
Attachment #190997 - Flags: superreview?(sfraser_bugs)
Attachment #190997 - Flags: review?(pinkerton)
(Assignee)

Updated

12 years ago
Attachment #190757 - Flags: review?(pinkerton)
(Assignee)

Comment 23

12 years ago
Needs #include "nsCRT.h" (but only when building debug?!)
(Reporter)

Comment 24

12 years ago
Mark, you might want to take a look at bug 302826
Starting yesterday it caused a failed build on my 10.4.2, GCC 4.0.0, 10.4u SDK
setup.

After I backed out the patch from my source it compiled successfully!
(Assignee)

Comment 25

12 years ago
Kai, I can't reproduce what was bug 302826.  None of the ld output you pasted
there is an error, there are only warnings.  If you're still experiencing
trouble, reopen that bug and give me a more complete dump of the final g++ -o
mozilla-bin that fails.
(Assignee)

Comment 26

12 years ago
Sorry, I missed the error there because the bug summary was misleading.  Still,
I can't reproduce.  I changed some things between v2 and v3 of the patch that
might cause this error if you don't cleanly and properly back out v2 before
applying v3.  Try pulling a fresh source tree in at least mozilla/gfx/src/mac
and reapplying the most recent version of the patch.
(Assignee)

Comment 27

12 years ago
Created attachment 191418 [details] [diff] [review]
Rate-limited QD buffer flushes, v3, includes Makefile patch

The problem Kai was seeing was due to my having not included the Makefile diff
in 301774.5.  This version corrects that omission, but is otherwise identical.
(Assignee)

Updated

12 years ago
Attachment #190997 - Attachment is obsolete: true
Attachment #191418 - Flags: superreview?(sfraser_bugs)
Attachment #191418 - Flags: review?(pinkerton)
(Assignee)

Updated

12 years ago
Attachment #190997 - Flags: superreview?(sfraser_bugs)
Attachment #190997 - Flags: review?(pinkerton)

Updated

12 years ago
Attachment #191418 - Flags: superreview?(sfraser_bugs) → superreview+
Comment on attachment 191418 [details] [diff] [review]
Rate-limited QD buffer flushes, v3, includes Makefile patch

r=pink
Attachment #191418 - Flags: review?(pinkerton) → review+
(Reporter)

Comment 29

12 years ago
(In reply to comment #27)

Thanks Mark!
After I used the 301774.6.patch Camino built successfully again.
(Assignee)

Comment 30

12 years ago
Checked in.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Reporter)

Updated

12 years ago
Status: RESOLVED → VERIFIED
(Assignee)

Updated

12 years ago
Depends on: 311618
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.