Given the crash resolved in part 1, it is possible for the blob
rasterizer in the compositor process to still be using surfaces after
the animation has advanced to the next frame. With recycling this can be
problematic as the recycled surface will be reused for a future frame.
In an ideal world, the blob recording would use the animation's image
key instead, but the rasterizer doesn't have easy access to the mapping
table. As such, for any frames used in a blob recording, we now
explicitly mark them as non-recyclable and we will be forced to allocate
a new frame instead.
Differential Revision: https://phabricator.services.mozilla.com/D16192
Beyond the necessary reinitialization methods, we need to protect
ourselves from recycling a frame that some other entity in the browser
is still using. Generally speaking the animated surface will only be
used in imgFrame::Draw since we don't layerize animated images, which
will be safe. However with OMTP or blob recordings, we could retain a
reference to the surface outside the current stack context. Additional
if something calls RasterImage::GetImageContainer(AtSize) or
RasterImage::GetFrame(AtSize), it may also have a reference to the
surface for an indetermine period of time.
As such, if an imgFrame is a candidate for recycling, it will wrap
imgFrame::mLockedSurface in a RecyclingSourceSurface. Its job is to
track how many consumers there are still of the surface, so that after
we advance the animation, the decoder will know if there are still
outstanding consumers.
If the surface is still in use, it will block for a finite period of
time (the refresh interval) before giving up on reclaiming the surface,
and will allocate a new surface. The old surface can then remain in
circulation for as long as necessary without further blocking the
animation progression, since we stop recycling that surface/imgFrame.
Differential Revision: https://phabricator.services.mozilla.com/D7511
Since imgFrame::Draw will limit the drawing to only look at pixels that
we have written to and posted an invalidation for, there is no need to
hold the monitor while doing so. By taking the most expensive operation
outside the lock, we will minimize our chances of hitting contention
with the decoder thread.
A later part in this series will require that a surface be freed outside
the lock because it may end up reacquiring it. In addition to the
contention win, this change should facilitate that.
Differential Revision: https://phabricator.services.mozilla.com/D7510
At present, surface providers roll up all of their individual surfaces
into a single reporting unit. Specifically this means animated image
frames are all reported as a block. This patch removes that
consolidation and reports every frame as its own SurfaceMemoryReport.
This is important because each frame may have its own external image ID,
and we want to cross reference that with what we expect from the GPU
shared surfaces cache.
At present, surface providers roll up all of their individual surfaces
into a single reporting unit. Specifically this means animated image
frames are all reported as a block. This patch removes that
consolidation and reports every frame as its own SurfaceMemoryReport.
This is important because each frame may have its own external image ID,
and we want to cross reference that with what we expect from the GPU
shared surfaces cache.
DecoderFlags::BLEND_ANIMATION will cause the decoder to inject the
BlendAnimationFilter from the previous patch into the SurfacePipe filter
chain. All frames produced by this decoder will be complete, and
should be equivalent to the result outputted by FrameAnimator.
DecoderFlags::BLEND_ANIMATION will cause the decoder to inject the
BlendAnimationFilter from the previous patch into the SurfacePipe filter
chain. All frames produced by this decoder will be complete, and
should be equivalent to the result outputted by FrameAnimator.
RawAccessFrameRef ensures there is a valid data pointer to the pixel
data for the frame. It is a common pattern for users of
RawAccessFrameRef to follow up with a request for the data pointer
shortly after creation. We can avoid an extra lock by exposing this data
pointer from RawAccessFrameRef, and populating it via
imgFrame::LockImageData.
We currently choose to set the animation parameters (blend method, blend
rect, disposal method, timeout) in imgFrame::Finish instead of
imgFrame::InitForDecoder. The decoders themselves already have access to
the necessary information at the time InitForDecoder is called, so there
is no reason to do this. Moving the configuration to initialization will
allow us to relax the mutex protection on these parameters.
This part simply reorganizes imgFrame, and subsequent parts will
introduce the necessary changes to SurfacePipe and decoders.
The shared memory handle reporting has been generalized to be an
external handle reporting. This is used for both shared memory, and for
volatile memory (on Android.) This will allow us to have a better sense
of just how many handles are being used by images on Android.
Additionally we were not properly reporting forced heap allocated
memory, if we were putting animated frames on the heap. This is because
we used SourceSurfaceAlignedRawData without implementing
AddSizeOfExcludingThis.
image.mem.volatile.min_threshold_kb is the minimum buffer allocation for
an image frame in KB before it will use volatile memory. If it is less
than it will use the heap. This only is set to > 0 on Android.
image.mem.animated.use_heap forces image frames to use the heap if it is
for an animated image. This is only enabled for Android, and was
previously a compile time option also for Android.
Move the initialization of SharedSurfacesParent from the compositor
thread creation to mirror the other WebRender-specific components, such
as the render thread creation. Now it will only be created if WebRender
is in use. Also prevent shared surfaces from being used by the image
frame allocator, even if image.mem.shared is set -- there is no purpose
in allowing this at present. It was causing startup crashes for users
who requested image.mem.shared and/or WebRender via gfx.webrender.all
but did not actually get WebRender at all. Surfaces would get allocated
in the shared memory, try to register themselves with the WR render
thread, and then crash since that thread was never created.
Move the initialization of SharedSurfacesParent from the compositor
thread creation to mirror the other WebRender-specific components, such
as the render thread creation. Now it will only be created if WebRender
is in use. Also prevent shared surfaces from being used by the image
frame allocator, even if image.mem.shared is set -- there is no purpose
in allowing this at present. It was causing startup crashes for users
who requested image.mem.shared and/or WebRender via gfx.webrender.all
but did not actually get WebRender at all. Surfaces would get allocated
in the shared memory, try to register themselves with the WR render
thread, and then crash since that thread was never created.
Factory::DoesBackendSupportDataDrawtarget already fulfills the same
purpose and we should use that instead, as imgFrame is the only user of
the former API. It has the added bonus of allowing us to use shared
surfaces on Linux with WebRender, and using volatile surfaces on Windows
when D2D is disabled.
In bug 1383499 we fixed the case where on Android, animated images could
consume all of the available file handles. This is because each volatile
buffer will contain a file handle on Android, and animated images can
contain many, many frames. However in doing so we introduced a bug where
the stride of replacement surface was aligned to a 16-byte boundary. We
do not currently support any stride value but pixel size * image width
in the image decoding framework. This may be something we correct in the
future but for now, we should just ensure all surfaces follow the
expected stride value.
Animated image frames are never actually released to let the OS purge
them, because AnimationSurfaceProvider keeps them as RawAccessFrameRef.
Meanwhile, each volatile buffer on Android needs a file handle to be
retained for as long as the buffer lives. Given sufficient number of
animated image frames, we could easily exhaust the available file
handles (which in turn causes many problems). As such on Android we
should stick with the heap for animated image frames. On other platforms
it is better to stay because we will avoid a memset, because the OS will
zero-fill the requested pages on our behalf.
Most of the changes in this patch are just using the explicit
constructor from gfx::IntSize to gfx::Size, since gfxSize did
that implicitly but gfx::Size doesn't.
MozReview-Commit-ID: CzikGjHEXje
--HG--
extra : rebase_source : 9d19977f2a774d9a2a653db923553a6c2e06f82a
This patch makes the following changes to the macros.
- Removes PROFILER_LABEL_FUNC. It's only suitable for use in functions outside
classes, due to PROFILER_FUNCTION_NAME not getting class names, and it was
mostly misused.
- Removes PROFILER_FUNCTION_NAME. It's no longer used, and __func__ is
universally available now anyway.
- Combines the first two string literal arguments of PROFILER_LABEL and
PROFILER_LABEL_DYNAMIC into a single argument. There was no good reason for
them to be separate, and it forced a '::' in the label, which isn't always
appropriate. Also, the meaning of the "name_space" argument was interpreted
in an interesting variety of ways.
- Adds an "AUTO_" prefix to PROFILER_LABEL and PROFILER_LABEL_DYNAMIC, to make
it clearer they construct RAII objects rather than just being function calls.
(I myself have screwed up the scoping because of this in the past.)
- Fills in the 'js::ProfileEntry::Category::' qualifier within the macro, so
the caller doesn't need to. This makes a *lot* more of the uses fit onto a
single line.
The patch also makes the following changes to the macro uses (beyond those
required by the changes described above).
- Fixes a bunch of labels that had gotten out of sync with the name of the
class and/or function that encloses them.
- Removes a useless PROFILER_LABEL use within a trivial scope in
EventStateManager::DispatchMouseOrPointerEvent(). It clearly wasn't serving
any useful purpose. It also serves as extra evidence that the AUTO_ prefix is
a good idea.
- Tweaks DecodePool::SyncRunIf{Preferred,Possible} so that the labelling is
done within them, instead of at their callsites, because that's a more
standard way of doing things.
--HG--
extra : rebase_source : 318d1bc6fc1425a94aacbf489dd46e4f83211de4
Andrew Osmond
Sylvestre Ledru
Narcis Beleuzu
Andreea Pavel
Coroiu Cristina
Andi-Bogdan Postelnicu
Sebastian Hengst
Tristan Bourvon
Milan Sreckovic
Kartikaya Gupta
Nicholas Nethercote
Phil Ringnalda