1709945 - Full-range h264-mp4 videos are decoded as narrow-range because MediaChangeMonitor overlooks changes during creation

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect, P2)

Description

[Kelsey Gilbert [:jgilbert]]

Bug 1459526 fixes Webrender's decoding such that when it receives frames marked as full-range, it will display them properly.

However, my investigation shows that full-range h264-mp4 video decoding currently doesn't correctly tag frames they send to webrender as full-range.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: cq_16_127_235.720p.png

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: cq_16_127_235.720p.png.bt709.bt709.pc.yuv420p.h264.mp4

Comment 3

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Do you know if this happens regardless of platform? One of my first guesses is this could be platform decoder specific, and wonder if we have any differences there.

Platform specific points near the decode for h264 where full range is set to help investigation:

• Linux
• MacOS
• Windows

Comment 4

[Kelsey Gilbert [:jgilbert]]

I'm pretty sure I narrowed down where this is in this comment: https://bugzilla.mozilla.org/show_bug.cgi?id=1459526#c31

I have WR setup to handle full-range properly now, but WR's only being fed narrow-tagged frames.
It looks like while MediaChangeMonitor correctly sees the frame as full-range, but by the time we get to WMFVideoMFTManager::CreateD3DVideoFrame (via WMFMediaDataDecoder::ProcessDecode), MediaRawData has lost its mTrackInfo data, which ends up causing it to be treated with default space/range. (709narrow)

I beliiiieve that this is dropped when serializing MediaRawData across IPC, in particular in ArrayOfRemoteMediaRawData::Fill. The mExtraData does seem to make it across IPC, and mExtraData is actually what MediaChangeMonitor had used to identify full-range. Does WMFVideoMFTManager need to re-do the extra-data-decoding, or do we need to serialize (some of?) mTrackInfo across the wire?

Well, rather by WMFVideoMFTManager::CreateD3DVideoFrame, MediaRawData has lost its mTrackInfo data, which leaves us unable to pick up the correct narrow/full tagging.

Comment 5

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Thanks. Part of the reason I ask is that we use some of the same IPC for other codecs, so if this works for AV1 but not H264 it doesn't rule out the IPC, but may indicate there are other parts that are causing us problems.

I agree that it appears some sample information is being dropped across IPC, I think we're also facing another issue that I detail below. My working theory is that we may be able to get fullrange output without fixing the samples (at least with how the windows decoder works). We will, of course, want to fix up the sample issue also.

My hypothesis is that we're not correctly populating colour range even before IPC for some structures. We have some code to handle it, but, at least for h264, it's not working as expected. I also don't think the information is being populated correctly for av1 either, but I think the decoder is smart and figures it out itself.

Looking at different points in the pipeline for the example:

• The MeidaFormatReader has info that includes range when it create the decoder[0] -- the first arg to that function (*ownerData.GetCurrentInfo()->GetAsVideoInfo()) has a limited colour range.
• After that call, the MediaChangeMonitor does detect full range when parsing the sps[1], but doesn't appear to use that information when creating a decoder. It looks like while creating the H264ChangeMonitor we have it parse the data[2], but we still just use the aParams parsed to create our decoder[3], rather than using info from the change monitor.

[0] https://searchfox.org/mozilla-central/rev/a8773ba2a8f015e0525f219a7e55087b04d30258/dom/media/MediaFormatReader.cpp#382
[1] https://searchfox.org/mozilla-central/rev/a8773ba2a8f015e0525f219a7e55087b04d30258/dom/media/platforms/wrappers/MediaChangeMonitor.cpp#136
[2] https://searchfox.org/mozilla-central/rev/a8773ba2a8f015e0525f219a7e55087b04d30258/dom/media/platforms/wrappers/MediaChangeMonitor.cpp#261
[3] https://searchfox.org/mozilla-central/rev/a8773ba2a8f015e0525f219a7e55087b04d30258/dom/media/platforms/wrappers/MediaChangeMonitor.cpp#261

---

Here's a hack to test my hypothesis. I'd be interested to know if it has any affect. If it helps, the change monitor is not corrrectly forwarding full range (and maybe other data) to the decoders as expected (which will of course need to be fixed).

diff --git a/dom/media/platforms/wrappers/MediaChangeMonitor.cpp b/dom/media/platforms/wrappers/MediaChangeMonitor.cpp
--- a/dom/media/platforms/wrappers/MediaChangeMonitor.cpp
+++ b/dom/media/platforms/wrappers/MediaChangeMonitor.cpp
@@ -266,20 +266,21 @@ RefPtr<PlatformDecoderModule::CreateDeco
   if (!changeMonitor->CanBeInstantiated()) {
     // nothing found yet, will try again later
     return PlatformDecoderModule::CreateDecoderPromise::CreateAndResolve(
         new MediaChangeMonitor(aPDM, std::move(changeMonitor), nullptr,
                                aParams),
         __func__);
   }

