1255626 - MP4Demuxer gtest will hang if an error occurs.

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

The MP4DemuxerBinding runs the test and then call mTaskQueue->AwaitShutdownAndIdle();

However, in case of error, the task queue is never shutdown.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: MozReview Request: Bug 1255626: [gtest] Properly shutdown task queue should error occurs. r?gerald

Review commit: https://reviewboard.mozilla.org/r/39313/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39313/

Comment 2

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

Comment on attachment 8729255 [details]
MozReview Request: Bug 1255626: [gtest] Properly shutdown task queue should error occurs. r?gerald

https://reviewboard.mozilla.org/r/39313/#review36025

r+

Check-in comment: "should error occurs" -> "should error occur" or "if error occurs".

Optional nits:

::: dom/media/gtest/TestMP4Demuxer.cpp:64
(Diff revision 1)
> -    RefPtr<MP4DemuxerBinding> self = this;
> +    RefPtr<MP4DemuxerBinding> binding = this;

You could move this line down, closer to the DispatchTask, as it wouldn't even be needed in case of an early return (line 79).

::: dom/media/gtest/TestMP4Demuxer.cpp:275
(Diff revision 1)
> -        [binding, video] () {
> +        [binding, &video] () {

'video' is a pointer, and you don't modify it in here, so I don't see an advantage in capturing a pointer by reference.
Is it because of ArrayLength?
But in this context, 'video' comes from a by-value capture in the outer lambda starting at line 270, so again I'm not sure about the difference.
But in the end, if it works, who cares? :-)

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fddbd0acfb1d

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fddbd0acfb1d