Closed Bug 1525320 Opened Last year Closed 11 months ago

Add config prefs that let us tell media and animated images to paint only the first frame

Categories

(Core :: Performance, enhancement)

37 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: jesup, Assigned: whawkins)

Details

(Whiteboard: [qf:p1:pageload])

Attachments

(2 files, 11 obsolete files)

8.20 KB, patch
jesup
: feedback+
Details | Diff | Splinter Review
47 bytes, text/x-phabricator-request
Details | Review

For visual comparisons like SpeedIndex, animations and videos make it hard to do automated comparison of video frames between runs (for visual completeness, etc).

Config prefs to only paint (and repaint) static images would make comparison far easier and more accurate. Painting on the first image and then repainting that would work, but painting a fixed pattern for all videos and images would be better, because some pages will show different videos or anims depending on random factors.

Whiteboard: [qf] → [qf:p1:pageload]

(In reply to Randell Jesup [:jesup] from comment #0)

For visual comparisons like SpeedIndex, animations and videos make it hard to do automated comparison of video frames between runs (for visual completeness, etc).

Config prefs to only paint (and repaint) static images would make comparison far easier and more accurate. Painting on the first image and then repainting that would work, but painting a fixed pattern for all videos and images would be better, because some pages will show different videos or anims depending on random factors.

I believe that I have a very small, but relatively effective, change that will handle this. It's not perfect, but I'm happy to share the code with you if you are interested.

Hope it helps!
Will

Flags: needinfo?(rjesup)

sure, that'd be great!

Flags: needinfo?(rjesup)

This patch adds a browser.measurement.stop_play boolean preference that will stop videos from displaying more than their first frame when doing visual measurement.

Flags: needinfo?(rjesup)

Thanks!

This is better than playing the video; however the best solution here would be to paint on each new frame, but paint the original frame (or even better, a pre-defined image used in the place of all videos). Note that if we replace videos with a fixed image, we need to ensure it isn't scaled or expensive to paint.

If you need advice on how to do this, ask on IRC in #media, perhaps of jya

Also, we need this to be controlled by a config pref.

Separately, we want to do something similar for animgifs. You may want to ask in #gfx on IRC about that. They can be two separate patches (and config vars).

Flags: needinfo?(rjesup)

(In reply to Randell Jesup [:jesup] from comment #4)

Thanks!

Thanks for the feedback!

This is better than playing the video; however the best solution here would be to paint on each new frame, but paint the original frame (or even better, a pre-defined image used in the place of all videos). Note that if we replace videos with a fixed image, we need to ensure it isn't scaled or expensive to paint.

If you need advice on how to do this, ask on IRC in #media, perhaps of jya

I will absolutely reach out to jya for help on this. I am going to do some investigation on my own first and then I will go there if I need help. The learning challenge is fun!

Also, we need this to be controlled by a config pref.

I tried. I contacted you directly in irc about this. I will investigate further.

Separately, we want to do something similar for animgifs. You may want to ask in #gfx on IRC about that. They can be two separate patches (and config vars).

(In reply to Will Hawkins from comment #5)

(In reply to Randell Jesup [:jesup] from comment #4)

Thanks!

Thanks for the feedback!

This is better than playing the video; however the best solution here would be to paint on each new frame, but paint the original frame (or even better, a pre-defined image used in the place of all videos). Note that if we replace videos with a fixed image, we need to ensure it isn't scaled or expensive to paint.

If you need advice on how to do this, ask on IRC in #media, perhaps of jya

I will absolutely reach out to jya for help on this. I am going to do some investigation on my own first and then I will go there if I need help. The learning challenge is fun!

Also, we need this to be controlled by a config pref.

I tried. I contacted you directly in irc about this. I will investigate further.

Sorry to send this back for you for needinfo, but I think that I am obviously confused about the difference between what I was using (a browser pref, perhaps?) and a "config pref". I want to make sure that I get it right! Thanks for the help on this initial minor task!

Separately, we want to do something similar for animgifs. You may want to ask in #gfx on IRC about that. They can be two separate patches (and config vars).

Flags: needinfo?(rjesup)
Attached patch 1525320.patch (obsolete) — Splinter Review

While this version still does not address animated gifs, it does allow for videos to render only the first frame while continuing to let the video element act normally.

Attachment #9042246 - Attachment is obsolete: true

(In reply to Randell Jesup [:jesup] from comment #0)

For visual comparisons like SpeedIndex, animations and videos make it hard to do automated comparison of video frames between runs (for visual completeness, etc).

Config prefs to only paint (and repaint) static images would make comparison far easier and more accurate. Painting on the first image and then repainting that would work, but painting a fixed pattern for all videos and images would be better, because some pages will show different videos or anims depending on random factors.

But then, what would you be testing? and should we rely on such tests in the first place?

We had already considered in the past a pref to be used for testing that would have prevented the skip to next keyframe logic when decoding is too slow.
However, while it would make playback consistent it would cause a behaviour that would never been seen by our end user, hence we would be testing something that adds no value.
So we reverted that behaviour.

The only reftest that makes sense is if the video is paused, such as pausing the video, seeking and performing the reftest.
Rather than making an invalid test give consistent result, I'd prefer to make the test valid and serving an actual purpose to start with.

Attached patch 1525320.patch (obsolete) — Splinter Review

This version of the patch properly handles YouTube/Vimeo (video elements) as well as animated GIFs and PNGs. Feedback is definitely welcome!

Attachment #9042368 - Attachment is obsolete: true

For visual comparisons like SpeedIndex, animations and videos make it hard to do automated comparison of video frames between runs (for visual completeness, etc).

Config prefs to only paint (and repaint) static images would make comparison far easier and more accurate. Painting on the first image and then repainting that would work, but painting a fixed pattern for all videos and images would be better, because some pages will show different videos or anims depending on random factors.

But then, what would you be testing? and should we rely on such tests in the first place?

This is specifically so we can feed a recording of the rendering to a visual-comparison algorithm to determine % visually-complete (i.e. what WebPageTest does to calculate SpeedIndex ratings.) Right now video and animations (and offsets in when they start) throw off the calculations; they try to sidestep that partly by looking at chroma information (don't ask me why...)

The only reftest that makes sense is if the video is paused, such as pausing the video, seeking and performing the reftest.
Rather than making an invalid test give consistent result, I'd prefer to make the test valid and serving an actual purpose to start with.

This is not for reftests or any other automated test in our normal test suite; it would be used for raptor and WebPageTest so we can calculate SpeedIndex numbers more accurately. That's why I want to actually render pixels for each frame shown, just ensure that they're all the same pixels every time (so the visual-comparison algorithm considers that area "done").

Comment on attachment 9042636 [details] [diff] [review]
1525320.patch

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +2678,5 @@
>    InitVideoQueuePrefs();
>  
>    DDLINKCHILD("reader", aReader);
> +
> +  if (mozilla::Preferences::GetBool("browser.measurement.first_frame_only")) {

Note that all the other prefs used in this file are either StaticPrefs, or in InitVideoQueuePrefs() which reads Preferences::*() directly for a few.  Note that Preferences::* are generally all MainThread-only APIs, and InitVideoQueuePrefs() asserts it's on MainThread.

We don't need it to necessarily use StaticPrefs here; generally this would be set before starting the browser.

Flags: needinfo?(rjesup)

(In reply to Randell Jesup [:jesup] from comment #11)

Comment on attachment 9042636 [details] [diff] [review]
1525320.patch

Review of attachment 9042636 [details] [diff] [review]:

::: dom/media/MediaDecoderStateMachine.cpp
@@ +2678,5 @@

InitVideoQueuePrefs();

DDLINKCHILD("reader", aReader);
+

  • if (mozilla::Preferences::GetBool("browser.measurement.first_frame_only")) {

Note that all the other prefs used in this file are either StaticPrefs, or
in InitVideoQueuePrefs() which reads Preferences::() directly for a few.
Note that Preferences::
are generally all MainThread-only APIs, and
InitVideoQueuePrefs() asserts it's on MainThread.

Thanks for the review. Granting the fact that there is no assertion here that we are on the main thread (which I agree there should be if I wanted to leave the GetBool call there [but won't, see below]), if we are able to call InitVideoQueuePrefs() without asserting, is there any way that this constructor could get called off the main thread? I just checked the usages of the constructor and they are all invoked within functions that themselves assert they are on the main thread.

Just to be clear, I am not pushing back in any way (I am certainly going to move my preference fetch to the the InitVideoQueuePrefs() function). I am just curious about the code flow and want to make sure that I have the proper mental model of how threads are handled in this part of the code.

Thanks again for the review!

Flags: needinfo?(rjesup)
Comment on attachment 9042636 [details] [diff] [review]
1525320.patch

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +2679,5 @@
>  
>    DDLINKCHILD("reader", aReader);
> +
> +  if (mozilla::Preferences::GetBool("browser.measurement.first_frame_only")) {
> +    SetFirstFrameOnly(true);

Definitely move this to a StaticPref - see modules/libpref/init/StaticPrefList.h.  If we're accessing it off mainthread, use an atomic type (see the .h file)

@@ +2787,5 @@
> +  return mFirstFrameOnly;
> +}
> +
> +void MediaDecoderStateMachine::SetFirstFrameOnly(bool aFirstFrameOnly) {
> +  mFirstFrameOnly = aFirstFrameOnly;

We don't need Set/GetFirstFrameOnly - it's used internally to the class only, and with a StaticPref we would have even less need for it.

::: image/AnimationSurfaceProvider.cpp
@@ +51,5 @@
> +
> +  if (mozilla::Preferences::GetBool("browser.measurement.first_frame_only")) {
> +    mDecoder->SetDecoderFlags(mDecoder->GetDecoderFlags() |
> +        DecoderFlags::FIRST_FRAME_ONLY);
> +  }

This is a reasonable first cut, but what we really want is to render for each new frame, but render the same data as the first frame (or a frame of fixed data, preferably not a blank background).  We want to keep the same (roughly) execution paths and timings.

Again, this should be a StaticPref to avoid having to worry about what thread it's on.
Flags: needinfo?(rjesup)
Attached patch 1525320.patch (obsolete) — Splinter Review

This is a full featured version of the patch. With this patch, when the user sets browser.measurement.render_animations_blank, all (gif, png, webp and <video>) animations will be rendered as a solid color. NB: This patch does not affect what "animations" sites lay on top of those animations (e.g., controls, ads, etc). Per feedback from previous iterations, this patch attempts to maintain the original decoding/rendering cycle as much as possible so that it is useful for performance tests.

Attachment #9042636 - Attachment is obsolete: true
Flags: needinfo?(rjesup)
Comment on attachment 9043741 [details] [diff] [review]
1525320.patch

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

It would be much better IMHO to patch this at the compositor level rather than individually patching where images are rendered (there are some path missing like with webrtc, or jpeg). That way you only ever need to patch it once and not have to worry about where images are created from.

Also, under which circumstances would that pref be set? for all tests?

(In reply to Jean-Yves Avenard [:jya] from comment #16)

Comment on attachment 9043741 [details] [diff] [review]
1525320.patch

Review of attachment 9043741 [details] [diff] [review]:

It would be much better IMHO to patch this at the compositor level rather
than individually patching where images are rendered (there are some path

I am happy to do this however you and jesup think is best. I thought that I was following his instructions on how to do this, but, again, I'm new here and just trying to do the right thing.

missing like with webrtc, or jpeg). That way you only ever need to patch it
once and not have to worry about where images are created from.

This makes perfect sense. As I understand it, we are focusing on animated "things" for non-interactive tests which would mean that webrtc is likely out of scope. As for jpeg, are you referring to Motion JPEG?

Also, under which circumstances would that pref be set? for all tests?

That's a jesup question, I think. Sorry I can't answer.

Flags: needinfo?(jyavenard)
Comment on attachment 9043741 [details] [diff] [review]
1525320.patch

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

I added a few notes-to-self for a separate revision to this patch. That is, of course if we leave it like this -- we may want to restructure it completely as :jya suggested!

::: image/decoders/nsPNGDecoder.cpp
@@ +743,4 @@
>    const uint32_t pixel = gfxPackedPixel(0xFF, aRawPixelInOut[0],
>                                          aRawPixelInOut[1], aRawPixelInOut[2]);
>    aRawPixelInOut += 3;
> +  if (blank) {

I think that we should wrap this in a MOZ_UNLIKELY. (same with the subsequent ifs in the next few hunks.)

@@ +879,5 @@
>    if (HasAlphaChannel()) {
>      if (mDisablePremultipliedAlpha) {
>        result = mPipe.WritePixelsToRow<uint32_t>(
> +          [&] { return PackUnpremultipliedRGBAPixelAndAdvance(rowToWrite,
> +              GetRenderAnimationsBlank()); });

I want to make sure that the calls to these functions are inlined.

::: image/decoders/nsPNGDecoder.cpp
@@ +743,4 @@

const uint32_t pixel = gfxPackedPixel(0xFF, aRawPixelInOut[0],
aRawPixelInOut[1], aRawPixelInOut[2]);
aRawPixelInOut += 3;

  • if (blank) {

I think that we should wrap this in a MOZ_UNLIKELY. (same with the
subsequent ifs in the next few hunks.)

Right

@@ +879,5 @@

if (HasAlphaChannel()) {
if (mDisablePremultipliedAlpha) {
result = mPipe.WritePixelsToRow<uint32_t>(

  •      [&] { return PackUnpremultipliedRGBAPixelAndAdvance(rowToWrite,
    
  •          GetRenderAnimationsBlank()); });
    

I want to make sure that the calls to these functions are inlined.

Perhaps make them MOZ_ALWAYS_INLINE?

(In reply to Jean-Yves Avenard [:jya] from comment #16)

It would be much better IMHO to patch this at the compositor level rather
than individually patching where images are rendered (there are some path
missing like with webrtc, or jpeg). That way you only ever need to patch it
once and not have to worry about where images are created from.

That might make well make sense; my point was to ensure that as much as possible the code and data flows remain the same as normal (but that the data displayed was always the same and not a blank background (or probably solid color)), to avoid perturbing the profiling results and allow automated page-comparison tools to do a good job.

Also, under which circumstances would that pref be set? for all tests?

No, I expect it would only be set for tests doing automated visual comparisons for completeness, in particular in external tools like WebPageTest, or when we add visual-completeness metrics to raptor.

Flags: needinfo?(rjesup)
Comment on attachment 9043741 [details] [diff] [review]
1525320.patch

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

::: image/Decoder.cpp
@@ +67,5 @@
>        mShouldReportError(false),
> +      mFinalizeFrames(true),
> +      mRenderAnimationsBlank(false) {
> +  if (StaticPrefs::browser_measurement_render_animations_blank()) {
> +    mRenderAnimationsBlank = true;

this is probably smart, as even an relaxed atomic will have more overhead I think than a local bool on the stack (especially when we consider cache effects).  We also don't really care if the pref is switched in mid-run; we should be fine with requiring a fresh start to ensure it's uniformly picked up.

::: image/decoders/nsWebPDecoder.cpp
@@ +485,5 @@
>            const uint32_t pixel =
>                gfxPackedPixelNoPreMultiply(src[3], src[0], src[1], src[2]);
>            src += 4;
> +	  if (GetRenderAnimationsBlank()) {
> +	    return AsVariant((const uint32_t)0x0);

Don't use pure black or white....  more likely to end up looking like something else.  If we must use a solid color, choose something unusual - and with a distinct chroma value (not gray), since WebPageTest uses chroma patterns as it a primary way to evaluate (video?), apparently.

Perhaps use a patterned value depending on position?

Also, even introducing a MOZ_UNLIKELY if() at a per-pixel level could negatively impact perf when not using the pref; probably this needs to be at a higher level, or as jya suggests in the compositor.  Avoiding any significant perf impacts in the case of not using the pref is critical.

Based on the feedback, I am going to make another pass at this by doing the blank rendering at the compositor level. I will keep you posted on the progress. Sorry it is taking so long.

Attached patch 1525320.patch (obsolete) — Splinter Review

This is a WIP patch that handles all the image animation formats. The solution here is just one of the options endorsed by jya. There are others and I am going to look into those tomorrow.

The point of this patch is to intercept rasterized images just before they get drawn to the graphics surface. When the rasterized image is from an animation, the render to the graphics surface is skipped.

Having said that this is a WIP, I'd love any feedback that people are willing to offer.

Thanks!

Attachment #9043741 - Attachment is obsolete: true
Flags: needinfo?(jyavenard)
Attached patch 1525320.patch (obsolete) — Splinter Review

This is a complete version of the patch. It renders all animations blank and does so from the most centralized place. There is a path for animated images and there is a path for videos since they are rendered separately. I look forward to feedback so that I can make this as good as possible.

Attachment #9044079 - Attachment is obsolete: true
Flags: needinfo?(rjesup)
Comment on attachment 9044353 [details] [diff] [review]
1525320.patch

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

Nice work. This is getting close.

::: dom/media/mediasink/VideoSink.cpp
@@ +35,5 @@
>  // duration of a 60-fps frame.
>  static const int64_t MIN_UPDATE_INTERVAL_US = 1000000 / (60 * 2);
>  
> +static bool sRenderAnimationsBlankChecked= false;
> +static bool sRenderAnimationsBlank = false;

Same comment as for ImgFrame.cpp

@@ +38,5 @@
> +static bool sRenderAnimationsBlankChecked= false;
> +static bool sRenderAnimationsBlank = false;
> +static RefPtr<Image> sBlankImage = nullptr;
> +static void SetImageToBlackPixel(PlanarYCbCrImage* aImage) {
> +  uint8_t blackPixel[] = {0x10, 0x80, 0x80};

Same comment as to previous patches -- solid colors are more likely to confuse image-comparisons, though we can absolutely land this and revise later if needed.  A "real" image or a pattern (with some chroma content) would be better.

@@ +438,5 @@
>      ImageContainer::NonOwningImage* img = images.AppendElement();
>      img->mTimeStamp = t;
>      img->mImage = frame->mImage;
> +    if (MOZ_UNLIKELY(sRenderAnimationsBlank)) {
> +      img->mImage = sBlankImage;

Since this image will be 1x1, this will cause very different pathways and performance in the rendering code, especially if the GPU is rendering it (not sure if it will only write 1x1, or scale to NxN, or something else - jya?)
While the above is true, it may not matter too much, and we're less interested in exact profile-comparison as avoiding any major perf differences between on and off (at the ms level)

::: image/imgFrame.cpp
@@ +180,5 @@
>        mIsFullFrame(false),
> +      mCompositingFailed(false) {
> +  if (MOZ_UNLIKELY(!sInitializedAnimationsBlank)) {
> +    sInitializedAnimationsBlank = true;
> +    sRenderAnimationsBlank =

I'll defer to jya, but in this case I suspect we should just access the staticpref directly.  This is a smidge faster (bool vs relaxed atomic bool) -- but unlike the other patch, we're doing it on an image basis not a per-pixel basis.

@@ +673,1 @@
>      gfxUtils::DrawPixelSnapped(aContext, surfaceResult.mDrawable,

This appears to draw nothing for the blank case - am I correct?  We need to draw something (and something distinguishable from background).
Flags: needinfo?(rjesup) → needinfo?(jyavenard)

I'm still puzzled on when this pref will be turned on, which tests exactly?

As for the code, we need an image of the same size as the original one. You only need to generate this image once if the resolution has changed.

Flags: needinfo?(jyavenard)

Right now it may be used in future (talos) raptor tests (as part of adding visual completeness measurements ala WebPageTest). We may use it with our private instance of WebPageTest and WebPageTest may also use it (optionally, or if we tell it to for a specific run). Visual completeness is a critical measurement for user perception of page load speed.

In terms of image sizes, could we generate one image and just use part of it whenever the image we're replacing is smaller? (if need be, increase the size as needed, or just make a single "large" image and not worry about the unlikely event an image is larger)

(In reply to Randell Jesup [:jesup] from comment #26)

Visual completeness is a critical measurement for user perception of page load speed.

I'm still at loss on how much critical a measurement actually is when we paint rubbish content.

All we will prove is that we're fast at painting rubbish.

(In reply to Jean-Yves Avenard [:jya] from comment #27)

(In reply to Randell Jesup [:jesup] from comment #26)

Visual completeness is a critical measurement for user perception of page load speed.

I'm still at loss on how much critical a measurement actually is when we paint rubbish content.

All we will prove is that we're fast at painting rubbish.

The point isn't how fast we paint the content itself. It's so an automated tool can more easily determine when a page reaches "complete"; with normal animation and video simple comparisons will be confused. Even more importantly, this is used to calculate SpeedIndex, which is "the area above the curve" of visual completeness: https://sites.google.com/a/webpagetest.org/docs/using-webpagetest/metrics/speed-index

With this setting, visual completeness calculation and speedindex are much more accurate. Currently WebPageTest's tool uses heuristic tricks to try to avoid being messed up by anims/video.

(In reply to Randell Jesup [:jesup] from comment #24)

Comment on attachment 9044353 [details] [diff] [review]
1525320.patch

Review of attachment 9044353 [details] [diff] [review]:

Nice work. This is getting close.

::: dom/media/mediasink/VideoSink.cpp
@@ +35,5 @@

// duration of a 60-fps frame.
static const int64_t MIN_UPDATE_INTERVAL_US = 1000000 / (60 * 2);

+static bool sRenderAnimationsBlankChecked= false;
+static bool sRenderAnimationsBlank = false;

Same comment as for ImgFrame.cpp

@@ +38,5 @@

+static bool sRenderAnimationsBlankChecked= false;
+static bool sRenderAnimationsBlank = false;
+static RefPtr<Image> sBlankImage = nullptr;
+static void SetImageToBlackPixel(PlanarYCbCrImage* aImage) {

  • uint8_t blackPixel[] = {0x10, 0x80, 0x80};

Same comment as to previous patches -- solid colors are more likely to
confuse image-comparisons, though we can absolutely land this and revise
later if needed. A "real" image or a pattern (with some chroma content)
would be better.

I've changed this to make it a "green" image. The problem with making it a pattern at this stage is that we don't ultimately know how big the image is going to be. So, if we make it smaller than the ultimate display, the container is going to scale the pattern in ways that will render it "odd". I tried several today and they did not render reliably in ways that would make comparison straightforward.

In other words, it seems like using a special set color is the way to go if we want to minimize the overhead of rendering special images by creating them a single time when the Sink is created.

Does that description make sense?

@@ +438,5 @@

 ImageContainer::NonOwningImage* img = images.AppendElement();
 img->mTimeStamp = t;
 img->mImage = frame->mImage;
  • if (MOZ_UNLIKELY(sRenderAnimationsBlank)) {
  •  img->mImage = sBlankImage;
    

Since this image will be 1x1, this will cause very different pathways and
performance in the rendering code, especially if the GPU is rendering it
(not sure if it will only write 1x1, or scale to NxN, or something else -
jya?)
While the above is true, it may not matter too much, and we're less
interested in exact profile-comparison as avoiding any major perf
differences between on and off (at the ms level)

Makes sense. For the time being, I will leave it as it is!

::: image/imgFrame.cpp
@@ +180,5 @@

   mIsFullFrame(false),
  •  mCompositingFailed(false) {
    
  • if (MOZ_UNLIKELY(!sInitializedAnimationsBlank)) {
  • sInitializedAnimationsBlank = true;
  • sRenderAnimationsBlank =

I'll defer to jya, but in this case I suspect we should just access the
staticpref directly. This is a smidge faster (bool vs relaxed atomic bool)
-- but unlike the other patch, we're doing it on an image basis not a
per-pixel basis.

I understand what you are saying, but don't necessarily understand what should be changed. I am reading your feedback as:

Simply check the staticpref here and subsequently when we render?

It makes sense to me, I just wanted to make sure that we are on the same page!

@@ +673,1 @@

 gfxUtils::DrawPixelSnapped(aContext, surfaceResult.mDrawable,

This appears to draw nothing for the blank case - am I correct? We need to
draw something (and something distinguishable from background).

See my previous comment about "green" images. If that is an okay solution, then I will draw green pixels!

Thanks again for the review!
Will

Flags: needinfo?(rjesup)
Attached patch 1525320.patch (obsolete) — Splinter Review

Here is an updated version. It responds to the feedback from jesup and handles the case where the user seeks a video that is paused. Previous versions of the patch did not handle such a scenario.

In this patch, all animated content is rendered as a white (opaque) image. If we are concerned that rendering something white cause problems, we can change it to some other color (green, perhaps) but that would cause some additional work in the animated image decoding pipeline code.

I hope that this version is an improvement over the previous ones. Thanks for all the reviews!

Attachment #9044353 - Attachment is obsolete: true

Same comment as to previous patches -- solid colors are more likely to
confuse image-comparisons, though we can absolutely land this and revise
later if needed. A "real" image or a pattern (with some chroma content)
would be better.

I've changed this to make it a "green" image. The problem with making it a pattern at this stage is that we don't ultimately know how big the image is going to be. So, if we make it smaller than the ultimate display, the container is going to scale the pattern in ways that will render it "odd". I tried several today and they did not render reliably in ways that would make comparison straightforward.

In other words, it seems like using a special set color is the way to go if we want to minimize the overhead of rendering special images by creating them a single time when the Sink is created.

So, we could have a single static large image we render a subset of, or as looked at earlier, a single image per anim/video.

If we need to stick to a single color, we also could use a different color for each anim/video, which should help the visual-completeness tool (which tries to not use location as a prime factor when comparing).

However: we can land it with Green, and then see how well it works and tweak from there. So Green is ok for now.

::: image/imgFrame.cpp
@@ +180,5 @@

   mIsFullFrame(false),
  •  mCompositingFailed(false) {
    
  • if (MOZ_UNLIKELY(!sInitializedAnimationsBlank)) {
  • sInitializedAnimationsBlank = true;
  • sRenderAnimationsBlank =

I'll defer to jya, but in this case I suspect we should just access the
staticpref directly. This is a smidge faster (bool vs relaxed atomic bool)
-- but unlike the other patch, we're doing it on an image basis not a
per-pixel basis.

I understand what you are saying, but don't necessarily understand what should be changed. I am reading your feedback as:

Simply check the staticpref here and subsequently when we render?

yes.

It makes sense to me, I just wanted to make sure that we are on the same page!

@@ +673,1 @@

 gfxUtils::DrawPixelSnapped(aContext, surfaceResult.mDrawable,

This appears to draw nothing for the blank case - am I correct? We need to
draw something (and something distinguishable from background).

See my previous comment about "green" images. If that is an okay solution, then I will draw green pixels!

Yes.

Flags: needinfo?(rjesup)

In this patch, all animated content is rendered as a white (opaque) image. If we are concerned that rendering something white cause problems, we can change it to some other color (green, perhaps) but that would cause some additional work in the animated image decoding pipeline code.

White is really a bad choice, since it's the typical background color. Go with green.

Attached patch 1525320.patch (obsolete) — Splinter Review

Thanks to all the earlier feedback, as usual! This is a version that makes all blanked animations appear green. There are a few asserts in the animated image case that limit the conditions under which this type of test will work, but I think that it covers all the major cases. Please continue to let me know if there are any other ways that I can improve this work!

Assignee: nobody → whawkins
Attachment #9044787 - Attachment is obsolete: true
Flags: needinfo?(rjesup)
Comment on attachment 9045446 [details] [diff] [review]
1525320.patch

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

::: dom/media/mediasink/VideoSink.cpp
@@ +37,5 @@
>  
> +static bool sBlankImageCreatedIfNecessary = false;
> +static RefPtr<Image> sBlankImage = nullptr;
> +static void SetImageToGreenPixel(PlanarYCbCrImage* aImage) {
> +  uint8_t greenPixel[] = {0x00, 0x00, 0x00};

green is 0/0/0?  also: static const probably

BTW, ask for feedback (or review) on a patch, not needinfo, in general.

View Details for a splinter patch to set them. (or hg bzexport -F :jesup or -r :jesup)

Flags: needinfo?(rjesup)
Attached patch 1525320.patch (obsolete) — Splinter Review

Adds 'static' to the type of the green pixel data. It cannot be const without casting it away when assigning to the YCbCrData data.

Attachment #9045446 - Attachment is obsolete: true
Attachment #9045468 - Flags: feedback?(rjesup)

(In reply to Randell Jesup [:jesup] from comment #34)

green is 0/0/0? also: static const probably

with YUV, yes.

Comment on attachment 9045468 [details] [diff] [review]
1525320.patch

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

::: image/imgFrame.cpp
@@ +243,4 @@
>    mDirtyRect = aRect;
>  
>    if (aAnimParams) {
> +    mOfAnimation = true;

Perhaps "mAnimation" or mIsAnimation (I'm unsure what "OfAnimation" means...)

::: modules/libpref/init/StaticPrefList.h
@@ +423,5 @@
>  
> +// Disable blank animation rendering needed for performance testing
> +VARCACHE_PREF(
> +  "browser.measurement.render_animations_blank",
> +  browser_measurement_render_animations_blank,

The description and name don't match what it does: blanks anims and video.
browser.measurement.render_animations_blank
and
browser.measurement.render_video_blank

We could also keep it as one pref, render_anims_and_video_blank, which would be fine, as I don't think we'd want to blank one without the other.
Attachment #9045468 - Flags: feedback?(rjesup) → feedback+
Attached patch 1525320.patchSplinter Review

Thanks to the feedback from jesup, this is a better version of the patch:

  1. Changes the mOfAnimation member variable name to mFrameIsPartOfAnimation
  2. Changes the name of the preference to browser.measurement.render_anims_and_video_solid and includes a better comment describing what the flag does.
Attachment #9045468 - Attachment is obsolete: true
Attachment #9045957 - Flags: feedback?(rjesup)
Attachment #9045957 - Flags: feedback?(rjesup) → feedback+
Attachment #9046564 - Attachment is obsolete: true
Attachment #9046501 - Attachment is obsolete: true
Keywords: checkin-needed

Tried to land this but got:

On Sat, March 9, 2019, 10:10 PM GMT+2, by apavel@mozilla.com.
Revisions: D21613 diff 73290
Details: We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. (255, 'applying /tmp/tmp1lGNPq\npatching file dom/media/mediasink/VideoSink.cpp\nHunk #3 FAILED at 315\n1 out of 5 hunks FAILED -- saving rejects to file dom/media/mediasink/VideoSink.cpp.rej\nabort: patch failed to apply', '')

Flags: needinfo?(whawkins)
Keywords: checkin-needed

(In reply to Andreea Pavel [:apavel] from comment #43)

Tried to land this but got:

On Sat, March 9, 2019, 10:10 PM GMT+2, by apavel@mozilla.com.
Revisions: D21613 diff 73290
Details: We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. (255, 'applying /tmp/tmp1lGNPq\npatching file dom/media/mediasink/VideoSink.cpp\nHunk #3 FAILED at 315\n1 out of 5 hunks FAILED -- saving rejects to file dom/media/mediasink/VideoSink.cpp.rej\nabort: patch failed to apply', '')

Sorry about this and thank you for attempting to land this. I did the rebase and I am just getting a sanity check on that updated patch before I attempt to have someone land it again.

Thanks again for your help and my apologies.
Will

Flags: needinfo?(whawkins)

Hey Will, no problem. we check regularly for checkin-needed bugs and when you're done we'll try to land it again.

Attachment #9047552 - Flags: feedback?(jyavenard)
Attachment #9047552 - Flags: feedback?(jyavenard)
Keywords: checkin-needed

Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/80d51d772560
Add config prefs that let us tell media and animated images to paint only the first frame r=jya

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.