flypig.co.uk

I left the build running overnight after having applied an almighty tranche of audio and video patches yesterday. Twelve patches I applied in total. Fairly soon after the build had started, but in the middle of the night by the time I noticed, the build failed due to a Rust checksum error:

13:39.17 error: the listed checksum of `${PROJECT}/gecko-dev/third_party/rust/
    cubeb-sys/libcubeb/src/cubeb_pulse.c` has changed:
13:39.17 expected: 
    c20283e17cb8a893ea5e2588e051112462726e531d5f481169cff06d5254f7ec
13:39.17 actual:   
    d5d869d39e598870c64fbc256948d77e32b5ab69744862fae4b903f589d66878
13:39.17 directory sources are not intended to be edited, if modifications are 
    required then it is recommended that [replace] is used with a forked copy 
    of the source
13:39.18 make[4]: *** [${PROJECT}/gecko-dev/config/makefiles/rust.mk:405: 
    force-cargo-library-build] Error 101

After editing the third_party/rust/cubeb-sys/.cargo-checksum.json to change the checksum I then restarted the build. By the morning the build had finished, but not completed, having hit another — more serious — error further down the line:

96:11.26 In file included from Unified_cpp_dom_media_platforms0.cpp:11:
96:11.26 ${PROJECT}/gecko-dev/dom/media/platforms/PDMFactory.cpp: In member 
    function ‘void mozilla::PDMFactory::CreateDefaultPDMs()’:
96:11.26 ${PROJECT}/gecko-dev/dom/media/platforms/PDMFactory.cpp:606:5: error: 
    ‘m’ was not declared in this scope
96:11.26      m = new GeckoCameraDecoderModule();
96:11.26      ^

The change causing the error was made by applying patch 0089 "Add a video decoder based on gecko-camera". The issue here is that while m was a local variable defined at the top of the PDMFactory::CreatePDMs() method in the ESR 78 code, it's been removed completely from ESR 91. Here it is in the ESR 78 code:

void PDMFactory::CreatePDMs() {
  RefPtr<PlatformDecoderModule> m;
[...]
}

Here's the change as it's been applied by the patcher to the ESR 91 code:

@@ -594,6 +601,12 @@ void PDMFactory::CreateDefaultPDMs() {
                StaticPrefs::media_android_media_codec_preferred());
   }
 #endif
+#ifdef MOZ_EMBEDLITE
+  if (StaticPrefs::media_gecko_camera_codec_enabled()) {
+    m = new GeckoCameraDecoderModule();
+    StartupPDM(m, StaticPrefs::media_gecko_camera_codec_preferred());
+  }
+#endif
 
   CreateAndStartupPDM<AgnosticDecoderModule>();
 }

Although patch 0089 didn't apply cleanly, this particular change wasn't part of the conflicted code, so I'm not totally surprised that I missed it. Nevertheless now it's been highlighted it's clear why this would cause an error given m has disappeared. Luckily we have some related changes in the surrounding code that we can compare against. Take for example the code used to initialise the media codec module on Android. In ESR 78 it looked like this:

  if (StaticPrefs::media_android_media_codec_enabled()) {
    m = new AndroidDecoderModule();
    StartupPDM(m, StaticPrefs::media_android_media_codec_preferred());
  }

On ESR 91 this was changed to the following:

  if (StaticPrefs::media_android_media_codec_enabled()) {
    StartupPDM(AndroidDecoderModule::Create(),
               StaticPrefs::media_android_media_codec_preferred());
  }

There are similar changes for AppleMedia, FFmpeg, FFVPX, QNX and Windows. All that's happening is that, rather than capture the module instance in a temporary variable, it's being passed straight in to the StartupPDM() method as a parameter. I reckon we can do the same thing for our EmbedLite change. So after a bit of reworking the code, I now have this:

@@ -594,6 +601,12 @@ void PDMFactory::CreateDefaultPDMs() {
                StaticPrefs::media_android_media_codec_preferred());
   }
 #endif
+#ifdef MOZ_EMBEDLITE
+  if (StaticPrefs::media_gecko_camera_codec_enabled()) {
+    StartupPDM(GeckoCameraDecoderModule(),
+               StaticPrefs::media_gecko_camera_codec_preferred());
+  }
+#endif
 
   CreateAndStartupPDM<AgnosticDecoderModule>();
 }

Now I just have to do some careful git rebasing to ensure this change gets integrated into the correct patch. To do this I've added the changes to a new commit applied to HEAD. I then perform an interactive rebase that covers both this new patch and the original patch 0089 that I want to integrate my changes into. I then move the new commit to just after patch 0089 and set it to squash downwards on top of it.

The result is, I hope, a nice clean tree and a build that compiles. To find out whether we've achieved the latter I'm going to have to run the build, which will likely take the rest of the day, so I'm going to get that started straight away.

[...]

Unfortunately we reached the end of the day without a successful build. The following error was thrown up, resulting from changes to the way the PlatformDecoderModule subclasses are instantiated and managed.

116:46.05 In file included from Unified_cpp_dom_media_platforms0.cpp:11:
116:46.05 ${PROJECT}/gecko-dev/dom/media/platforms/PDMFactory.cpp: In member 
    function ‘void mozilla::PDMFactory::CreateDefaultPDMs()’:
116:46.05 ${PROJECT}/gecko-dev/dom/media/platforms/PDMFactory.cpp:607:65: error:
     no matching function for call to ‘mozilla::PDMFactory::StartupPDM(mozilla::
    GeckoCameraDecoderModule, mozilla::StripAtomic<mozilla::Atomic<bool, (
    mozilla::MemoryOrdering)0> >)’
116:46.05                 StaticPrefs::media_gecko_camera_codec_preferred());
116:46.05                                                                  ^

Unfortunately it's already late and I'm too tired to figure out how to fix it. I'm going to have to compare changes made between ESR 78 and ESR 91 for other decoder modules similar to ours. That means I'll have to pick this up in the morning to find out exactly how the code needs changing.

If you'd like to read any of my other gecko diary entries, they're all available on my Gecko-dev Diary page.