flypig.co.uk

Today I'm changing tack but not changing gear. Over the last few days I've been working through issues that have come up when testing using Jolla's test pages. Now I'm going back to the testing list to try to tackle the final two items there: video and audio.

Although audio should — on the face of it — be the easiest, I'm going to start with video because it has a bigger impact and there's a chance that by fixing the video I'll fix the audio as well.

The plan of action is to find the appropriate patches, apply them and test the results. That sounds simple, but it's the application of the patches that worries me most: it's unlikely to be as smooth as I'd like it to be and if I have to start digging into individual lines of code, I'll quickly get out of my depth. So while I'm excited that we're approaching the end-game, I'm also a little nervous.

I think it's also worth mentioning that in a week's time we'll also have hit a full year of development in wall-time. That is, I started this task on the 16th August 2023. There have been a few breaks here and there, so by the time we get there I expect we'll be on day 321 rather than day 365. So it won't be a full year of work, but it will be a full rotation round the sun since this all started.

$ time gecko-dev

real    515520m0.002s
user    48984m0.002s
sys     20250m0.001s

Alright, it's time for some development. Today I'm going to attempt to apply video-related patches. Now that I've looked through them I can see there are quite a few. First the patches related to video:

1. 0045 - "Prioritize GMP plugins over all others, and support decoding video for h264, vp8 & vp9"
2. 0046 - "Force recycling of gmp-droid instances"
3. 0049 - "Force use of mobile video controls"
4. 0085 - "Fix video hardware accelaration not being used on first playback"
5. 0089 - "Add a video decoder based on gecko-camera"
6. 0094 - "Bug 1750760 Create ffmpeg59 module for ffmpeg5.0"
7. 0095 - "Bug 1750760 Open libavcodec.so.59 library and bind ffmpeg 5.0"
8. 0096 - "Bug 1750760 Update audio and video decoders to ffmpeg 5.0"
9. 0097 - "Bug 1761471 [FFmpeg 5.0] Get frame color range and color space directly"
10. 0098 - "Bug 1758948 [FFmpeg] Use AVFrame::pts instead of AVFrame::pkt_pts on ffmpeg 4.x"

There are also a couple of additional patches which relate specifically to audio:

1. 0066 - "Ensure audio continues when screen is locked"
2. 0084 - "Fix audio underruns for fullduplex mode"

I honestly don't know how easy (or hard) it's going to be to apply these patches. My suspicion is that I've not touched much of this code to make changes myself, which should reduce the likelihood of conflicts. On the other hand I've no idea the extent of any changes that may have been made to this code upstream. We'll just have to try it and see.

Sadly we're not off to a totally auspicious start.

$ git am --3way \
    ../rpm/0045-sailfishos-gecko-Prioritize-GMP-plugins-over-all-oth.patch
Applying: Prioritize GMP plugins over all others, and support decoding video 
    for h264, vp8 & vp9.
Using index info to reconstruct a base tree...
M       dom/media/platforms/PDMFactory.cpp
M       dom/media/platforms/agnostic/gmp/GMPDecoderModule.cpp
Falling back to patching base and 3-way merge...
Auto-merging dom/media/platforms/agnostic/gmp/GMPDecoderModule.cpp
Auto-merging dom/media/platforms/PDMFactory.cpp
CONFLICT (content): Merge conflict in dom/media/platforms/PDMFactory.cpp
error: Failed to merge in the changes.
Patch failed at 0001 Prioritize GMP plugins over all others, and support 
    decoding video for h264, vp8 & vp9.
[...]

Things have changed quite a bit in the code. What was previously all performed inside a single method like so:

void PDMFactory::CreatePDMs() {
  RefPtr<PlatformDecoderModule> m;
[...]
  if (StaticPrefs::media_rdd_process_enabled() &&
      BrowserTabsRemoteAutostart()) {
    m = new RemoteDecoderModule;
    StartupPDM(m);
  }

  if (StaticPrefs::media_gmp_decoder_enabled()) {
    m = new GMPDecoderModule();
    mGMPPDMFailedToStartup = !StartupPDM(m);
  } else {
    mGMPPDMFailedToStartup = false;
  }
[...]
}

Is now partitioned into multiple sub-methods like so:

