The img.decode promise for animated images that aren't in the document might not resolve in common cases. Specifically for animated images that have been decoded and then discarded, so that mCompositedFrameInvalid is true.
mCompositedFrameInvalid is cleared by either:
1) the image is fully decoded and NotifyDecodeComplete is called.
2) RequestRefresh is able to advance to the current frame.
We don't call RequestRefresh on animated images that aren't in the document and we only decode a certain number of frames ahead of the current frame (even for fully retained animation buffers), so neither of these will happen with long enough animated images.
So we need to make sure that mCompositedFrameInvalid eventually gets cleared any time a decode is requested (which the FrameAnimator gets notified about via GetCompositedFrame). We do that be calling UpdateState in GetCompositedFrame whenever mCompositedFrameInvalid is true and the image is in the surface cache.
This runs into a second problem: UpdateState uses |mIsCurrentlyDecoded = aResult.Surface().IsFullyDecoded()| to determine when to clear mCompositedFrameInvalid. As mentioned above, we don't fully decode animated images. Further, for animated images that are large enough we use a discarding animation buffer that never keeps around all the frames and so IsFullyDecoded always returns false. So we replace that with a check that we can seek to the current animated frame index.
Differential Revision: https://phabricator.services.mozilla.com/D73564
We invalidate for more things than just frames advancing (if we are redecoding and still showing the same frame as before discarding for example). So there is no reason that the dirty rect and a frame being advanced should be linked.
Differential Revision: https://phabricator.services.mozilla.com/D71483
This is actually a one line change. The rest is just removing unneeded parameters.
The composited frame used to be valid for animations that had finished because we saved a separate composited frame, and since the animation had finished it was the final frame, so it had to be valid to display.
Now we don't have a separate composited frame so we only have a valid frame to display if we've decoded up to the current frame. For non-finished animations RequestRefresh will clear mCompositedFrameInvalid, but for finished animations we have to clear it, the easiest place is when we know we are fully decoded.
This problem caused img.decode to never fulfill or reject because we never sent out any frame update notifications because mCompositedFrameInvalid was always false because mAnimationFinished was always true. So we didn't get the invalidation that flipping mCompositedFrameInvalid to false when a full decode finished would cause.
Differential Revision: https://phabricator.services.mozilla.com/D70838
We invalidate for more things than just frames advancing (if we are redecoding and still showing the same frame as before discarding for example). So there is no reason that the dirty rect and a frame being advanced should be linked.
Differential Revision: https://phabricator.services.mozilla.com/D71483
This is actually a one line change. The rest is just removing unneeded parameters.
The composited frame used to be valid for animations that had finished because we saved a separate composited frame, and since the animation had finished it was the final frame, so it had to be valid to display.
Now we don't have a separate composited frame so we only have a valid frame to display if we've decoded up to the current frame. For non-finished animations RequestRefresh will clear mCompositedFrameInvalid, but for finished animations we have to clear it, the easiest place is when we know we are fully decoded.
This problem caused img.decode to never fulfill or reject because we never sent out any frame update notifications because mCompositedFrameInvalid was always false because mAnimationFinished was always true. So we didn't get the invalidation that flipping mCompositedFrameInvalid to false when a full decode finished would cause.
Differential Revision: https://phabricator.services.mozilla.com/D70838
This was done by:
This was done by applying:
```
diff --git a/python/mozbuild/mozbuild/code-analysis/mach_commands.py b/python/mozbuild/mozbuild/code-analysis/mach_commands.py
index 789affde7bbf..fe33c4c7d4d1 100644
--- a/python/mozbuild/mozbuild/code-analysis/mach_commands.py
+++ b/python/mozbuild/mozbuild/code-analysis/mach_commands.py
@@ -2007,7 +2007,7 @@ class StaticAnalysis(MachCommandBase):
from subprocess import Popen, PIPE, check_output, CalledProcessError
diff_process = Popen(self._get_clang_format_diff_command(commit), stdout=PIPE)
- args = [sys.executable, clang_format_diff, "-p1", "-binary=%s" % clang_format]
+ args = [sys.executable, clang_format_diff, "-p1", "-binary=%s" % clang_format, '-sort-includes']
if not output_file:
args.append("-i")
```
Then running `./mach clang-format -c <commit-hash>`
Then undoing that patch.
Then running check_spidermonkey_style.py --fixup
Then running `./mach clang-format`
I had to fix four things:
* I needed to move <utility> back down in GuardObjects.h because I was hitting
obscure problems with our system include wrappers like this:
0:03.94 /usr/include/stdlib.h:550:14: error: exception specification in declaration does not match previous declaration
0:03.94 extern void *realloc (void *__ptr, size_t __size)
0:03.94 ^
0:03.94 /home/emilio/src/moz/gecko-2/obj-debug/dist/include/malloc_decls.h:53:1: note: previous declaration is here
0:03.94 MALLOC_DECL(realloc, void*, void*, size_t)
0:03.94 ^
0:03.94 /home/emilio/src/moz/gecko-2/obj-debug/dist/include/mozilla/mozalloc.h:22:32: note: expanded from macro 'MALLOC_DECL'
0:03.94 MOZ_MEMORY_API return_type name##_impl(__VA_ARGS__);
0:03.94 ^
0:03.94 <scratch space>:178:1: note: expanded from here
0:03.94 realloc_impl
0:03.94 ^
0:03.94 /home/emilio/src/moz/gecko-2/obj-debug/dist/include/mozmemory_wrap.h:142:41: note: expanded from macro 'realloc_impl'
0:03.94 #define realloc_impl mozmem_malloc_impl(realloc)
Which I really didn't feel like digging into.
* I had to restore the order of TrustOverrideUtils.h and related files in nss
because the .inc files depend on TrustOverrideUtils.h being included earlier.
* I had to add a missing include to RollingNumber.h
* Also had to partially restore include order in JsepSessionImpl.cpp to avoid
some -WError issues due to some static inline functions being defined in a
header but not used in the rest of the compilation unit.
Differential Revision: https://phabricator.services.mozilla.com/D60327
--HG--
extra : moz-landing-system : lando
rg -l 'mozilla/Move.h' | xargs sed -i 's/#include "mozilla\/Move.h"/#include <utility>/g'
Further manual fixups and cleanups to the include order incoming.
Differential Revision: https://phabricator.services.mozilla.com/D60323
--HG--
extra : moz-landing-system : lando
This requires replacing inclusions of it with inclusions of more specific prefs
files.
The exception is that StaticPrefsAll.h, which is equivalent to StaticPrefs.h,
and is used in `Codegen.py` because doing something smarter is tricky and
suitable for a follow-up. As a result, any change to StaticPrefList.yaml will
still trigger recompilation of all the generated DOM bindings files, but that's
still a big improvement over trigger recompilation of every file that uses
static prefs.
Most of the changes in this commit are very boring. The only changes that are
not boring are modules/libpref/*, Codegen.py, and ServoBindings.toml.
Differential Revision: https://phabricator.services.mozilla.com/D39138
--HG--
extra : moz-landing-system : lando
Currently it's completely unclear at use sites that the getters for `once`
static prefs return the pref value from startup, rather than the current pref
value. (Bugs have been caused by this.) This commit improves things by changing
the getter name to make it clear that the pref value obtained is from startup.
This required changing things within libpref so it distinguishes between the
"base id" (`foo_bar`) and the "full id" (`foo_bar` or
`foo_bar_DoNotUseDirectly` or `foo_bar_AtStartup` or
`foo_bar_AtStartup_DoNotUseDirectly`; the name used depends on the `mirror` and
`do_not_use_directly` values in the YAML definition.) The "full id" is used in
most places, while the "base id" is used for the `GetPrefName_*` and
`GetPrefDefault_*` functions.
(This is a nice demonstration of the benefits of the YAML file, BTW. Making
this change with the old code would have involved adding an entry to every
single pref in StaticPrefList.h.)
The patch also rejigs the comment at the top of StaticPrefList.yaml, to clarify
some things.
Differential Revision: https://phabricator.services.mozilla.com/D38604
--HG--
extra : moz-landing-system : lando
gfxPrefs Live preferences are almost identical to StaticPrefs.
We leave aside for now those that set a custom change callback as this feature isn't yet supported in StaticPrefs.
Differential Revision: https://phabricator.services.mozilla.com/D31256
--HG--
extra : moz-landing-system : lando
gfxPrefs Live preferences are almost identical to StaticPrefs.
We leave aside for now those that set a custom change callback as this feature isn't yet supported in StaticPrefs.
Differential Revision: https://phabricator.services.mozilla.com/D31256
--HG--
extra : moz-landing-system : lando
gfxPrefs Live preferences are almost identical to StaticPrefs.
We leave aside for now those that set a custom change callback as this feature isn't yet supported in StaticPrefs.
Differential Revision: https://phabricator.services.mozilla.com/D31256
--HG--
extra : moz-landing-system : lando
Also, fix a minor bug where when we discard an animated image that is a
full frame animated image, we need to reset
AnimationState::mCompositedFrameInvalid to false, just like we do for
animated images blended by FrameAnimator. This is because it is used as
part of our state checking in FrameAnimator::GetCompositedFrame before
we are willing to yield frame data.
Differential Revision: https://phabricator.services.mozilla.com/D12362
When blending full frames off the main thread, FrameAnimator no longer
requires access to the raw data of the frame to advance the animation.
Now we only request a RawAccessFrameRef for the current/next frames when
we have discovered that we need to do blending on the main thread.
In addition to avoiding the mutex overhead of RawAccessFrameRef, this
will also facilitate potentially optimizing the surfaces for the
DrawTarget for individual animated image frames.
Differential Revision: https://phabricator.services.mozilla.com/D7506
We were marking them used even if only a decode was requested.
This can cause us to hold extra decoded copies of the image around because we have a tendency to request decode at the intrinsic size.
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.
This was done automatically replacing:
s/mozilla::Move/std::move/
s/ Move(/ std::move(/
s/(Move(/(std::move(/
Removing the 'using mozilla::Move;' lines.
And then with a few manual fixups, see the bug for the split series..
MozReview-Commit-ID: Jxze3adipUh
In FrameAnimator::GetCompositedFrame, we call SurfaceCache::Lookup even
when we use the composited frame directly and leave the lookup result
unused. The only value in performing the lookup could be to mark the
surface as used to avoid expiring it too soon, but
FrameAnimator::RequestRefresh should already be doing enough to keep it
alive, if the image isn't locked in the first place.
In FrameAnimator::RequestRefresh and AdvanceFrame, we currently create
several RawAccessFrameRef objects to the same frames, either to get
timeouts or perform the blending. With some tweaking, we can avoid
requesting the same frame more than once. This will avoid mutex locks on
the surface provider and the frame itself.
DrawableSurface only exposes DrawableFrameRef to its users. This is
sufficient for the drawing related code in general, but FrameAnimator
really needs RawAccessFrameRef to the underlying pixel data (which may
be paletted). While one can get a RawAccessFrameRef from a
DrawableFrameRef, it requires yet another lock of the imgFrame's mutex.
We can avoid this extra lock if we just allow the callers to get the
right data type in the first place.
All animated images on a page are currently registered with the refresh
driver and advance with the tick refresh. These animations may not even
be in view, and if they are large and thus cause redecoding, cause a
marked increase in CPU usage for no benefit to the user.
This patch adds an additional flag, mCompositedFrameRequested, to the
AnimationState used by FrameAnimator. It is set to true each time the
current animated image frame is requested via
FrameAnimator::GetCompositedFrame. It is set to false each time the
frame is advanced in FrameAnimator::AdvanceFrame (via
FrameAnimator::RequestRefresh). If it is true when
FrameAnimator::RequestRefresh is called, then it will advance the
animation according to the normal rules. If it is false, then it will
set the current animation time to the current time instead of advancing.
This should not cause the animation to fall behind anymore or skip
frames more than it does today. This is because if
FrameAnimator::GetCompositedFrame is not called, then the internal state
of the animation is advancing ahead of what the user sees. Once it is
called, the new frame is far ahead of the previously displayed frame.
The only difference now is that we will display the previous frame for
slightly longer until the next refresh tick.
Note that if an animated image is layerized (should not happen today) or
otherwise uses an ImageContainer, this optimization fails. While we know
whether or not we have an image container, we do not know if anything is
actively using it.
Note that AnimationSurfaceProvider will override these methods to give a
proper implementation in a later patch in this series. For now, they are
mostly stubbed, using the default implementation from ISurfaceProvider.
They focus on the main operations we perform on an animation:
1) Progressing through the animation, e.g. advancing a frame. If we
don't decode the whole animation up front, we need to know at the
decoder level where we are in the display of the animation.
2) Restarting an animation from the beginning. This is a specialized
case of the above, where we want to skip explicitly advancing through
the remaining frames and instead restart at the beginning. The decoder
may have already discarded the earliest frames and must start redecoding
them.
3) Knowing whether or not the decoder is still active, e.g. can we be
missing frames.
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.
When an animated image has been discarded, we avoided marking the
composited frame invalid unless it had been previously decoded. Most of
the time this was fine, but if the animated image was still decoding for
the first time, then we still had a composited frame lingering that we
did not mark as invalid. As a result, when we called
RasterImage::LookupFrame (and indirectly
FrameAnimator::GetCompositedFrame), it would always return the
composited frame. This meant that RasterImage::Decode would never be
called to trigger a redecode. At the same time,
FrameAnimator::RequestRefresh would not cause us to advance the frame
because the state was still discarded.
With this patch we separate out the concepts of "has ever requested to
be decoded" and "has ever completed decoding." The former is now used to
control whether or not a composited frame is marked as invalid after we
discover we currently have no surface for the animation -- this solves
the animation remaining frozen as we now request the redecode as
expected. The latter remains used to determine if we actually know the
total number of frames.
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
We invalidate if FrameAnimator::UpdateState marks the composited frame as valid, but we fail to do so if FrameAnimator::RequestRefresh does so.
The other place that sets the composited frame as valid is RasterImage::Decode. This call is actually redundant because the UpdateState call will do the same.
Even though we will invalidate when the decode produces results we could still draw incorrectly if something else invalidates. In which case we would only draw the part of the image that was invalidated. But that should actually be impossible as explained in the comment.
Timothy Nikkel
Daniel Varga
Simon Giesecke
Emilio Cobos Álvarez
Nicholas Nethercote
Jean-Yves Avenard
Gurzau Raul
Andrew Osmond
Sylvestre Ledru
Narcis Beleuzu
Coroiu Cristina
Tom Ritter
Milan Sreckovic