1248308 - Fix non-unified build in dom/media

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Found while trying to add a file to UNIFIED_SOURCES in dom/media/moz.build.

Changing "UNIFIED_SOURCES" to "SOURCES" reveals many errors in DOMMediaStream.cpp, dom/media/MediaDevices.cpp, dom/media/MediaFormatReader.cpp, dom/media/MediaManager.cpp, dom/media/MediaRecorder.cpp, dom/media/RtspMediaResource.cpp.
(Mostly due to missing #include's, and bleeding using's.)

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Working on a fix.

Try with non-unified build, to check that each file may be built separately:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=79c6bc08615c

Note: There is still an issue with a false alarm about unreachable code in AudioStream.cpp, but I'm unclear why it appears only in non-unified builds; and I don't have the time to investigate it now; and I don't think this issue is likely to show up soon when adding files (because AudioStream.cpp is near the beginning of a unified group). So I will not work on it further right now.

Try with unified build, to make sure it still works:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf611fe809e0

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1248308-fix-dom-media-unified-build.patch

Fix dom/media non-unified build errors.

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Gerald Squelart [:gerald] from comment #1)
> Note: There is still an issue with a false alarm about unreachable code in
> AudioStream.cpp, but I'm unclear why it appears only in non-unified builds;
> and I don't have the time to investigate it now; and I don't think this
> issue is likely to show up soon when adding files (because AudioStream.cpp
> is near the beginning of a unified group). So I will not work on it further
> right now.

I don't think they are false positives. AUDIO_OUTPUT_FORMAT is a compile time constant which is either AUDIO_FORMAT_S16 or AUDIO_FORMAT_FLOAT32 depending how MOZ_SAMPLE_TYPE_S16 is defined. It is easy to fix them using some TMP tricks.

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to JW Wang [:jwwang] from comment #3)
> (In reply to Gerald Squelart [:gerald] from comment #1)
> > Note: There is still an issue with a false alarm about unreachable code in
> > AudioStream.cpp, [...]
> 
> I don't think they are false positives. AUDIO_OUTPUT_FORMAT is a compile
> time constant which is either AUDIO_FORMAT_S16 or AUDIO_FORMAT_FLOAT32
> depending how MOZ_SAMPLE_TYPE_S16 is defined.

Maybe "false positive" was the wrong term.
I meant that the constant-equality-tests are intended in these cases, assuming that the compiler will optimize away one of the branches.

> It is easy to fix them using some TMP tricks.

How do you do that, please?

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Do you mean for the code like:

if (0 == 1) {
  // do something.
} else (0 == 0) {
  // do some other things.
}

the compiler will eliminate the (0 == 1) branch?

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Yes.
Or even with a single if-branch, e.g.: http://mxr.mozilla.org/mozilla-central/source/dom/media/AudioStream.cpp#284
>   if (AUDIO_OUTPUT_FORMAT == AUDIO_FORMAT_S16) {
>     fwrite(aBuffer, 2, samples, aDumpFile);        // <-- Compiler complains about unreachable code here.
>     return;
>   }

Comment 7

[Jean-Yves Avenard [:jya]]

Windows in particular will complain for even something like:

#define foo 0

if (foo) {
  ....
}

and even if a function is called, it will cause a linkage error.

Had the issue with code like:
if (HAVE_IOCTL) {
  ioctl(...);
}

and getting linkage error that ioctl couldn't be found

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

I guess |if (HAVE_IOCTL)| is not optimized away in debug builds?

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Gerald Squelart [:gerald] from comment #4)
> How do you do that, please?

Here is an attempt.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbbfed2a82cb
https://hg.mozilla.org/try/rev/6c3d5c785ea3

Comment 10

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to JW Wang [:jwwang] from comment #3)
> [...] It is easy to fix them using some TMP tricks.

(In reply to JW Wang [:jwwang] from comment #9)
> Here is an attempt.
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbbfed2a82cb
> https://hg.mozilla.org/try/rev/6c3d5c785ea3

Uh, what's your definition of "easy"? :-)

Anyway, how about we go ahead with my proposed (partial) fix for now, so I can move on with more important stuff; and I can open a separate bug to deal with these harmless warnings/errors later on?

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

Sure. I will be happy to fix some minor bugs when waiting for the build. :)

Comment 12

[Jean-Yves Avenard [:jya]]

Comment on attachment 8719388 [details] [diff] [review]
1248308-fix-dom-media-unified-build.patch

Review of attachment 8719388 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for the missing headers.

I would much prefer you to add using dom; using media; etc whenever a namespace was missing.

makes the code harder to read now.

::: dom/media/MediaManager.cpp
@@ +104,5 @@
>  const nsIID nsIMediaDevice::COMTypeInfo<mozilla::AudioDevice, void>::kIID = NS_IMEDIADEVICE_IID;
>  
>  namespace {
>  already_AddRefed<nsIAsyncShutdownClient> GetShutdownPhase() {
> +  nsCOMPtr<nsIAsyncShutdownService> svc = mozilla::services::GetAsyncShutdown();

would make more sense to have using mozilla and using dom in this file.

it looks ugly now.

Comment 13

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Thank you for the review.

(In reply to Jean-Yves Avenard [:jya] from comment #12)
> I would much prefer you to add using dom; using media; etc whenever a
> namespace was missing.

I'm not a fan of blanket using's, they pollute other files in unified-build groups (one reason why this bug exists!).
I'll add specific "using mozilla::services"&others instead, not perfect but a bit less naughty.

Comment 14

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Hmm, I see that the coding style is fine with "using namespace ..." in cpp files.
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Namespaces

And there are a lot of these around already.

So maybe I'll just take the easier path to the dark side... What could possibly go wrong, eh?

Comment 15

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1248308-fix-dom-media-unified-build.patch

Updated code as per review in comment 12. Carrying r+.
Try with non-unified build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=932907b9aee7
Try with unified build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b45737a9c99

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/932ebb82c298

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/932ebb82c298