void PDMFactory::CreatePDMs() {
[...]
  if (XRE_IsGPUProcess()) {
    CreateGpuPDMs();
  } else if (XRE_IsRDDProcess()) {
    CreateRddPDMs();
  } else if (XRE_IsContentProcess()) {
    CreateContentPDMs();
  } else {
    MOZ_DIAGNOSTIC_ASSERT(
        XRE_IsParentProcess(),
        "PDMFactory is only usable in the Parent/GPU/RDD/Content 
    process");
    CreateDefaultPDMs();
  }
}

Thankfully the code that actually needs changing appears to have been moved into the CreateDefaultPDMs() without too many other alterations besides, so I've been able to manually apply what look to me to be equivalent changes. The next three patches fare better:

$ git am --3way \
    ../rpm/0046-sailfishos-gecko-Force-recycling-of-gmp-droid-instan.patch
Applying: Force recycling of gmp-droid instances. JB#51730
Using index info to reconstruct a base tree...
M       dom/media/platforms/agnostic/gmp/GMPVideoDecoder.h
Falling back to patching base and 3-way merge...
Auto-merging dom/media/platforms/agnostic/gmp/GMPVideoDecoder.h
$ git am --3way ../rpm/
    0049-sailfishos-gecko-Force-use-of-mobile-video-controls..patch
Applying: Force use of mobile video controls. JB#55484 OMP#JOLLA-371
$ git am --3way ../rpm/
    0085-sailfishos-gecko-dev-Fix-video-hardware-accelaration.patch
Applying: Fix video hardware accelaration not being used on first playback. 
    JB#56630 OMP#JOLLA-568

But after that things get a bit more complex:

$ git am --3way \
    ../rpm/0089-sailfishos-gecko-Add-a-video-decoder-based-on-gecko-.patch
Applying: Add a video decoder based on gecko-camera. JB#56755
Using index info to reconstruct a base tree...
M       dom/media/platforms/PDMFactory.cpp
M       dom/media/platforms/moz.build
M       modules/libpref/init/StaticPrefList.yaml
M       toolkit/moz.configure
Falling back to patching base and 3-way merge...
Auto-merging toolkit/moz.configure
CONFLICT (content): Merge conflict in toolkit/moz.configure
Auto-merging modules/libpref/init/StaticPrefList.yaml
Auto-merging dom/media/platforms/moz.build
CONFLICT (content): Merge conflict in dom/media/platforms/moz.build
Auto-merging dom/media/platforms/PDMFactory.cpp
error: Failed to merge in the changes.
Patch failed at 0001 Add a video decoder based on gecko-camera. JB#56755

Thankfully the two conflicts turn out to be pretty straightforward to fix. The first in toolkit/moz.configure turns out to be something I changed in an earlier commit:

$ git log -1 1c02a359c3687
commit 1c02a359c368750f7b03987d13a4de842db94616
Author: David Llewellyn-Jones <david@flypig.co.uk>
Date:   Wed Aug 9 23:30:20 2023 +0100

    Add --with-embedlite flag
    
    Adds the --with-embedlite flag so that we can use the flag in
    embedding/embedlite/config/mozconfig.merqtxulrunner.

Since the changes I made earlier are identical to the changes in this patch, I can just skip them now. The second conflict in dom/media/platforms/moz.build is due to the classic "single quotes switched for double quotes" in the build files. This has tripped me up a few times before. In ESR 78 strings were delineated in build files using single quotes. In ESR 91 they've all been changed so that they're now delineated using double quotes. This plays havoc with the patcher which can no longer match any lines that happen to contain strings.

Thankfully, fixing it manually is straightforward if a little laborious. So with these two conflicts addressed the patch is now applied. The next one I'm applying is an audio one, simply because I thought it'd be better to apply them in some semblance of the same order. Thankfully it applies okay:

$ git am --3way \
    ../rpm/0084-sailfishos-gecko-Fix-audio-underruns-for-fullduplex-.patch
Applying: Fix audio underruns for fullduplex mode. JB#55461
Using index info to reconstruct a base tree...
A       media/libcubeb/src/cubeb_pulse.c
Falling back to patching base and 3-way merge...
Auto-merging third_party/rust/cubeb-sys/libcubeb/src/cubeb_pulse.c

The next one is not so happy:

$ git am --3way \
    ../rpm/0094-Bug-1750760-Create-ffmpeg59-module-for-ffmpeg5.0-r-a.patch
Applying: Bug 1750760 Create ffmpeg59 module for ffmpeg5.0 r=alwu
Using index info to reconstruct a base tree...
M       dom/media/platforms/ffmpeg/moz.build
M       tools/rewriting/ThirdPartyPaths.txt
${PROJECT}/.git/modules/gecko-dev/rebase-apply/patch:582: new blank line at EOF.
+
warning: 1 line adds whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging tools/rewriting/ThirdPartyPaths.txt
Auto-merging dom/media/platforms/ffmpeg/moz.build
CONFLICT (content): Merge conflict in dom/media/platforms/ffmpeg/moz.build
error: Failed to merge in the changes.
Patch failed at 0001 Bug 1750760 Create ffmpeg59 module for ffmpeg5.0 r=alwu

It's just the one build file, ffmpeg/moz.build, with a conflict and the fix is so simple that I can't even figure out why the patcher had a problem with it. There seems to be a bit of a pendulum swing with these, with the next now applying fine.

$ git am --3way \
    ../rpm/0095-Bug-1750760-Open-libavcodec.so.59-library-and-bind-f.patch
Applying: Bug 1750760 Open libavcodec.so.59 library and bind ffmpeg 5.0 symbols 
    r=alwu
Using index info to reconstruct a base tree...
M       dom/media/platforms/ffmpeg/FFmpegLibWrapper.cpp
M       dom/media/platforms/ffmpeg/FFmpegRuntimeLinker.cpp
Falling back to patching base and 3-way merge...
Auto-merging dom/media/platforms/ffmpeg/FFmpegRuntimeLinker.cpp
Auto-merging dom/media/platforms/ffmpeg/FFmpegLibWrapper.cpp

But then problems with the one after:

$ git am --3way \
    ../rpm/0096-Bug-1750760-Update-audio-and-video-decoders-to-ffmpe.patch
Applying: Bug 1750760 Update audio and video decoders to ffmpeg 5.0 r=alwu
Using index info to reconstruct a base tree...
M       dom/media/platforms/ffmpeg/FFmpegAudioDecoder.cpp
M       dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp
Falling back to patching base and 3-way merge...
Auto-merging dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp
CONFLICT (content): Merge conflict in dom/media/platforms/ffmpeg/
    FFmpegVideoDecoder.cpp
Auto-merging dom/media/platforms/ffmpeg/FFmpegAudioDecoder.cpp
error: Failed to merge in the changes.
Patch failed at 0001 Bug 1750760 Update audio and video decoders to ffmpeg 5.0 
    r=

This time it's actually managed to get properly confused, but it's still easy to fix with some manual inspection. Now, as if only to prove the pendulum pattern wrong, the next one has issues as well:

$ git am --3way \
    ../rpm/0097-Bug-1761471-FFmpeg-5.0-Get-frame-color-range-and-col.patch
Applying: Bug 1761471 [FFmpeg 5.0] Get frame color range and color space 
    directly r=alwu
Using index info to reconstruct a base tree...
M       dom/media/platforms/ffmpeg/FFmpegLibWrapper.cpp
M       dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp
M       dom/media/platforms/ffmpeg/FFmpegVideoDecoder.h
Falling back to patching base and 3-way merge...
Auto-merging dom/media/platforms/ffmpeg/FFmpegVideoDecoder.h
CONFLICT (content): Merge conflict in dom/media/platforms/ffmpeg/
    FFmpegVideoDecoder.h
Auto-merging dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp
CONFLICT (content): Merge conflict in dom/media/platforms/ffmpeg/
    FFmpegVideoDecoder.cpp
Auto-merging dom/media/platforms/ffmpeg/FFmpegLibWrapper.cpp
error: Failed to merge in the changes.
Patch failed at 0001 Bug 1761471 [FFmpeg 5.0] Get frame color range and color 
    space directly r=alwu

In both cases the problem is simply that a couple of the methods have now been marked as const which prevents the patcher from identifying them as the same line. Easily solved. The next one applies without incident:

$ git am --3way ../rpm/
    0098-Bug-1758948-FFmpeg-Use-AVFrame-pts-instead-of-AVFram.patch
Applying: Bug 1758948 [FFmpeg] Use AVFrame::pts instead of AVFrame::pkt_pts on 
    ffmpeg 4.x r=alwu

That leaves just one more audio patch to go.

$ git am --3way \
    ../rpm/0066-sailfishos-media-Ensure-audio-continues-when-screen-.patch
Applying: Ensure audio continues when screen is locked. Contributes to JB#51747
Using index info to reconstruct a base tree...
M       dom/html/HTMLMediaElement.cpp
M       dom/media/MediaDecoder.cpp
Falling back to patching base and 3-way merge...
Auto-merging dom/media/MediaDecoder.cpp
Auto-merging dom/html/HTMLMediaElement.cpp
CONFLICT (content): Merge conflict in dom/html/HTMLMediaElement.cpp
error: Failed to merge in the changes.
Patch failed at 0001 Ensure audio continues when screen is locked. Contributes 
    to JB#51747

It's not a totally clean application but the reason turns out to be just some functionality that's been moved out of a method and inlined into a condition. Once again, the fix appears to be pretty clear and clean on manual inspection.

So that's all of them: ten video and two audio patches applied. The next step is to build things and see how that's affected the audio and video functionality of the browser. Because the build files have changed it's going to be a full overnight rebuild, so we'll have to see how it's turned out in the morning.

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