Code analysis found 256 defects in the diff 523526:

• 256 build errors found by clang-tidy

You can run this analysis locally with:

• (C/C++)

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

What causes colors to change? Is this patch series more than just a pure optimization?

Also, the difference on looks very noticeable to the human eye; the big gray rectangle in the bottom left disappears.

In D134352#4389515, @mstange wrote:

What causes colors to change? Is this patch series more than just a pure optimization?

Also, the difference on looks very noticeable to the human eye; the big gray rectangle in the bottom left disappears.

I pushed a try run again today, and the big difference you mentioned would only happen on Windows. The weird thing is that, in all those failure cases, the reference file would show video not being showed in full-range color (like this, search for ), but the reference file is always format. These series of patches only affect h264, vp8 and vp9, it won't affect AV1.

AV1 video would be decoded by another decoder (dav1decoder), not ffmpeg decoder, so in theroy the failure reference case is not possible caused by the change here. I'm going to test my patches on my Windows 10 machine to see if I can reproduce that issue locally.

@mstange In this try-push, I disabled the feature of using shmem on ffmpeg, and you can see passed on Windows . Then take a deeper look into the result, you would find that the AV1 video that we want to compare with was actually different from the png file.

720p.png.bt709.bt709.pc.yuv420p.av1.webm == dom/media/test/reftest/color_quads/720p.png | image comparison, max difference: 38, number of differing pixels: 181038

And the following two tests (h264 and vp9) were having the same result with av1, which means they were also wrong.

dom/media/test/reftest/color_quads/720p.png.bt709.bt709.pc.yuv420p.vp9.webm == dom/media/test/reftest/color_quads/720p.png.bt709.bt709.pc.yuv420p.av1.webm | image comparison, max difference: 0, number of differing pixels: 0
dom/media/test/reftest/color_quads/720p.png.bt709.bt709.pc.yuv420p.vp9.mp4 == dom/media/test/reftest/color_quads/720p.png.bt709.bt709.pc.yuv420p.av1.webm | image comparison, max difference: 0, number of differing pixels: 0

Therefore, I suspect that these series comparison () are already wrong for a while, and my patches actually fix the result for vp9 and h264. That is why h264 and vp9 shows the correct result (search and ), but av1 didn't.

To further prove my assumption, in this try run, I changed the comparision to make h264 and vp9 comparing with png file, not av1 directly. Then you can see that and are both matching the png file, some fuzzy noise though.

In addition, currently we compare AV1 with PNG first, then compare other codecs with AV1. I think it would be better to compare AV1 with PNG first to ensure that AV1 result is correct, then compare all other codec with PNG, not AV1 directly. That can help to avoid this kind of error happening again.

Thanks for the analysis!

In D134352#4392240, @alwu wrote:

Therefore, I suspect that these series comparison () are already wrong for a while, and my patches actually fix the result for vp9 and h264.

I see. Were the patches expected to fix any bugs? That's the part that confuses me.

I think what you're suggesting sounds good - comparing more things to known-good PNG files. Maybe you can make that change first, "before" this patch series?
Ideally, patches which fix bugs should remove fuzzy annotations, not add them.

In D134352#4395212, @mstange wrote:

I see. Were the patches expected to fix any bugs? That's the part that confuses me.

No, so that is also a weird thing to me. The only possibility I could think of is that, because of the issue I discovered here (the second one), maybe doing decoding to shmem directly would somehow change timing or something to make vp9 and h264 capture the correct image.

I think what you're suggesting sounds good - comparing more things to known-good PNG files. Maybe you can make that change first, "before" this patch series?
Ideally, patches which fix bugs should remove fuzzy annotations, not add them.

I've NIed you on the bug I filed for improving the reftest first, but if the issue I mentioned in that bug is the cause of the issue we discussed here, then I think this issue should not be a blocker of this bug. In addition, as this feature is controlled by a pref, we can turn off it anytime (currently only turn it on on Nightly) if we discover any serious regression.

To review changes to the reftest markings, I really need to see the treeherder test failures without the test marking changes. Unfortunately it's a pretty hands-on process, I can't really do it from phabricator, sorry!

In D134352#4398905, @jgilbert wrote:

To review changes to the reftest markings, I really need to see the treeherder test failures without the test marking changes. Unfortunately it's a pretty hands-on process, I can't really do it from phabricator, sorry!

@jgilbert this is the try-run I pushed two days ago, which only include part1~part3. Thank you.

In addition, above try-run was based on the newer m-c, but at the time I made this patch, I was based on some older m-c. I will check the fuzzy value again, and revise the value and update this patch again.

Yeah, it looks like the windows R-swr test are seeing /720p.png.bt709.bt709.pc.yuv420p10.vp9.webm/mp4 working correctly and av1 being scaled incorrectly on full-range (that's the "pc", instead of "tv")

I will update this patch again to update new fuzzy range, some of them are too large (based on some old codebase) and now I didn't see such a large difference on the try server (based on the latest m-c).

Thanks for taking an extra look at these!

The adjustements are based on these try-push [1]

[1]
https://treeherder.mozilla.org/jobs?repo=try&revision=d6737f754e54c83bcae002e595e4c3bcaf8aec79
https://treeherder.mozilla.org/jobs?repo=try&revision=6c05495a7284768863ef76b50a4ea0ec5b1d0ad2&selectedTaskRun=Y6c6iZf2SGSRNs2kREYziA.0

Code analysis found 242 defects in the diff 525554:

• 242 build errors found by clang-tidy

You can run this analysis locally with:

• (C/C++)

For your convenience, here is a patch that fixes all the clang-format defects (use it in your repository with or ).

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

Code analysis found 242 defects in the diff 526976:

• 242 build errors found by clang-tidy

You can run this analysis locally with:

• (C/C++)

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of , , , , . Tip: this Firefox add-on makes it easy!

Code analysis found 270 defects in the diff 529748:

• 270 build errors found by clang-tidy

You can run this analysis locally with:

• (C/C++)

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

Code analysis found 272 defects in the diff 529755:

• 272 build errors found by clang-tidy

You can run this analysis locally with:

• (C/C++)

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

Code analysis found 275 defects in the diff 530043:

• 275 build errors found by clang-tidy

You can run this analysis locally with:

• (C/C++)

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

Code analysis found 270 defects in the diff 530420:

• 270 build errors found by clang-tidy

You can run this analysis locally with:

• (C/C++)

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

Remove the changes for 10+ bits video, which are all related with SW WR on Windows (but now we won't enable shmem decoding for 10+ bits video on Windows, see 1751498)

Partial try run and a completed try run both look good.

Code analysis found 217 defects in the diff 531591:

• 217 build errors found by clang-tidy

You can run this analysis locally with:

• (C/C++)

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.

Code analysis found 268 defects in the diff 531598:

• 268 build errors found by clang-tidy

You can run this analysis locally with:

• (C/C++)

If you see a problem in this automated review, please report it here.

You can view these defects on the code-review frontend and on Treeherder.