+  CreateDecoderParams newParams = CreateDecoderParams(changeMonitor->Config());
   RefPtr<PlatformDecoderModule::CreateDecoderPromise> p =
-      aPDM->AsyncCreateDecoder(aParams)->Then(
+      aPDM->AsyncCreateDecoder(newParams)->Then(
           GetCurrentSerialEventTarget(), __func__,
-          [params = CreateDecoderParamsForAsync(aParams), pdm = RefPtr{aPDM},
+          [params = CreateDecoderParamsForAsync(newParams), pdm = RefPtr{aPDM},
            changeMonitor = std::move(changeMonitor)](
               RefPtr<MediaDataDecoder>&& aDecoder) mutable {
             RefPtr<MediaDataDecoder> decoder = new MediaChangeMonitor(
                 pdm, std::move(changeMonitor), aDecoder, params);
             return PlatformDecoderModule::CreateDecoderPromise::
                 CreateAndResolve(decoder, __func__);
           },
           [](MediaResult aError) {

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: narrow-range: 720p.png.bt709.bt709.tv.yuv420p.h264.mp4

Comment 7

[colinlee10]

@br(In reply to Bryce Seager van Dyk (:bryce) from comment #5)

Thanks. Part of the reason I ask is that we use some of the same IPC for other codecs, so if this works for AV1 but not H264 it doesn't rule out the IPC, but may indicate there are other parts that are causing us problems.

I agree that it appears some sample information is being dropped across IPC, I think we're also facing another issue that I detail below. My working theory is that we may be able to get fullrange output without fixing the samples (at least with how the windows decoder works). We will, of course, want to fix up the sample issue also.

My hypothesis is that we're not correctly populating colour range even before IPC for some structures. We have some code to handle it, but, at least for h264, it's not working as expected. I also don't think the information is being populated correctly for av1 either, but I think the decoder is smart and figures it out itself.

Looking at different points in the pipeline for the example:

• The MeidaFormatReader has info that includes range when it create the decoder[0] -- the first arg to that function (*ownerData.GetCurrentInfo()->GetAsVideoInfo()) has a limited colour range.
• After that call, the MediaChangeMonitor does detect full range when parsing the sps[1], but doesn't appear to use that information when creating a decoder. It looks like while creating the H264ChangeMonitor we have it parse the data[2], but we still just use the aParams parsed to create our decoder[3], rather than using info from the change monitor.

[0] https://searchfox.org/mozilla-central/rev/a8773ba2a8f015e0525f219a7e55087b04d30258/dom/media/MediaFormatReader.cpp#382
[1] https://searchfox.org/mozilla-central/rev/a8773ba2a8f015e0525f219a7e55087b04d30258/dom/media/platforms/wrappers/MediaChangeMonitor.cpp#136
[2] https://searchfox.org/mozilla-central/rev/a8773ba2a8f015e0525f219a7e55087b04d30258/dom/media/platforms/wrappers/MediaChangeMonitor.cpp#261
[3] https://searchfox.org/mozilla-central/rev/a8773ba2a8f015e0525f219a7e55087b04d30258/dom/media/platforms/wrappers/MediaChangeMonitor.cpp#261

---

Here's a hack to test my hypothesis. I'd be interested to know if it has any affect. If it helps, the change monitor is not corrrectly forwarding full range (and maybe other data) to the decoders as expected (which will of course need to be fixed).

diff --git a/dom/media/platforms/wrappers/MediaChangeMonitor.cpp b/dom/media/platforms/wrappers/MediaChangeMonitor.cpp
--- a/dom/media/platforms/wrappers/MediaChangeMonitor.cpp
+++ b/dom/media/platforms/wrappers/MediaChangeMonitor.cpp
@@ -266,20 +266,21 @@ RefPtr<PlatformDecoderModule::CreateDeco
   if (!changeMonitor->CanBeInstantiated()) {
     // nothing found yet, will try again later
     return PlatformDecoderModule::CreateDecoderPromise::CreateAndResolve(
         new MediaChangeMonitor(aPDM, std::move(changeMonitor), nullptr,
                                aParams),
         __func__);
   }

+  CreateDecoderParams newParams = CreateDecoderParams(changeMonitor->Config());
   RefPtr<PlatformDecoderModule::CreateDecoderPromise> p =
-      aPDM->AsyncCreateDecoder(aParams)->Then(
+      aPDM->AsyncCreateDecoder(newParams)->Then(
           GetCurrentSerialEventTarget(), __func__,
-          [params = CreateDecoderParamsForAsync(aParams), pdm = RefPtr{aPDM},
+          [params = CreateDecoderParamsForAsync(newParams), pdm = RefPtr{aPDM},
            changeMonitor = std::move(changeMonitor)](
               RefPtr<MediaDataDecoder>&& aDecoder) mutable {
             RefPtr<MediaDataDecoder> decoder = new MediaChangeMonitor(
                 pdm, std::move(changeMonitor), aDecoder, params);
             return PlatformDecoderModule::CreateDecoderPromise::
                 CreateAndResolve(decoder, __func__);
           },
           [](MediaResult aError) {

This patch fixes full range h264 for me.

Comment 8

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

(In reply to colinlee10 from comment #7)

<snip>

This patch fixes full range h264 for me.

Thanks, that's useful to know. Now that bug 1459526 has landed, it sounds like we could use some follow up here. I'll look at getting some time for this, as I don't immediately have cycles available.

Comment 9

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Looking at this now, stealing assignment. Working plan is a slightly more sophisticated version of the patch in comment 5.

Comment 10

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1709945 - Adjust color_quads reftest limits to pass on local machine. r?jgilbert

These limits allow these tests to give expected results on my local machine.
These results should not be impacted by gecko changes against this bug.

Comment 11

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1709945 - Change full range h264 test so it expects full range to work on windows. r?jgilbert

720p.png.bt709.bt709.pc.yuv420p.h264.mp4 should match the AV1 case on Windows
once bug 1709945 fixes our handling. Adjust the test to reflect this.

Depends on D119547

Comment 12

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1709945 - Correctly record changes in VideoInfo found during MediaChangeMonitor::Create. r?alwu

During MediaChangeMonitor::Create we create a CodecChangeMonitor and parse it an
initial config. Following this the CodecChangeMonitor may already have detected
changes.

Prior to this patch, if the CodecChangeMonitor detected any changes, we did no
pick up on those. This meant that things such as full range h264 video were not
being correctly identified.

This patch ensures that we update the config we pass to take into account the
VideoInfo from the CodecChangeMonitor.

Depends on D119548

Comment 13

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1709945 - Relax short.mp4 reftest swgl fuzzy-if pending investigation. r?jgilbert

Relax these tests so we can land the bug without having a perma fail. Bug
1720346 has been raised to investigate the issue further.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f114ef3c417
Adjust color_quads reftest limits to pass on local machine. r=jgilbert
https://hg.mozilla.org/integration/autoland/rev/2a6e35ffcc2d
Change full range h264 test so it expects full range to work on windows. r=jgilbert
https://hg.mozilla.org/integration/autoland/rev/fabdd8c9ea59
Correctly record changes in VideoInfo found during MediaChangeMonitor::Create. r=alwu
https://hg.mozilla.org/integration/autoland/rev/70b3062e3ea5
Relax short.mp4 reftest swgl fuzzy-if pending investigation. r=jgilbert

Comment 15

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/6f114ef3c417
https://hg.mozilla.org/mozilla-central/rev/2a6e35ffcc2d
https://hg.mozilla.org/mozilla-central/rev/fabdd8c9ea59
https://hg.mozilla.org/mozilla-central/rev/70b3062e3ea5

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Probably too late to take this in 91 at this point given where we are in the cycle, but is this bug high-impact enough that we'll want to eventually consider uplifting to ESR91?

Comment 17

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)

Probably too late to take this in 91 at this point given where we are in the cycle, but is this bug high-impact enough that we'll want to eventually consider uplifting to ESR91?

Since 91 will have bug 1459526's changes and since the changes on this bug are limited in scope, it makes sense we could uplift. I'll hold the NI while I touch base with other Media team folks, I'd like to make sure we're on the same page before I request uplift.

Comment 18

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Comment on attachment 9230532 [details]
Bug 1709945 - Correctly record changes in VideoInfo found during MediaChangeMonitor::Create. r?alwu

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Improves viewing experience of media.
• User impact if declined: Users watching certain full range videos will see incorrect colour values that result in a poor viewing experience.
• Fix Landed on Version: 92
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The code changes in this patch are relatively small. Much of the code to support this already landed in 91. 1 of the 4 patches in this stack changes functionality, the rest are tests.

We have reasonable ref test coverage here (including in this stack), so if this does regress, which I don't expect, they will likely catch the problem.

• String or UUID changes made by this patch: None

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9230530 [details]
Bug 1709945 - Adjust color_quads reftest limits to pass on local machine. r?jgilbert

It's early in the lifecycle for the new ESR, makes sense that we'd want to get this right from users updating from ESR78. Approved for 91.1esr.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/d25ca98c2528
https://hg.mozilla.org/releases/mozilla-esr91/rev/230a249c8f7f
https://hg.mozilla.org/releases/mozilla-esr91/rev/244b85f8afd4
https://hg.mozilla.org/releases/mozilla-esr91/rev/e3cf0b0f6948