List items
Items from the current list are shown below.
Gecko
11 Aug 2024 : Day 316 #
Last night I was so tired. I was really desperate to get the video patches building and it felt like there were only a few remaining fixes needed before the build would go through. My hope was that if I could get them done I'd have a completed build by morning. But I was just too tied. So here I am now the next day with the changes still to make, but with a lot more focus and much greater chance of getting it right.
The error I got last night looked like this:CreateAndStartupPDM() is used, or StartupPDM() is used. So let's look at what these two methods do. The latter looks like this:
Finally for the last of the errors, we now have to move the data object rather than copying it, in line with the changes shown in D78249. This leaves us with the following replacement line for adding the data to the queue:
I've set the full build running and am excited to see first whether the build goes through and second whether this improves the audio and video support. With any luck I'll have an answer by morning.
If you'd like to read any of my other gecko diary entries, they're all available on my Gecko-dev Diary page.
The error I got last night looked like this:
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 ^
The problem seems to be with the way the decoder module is being created. Previously the standard C++ constructor was used to instantiate the module, but if we take a look at change D54876 we can see that in ESR 91 it's no longer being done this way. The description of the relevant commit isn't super helpful:
$ git log -1 665d2ab6f565f
commit 665d2ab6f565fd446e1786563c94508a1a9f0251
Author: Dan Glastonbury <dan.glastonbury@gmail.com>
Date: Tue Oct 20 23:24:27 2020 +0000
Bug 1595994 - P3: Only create PDMs that are supported in the current
process. r=kamidphish
Depends on D52797
Differential Revision: https://phabricator.services.mozilla.com/D54876
In practice this means that a templated call to a start-up method is being used instead of the constructor directly. By comparing the code in the AppleDecoderModule.h and AppleDecoderModule.cpp files we can see that this is because a new reference-counted initialiser is being used:
/* static */
already_AddRefed<PlatformDecoderModule> AppleDecoderModule::Create() {
return MakeAndAddRef<AppleDecoderModule>();
}
Following the example of the other modules there I've made a similar change to the GeckoCameraDecoderModule class. Looking through the other differences between the old a new versions of the Apple decoder module code I can see that there's also this another new method that's been added to the class:
bool Supports(const SupportDecoderParams& aParams,
DecoderDoctorDiagnostics* aDiagnostics) const override;
So now I'm wondering if I need to add something along these lines to the GeckoCameraDecoderModule class as well. This Supports() method was added in a different commit to the Create() changes and checking the description that goes along with the commit, it looks like this is a change that's specific to the AppleDecoderModule():
$ git log -1 f3562546bb2b1
commit f3562546bb2b1f91957b0c38d9662562980bd939
Author: Jean-Yves Avenard <jyavenard@mozilla.com>
Date: Thu Aug 13 02:16:19 2020 +0000
Bug 1657521 - P5. Add VP9 HW decoder support on macOS 11 (Big Sur). r=jolin
To create a VP9 decoder, the VideoToolbox requires a vppC atom similar to
how the H264 one requires an avcC one.
That information is typically not available in the webm container and is
found in the VP9 bytestream with each keyframe.
In order to minimise the extent of the changes, we move the task of
retrieving the vpcC content in the MediaChangeMonitor as it already
performs a similar task in order to detect if the format has changed.
The VPXChangeMonitor will now only instantiate a VP9 decoder once a
keyframe is seen.
Differential Revision: https://phabricator.services.mozilla.com/D86544
I've therefore decided not to add this Supports() method to our version of the class. However, I do still need to update the PDMFactory class to make use of the new approach to reference-counted instantiation. It seems there are two ways to do this, depending on the ordering the modules should be selected. For example, the code that creates the Apple decoder module just looks like this:
CreateAndStartupPDM<AppleDecoderModule>();Contrast this with the code for the Android decoder module:
if (StaticPrefs::media_android_media_codec_enabled()) {
StartupPDM(AndroidDecoderModule::Create(),
StaticPrefs::media_android_media_codec_preferred());
}
It's not clear which of the two I should be using. The difference seems to be between whether
bool PDMFactory::StartupPDM(already_AddRefed<PlatformDecoderModule> aPDM,
bool aInsertAtBeginning) {
RefPtr<PlatformDecoderModule> pdm = aPDM;
if (pdm && NS_SUCCEEDED(pdm->Startup())) {
if (aInsertAtBeginning) {
mCurrentPDMs.InsertElementAt(0, pdm);
} else {
mCurrentPDMs.AppendElement(pdm);
}
return true;
}
return false;
}
So in this case the static Create() method is called to create the module directly with the result passed in as aPDM. There's then some code to determine whether or not the module should be inserted at the beginning or end of the module list. In contrast using CreateAndStartupPDM() is marginally cleaner in the calling code, but in practice ends up doing exactly the same thing and calling StartupPDM(). The only difference is that it doesn't then allow control over the ordering in the list, the aInsertAtBeginning variable being always set to the default of false:
template <typename DECODER_MODULE, typename... ARGS>
bool CreateAndStartupPDM(ARGS&&... aArgs) {
return StartupPDM(DECODER_MODULE::Create(std::forward<ARGS>(aArgs)...));
}
In our case we have a preference media_gecko_camera_codec_preferred() that's intended to control the ordering in the list, so we need to go with the first approach, which I think should look something like this:
if (StaticPrefs::media_gecko_camera_codec_enabled()) {
StartupPDM(GeckoCameraDecoderModule::Create(),
StaticPrefs::media_gecko_camera_codec_preferred());
}
Having made these changes I'm feeling pretty good about things, but on attempting a partial build just of the relevant files to save time, there are several more errors thrown up by the compiler.
${PROJECT}/gecko-dev/dom/media/platforms/gecko-camera/GeckoCameraVideoDecoder.h:
In member function ‘virtual nsCString mozilla::GeckoCameraVideoDecoder::
GetDescriptionName() const’:
${PROJECT}/gecko-dev/dom/media/platforms/gecko-camera/GeckoCameraVideoDecoder.h:
48:12: error: ‘NS_LITERAL_CSTRING’ was not declared in this scope
return NS_LITERAL_CSTRING("gecko-camera video decoder");
^~~~~~~~~~~~~~~~~~
[...]
${PROJECT}/gecko-dev/dom/media/platforms/gecko-camera/
GeckoCameraVideoDecoder.cpp: In constructor mozilla::
GeckoCameraVideoDecoder::GeckoCameraVideoDecoder(gecko::codec::
CodecManager*, const mozilla::CreateDecoderParams&)’:
${PROJECT}/gecko-dev/dom/media/platforms/gecko-camera/
GeckoCameraVideoDecoder.cpp:31:26: error: ‘const struct mozilla::
CreateDecoderParams’ has no member named ‘mTaskQueue’
mTaskQueue(aParams.mTaskQueue),
^~~~~~~~~~
${PROJECT}/gecko-dev/dom/media/platforms/gecko-camera/
GeckoCameraVideoDecoder.cpp: In member function ‘virtual void mozilla::
GeckoCameraVideoDecoder::onDecodedYCbCrFrame(const gecko::camera::
YCbCrFrame*)’:
${PROJECT}/gecko-dev/dom/media/platforms/gecko-camera/
GeckoCameraVideoDecoder.cpp:161:21: error: ‘struct mozilla::VideoData::
YCbCrBuffer::Plane’ has no member named ‘mOffset’
buffer.mPlanes[0].mOffset = 0;
^~~~~~~
[...]
${PROJECT}/gecko-dev/dom/media/platforms/gecko-camera/
GeckoCameraVideoDecoder.cpp:190:22: error: cannot bind rvalue reference of
type ‘RefPtr<mozilla::MediaData>&&’ to lvalue of type ‘RefPtr<mozilla::
MediaData>’
mReorderQueue.Push(data);
^~~~
[...]
To fix the first of these I just have to use the new ESR 91 string-literal annotation, which has come up multiple times now. For the mTaskQueue error, checking the changes in other modules it becomes clear that this is no longer passed in as part of the mParams structure. Instead it now needs to be created directly in the constructor:
mTaskQueue(new TaskQueue(
GetMediaThreadPool(MediaThreadType::PLATFORM_DECODER),
"GeckoCameraVideoDecoder")),
I thought maybe there'd be some change tot he destruction code as well, but I don't see anything, or indeed any other consequences from this change, so it's a pretty simple fix. The mOffset error is also a case of the element having been removed from one of the structures. Following the upstream changes that we can see in changeset D78320 it looks like it should be safe just to remove the lines causing the errors entirely.Finally for the last of the errors, we now have to move the data object rather than copying it, in line with the changes shown in D78249. This leaves us with the following replacement line for adding the data to the queue:
mReorderQueue.Push(std::move(data));With these changes made the partial build goes through, so we should be ready for a successful full build. I've committed the changes and merged them in to the existing commit that applied patch 0089 "Add a video decoder based on gecko-camera", since this is the commit that added all of these Gecko Camera files to the build.
I've set the full build running and am excited to see first whether the build goes through and second whether this improves the audio and video support. With any luck I'll have an answer by morning.
If you'd like to read any of my other gecko diary entries, they're all available on my Gecko-dev Diary page.
Comments
Uncover Disqus comments