1839993 - FFmpegDataDecoder::ProcessDrain() should return error if needed

Status: RESOLVED FIXED

(Core :: Audio/Video: Web Codecs, task, P1)

Description

[C.M.Chang[:chunmin]]

ProcessDrain() ignores all errors in its pipeline and blindly resolve the DecodePromise, which cause problems in WebCodecs when an decoding-error is expected.

For example, given two encoded video frame f1 and f2. f1 is a valid key frame while f2 is a corrupt frame.

• When Decode(f1) is called, f1 stocks in the pipeline and then resolve the DecodePromise with an empty data
• When Decode(f2) is called, decoded f1 is push out of the pipeline, returned in a resolved DecodePromise
• When Drain() is called, the corrupt f2 is unable to be decoded so it returns a decode error. However, the ProcessDrain() will ignore this error, and resolve the DecodePromise with an empty data

In WebCodecs, an EncodingError is expected to be dispatched to the error-callback in this case. FFmpegDataDecoder::ProcessDrain() should not ignore the decode error.

The current workaround is to always enable low-latency so the data can be decoded immediately instead of being stocked in the pipeline.

Comment 1

[C.M.Chang[:chunmin]]

My concern is that fixing this may cause some regressions since all our media pipeline ignores this kind of error for quite some time.

Comment 2

[C.M.Chang[:chunmin]]

Attachment: Bug 1839993 - Return non-EOS errors in FFmpegDataDecoder::Drain

Comment 3

[C.M.Chang[:chunmin]]

It seems returning the error breaks lot of things: https://treeherder.mozilla.org/jobs?repo=try&revision=e85c79108f0dff77506a6f87a0bad8fc494a0949&selectedTaskRun=WU5gMuNmQPuQ56uf8pcr7Q.0.

The alternative is to have an parameter indicating if the decode error should be ignored in Drain(aIgnoreError = true). The default value can be true so most of the code won't be changed, and we don't set it to false in WebCodecs case.

Comment 4

[C.M.Chang[:chunmin]]

(In reply to C.M.Chang[:chunmin] from comment #3)

It seems returning the error breaks lot of things: https://treeherder.mozilla.org/jobs?repo=try&revision=e85c79108f0dff77506a6f87a0bad8fc494a0949&selectedTaskRun=WU5gMuNmQPuQ56uf8pcr7Q.0.

The alternative is to have an parameter indicating if the decode error should be ignored in Drain(aIgnoreError = true). The default value can be true so most of the code won't be changed, and we don't set it to false in WebCodecs case.

pernos record

https://pernos.co/self-service-api/mozilla/BVHwL5PJQ0C8AMTl-Zu6Fg/self-service.html

Comment 5

[C.M.Chang[:chunmin]]

Attachment: WIP: Bug 1839993 - Add an option in Drain() to ignore error

This patch introduces a parameter in MediaDataDecoder::Drain() to allow
for an options to disregard errors when draining decoder data.

Although the default parameter is configured to ignore errors, which
enables callers to invoke Drain() without setting the parameter, the
wrapper/proxy classes - MediaDataDecoderProxy, AllocationWrapper,
MediaChangeMonitor, RemoteMediaDataDecoder, and AudioTrimmer - will
propagate their Drain's parameter to the respective underlying
MediaDataDecoder instances.

Comment 6

[C.M.Chang[:chunmin]]

(In reply to C.M.Chang[:chunmin] from comment #3)

It seems returning the error breaks lot of things: https://treeherder.mozilla.org/jobs?repo=try&revision=e85c79108f0dff77506a6f87a0bad8fc494a0949&selectedTaskRun=WU5gMuNmQPuQ56uf8pcr7Q.0.

The alternative is to have an parameter indicating if the decode error should be ignored in Drain(aIgnoreError = true). The default value can be true so most of the code won't be changed, and we don't set it to false in WebCodecs case.

Maybe it's because the EOF error from avcodec_send_packet in FFmpegVideoDecoder returns NS_ERROR_DOM_MEDIA_DECODE_ERR, instead of NS_ERROR_DOM_MEDIA_END_OF_STREAM. The FFmpegAudioDecoder returns NS_ERROR_DOM_MEDIA_END_OF_STREAM in this case. If that's the cause of the failure, D205780 can be removed.

Comment 7

[C.M.Chang[:chunmin]]

Attachment: Bug 1839993 - Remove mandatory low-latency mode for WebCodecs decoders

Previously, to address the issue of FFmpegDataDecoder always ignoring
errors during the draining process, WebCodecs decoders were forced into
low-latency mode as a workaround to ensure that expected errors for
corrupt/invalid data were generated in WPTs. With upcoming changes to
ensure that the Drain() API of decoders return errors when its
underlying API fails unexpectedly, this patch eliminates this compulsion
and set the low-latency mode only when explicitly requested by the user.

Comment 8

[C.M.Chang[:chunmin]]

Attachment: Bug 1839993 - Align EOS error handling in FFmpegVideoDecoder with FFmpegAudioDecoder

The avcodec_send_packet function returns AVERROR_EOF when the
decoder has been flushed. In this scenario, FFmpegAudioDecoder returns
NS_ERROR_DOM_MEDIA_END_OF_STREAM, while FFmpegVideoDecoder returns
NS_ERROR_DOM_MEDIA_DECODE_ERR. To ensure consistent behavior between
the two decoders, this patch adjusts FFmpegVideoDecoder to return
NS_ERROR_DOM_MEDIA_END_OF_STREAM in such case.

In addition, this modification ensures expected funtionality of the
modifications introduced in the preivous patch.

Depends on D181829

Comment 9

[C.M.Chang[:chunmin]]

Attachment: Bug 1839993 - Alphabetize headers in DecoderTemplate.h

Depends on D206448

Comment 10

[C.M.Chang[:chunmin]]

Attachment: Bug 1839993 - Refine variable names and their comments

This patch renames the mId member in DecodeMessage and FlushMessage to
mSeqId and updates their comments accordingly to emphasize that these
IDs are sequence numbers rather than unique IDs.

Depends on D206932

Comment 11

[C.M.Chang[:chunmin]]

Attachment: Bug 1839993 - Allow Reset() to reject promises during Flush()'s callback

Previously, Reset() lacked the ability to reject promises of the pending
flush requests during the output callback responsible for delivering the
result request (e.g., [1]). This limitation arose because once the
promise was transferred from the FlushMessage to the task delivering the
flush's result, there was no external access to that flush's promise
outside of the dispatched task.

This patch addresses this issue by implementing pending flush promises
as outlined in the WebCodecs spec, albeit with a slight modification,
allowing access to any stored pending promise as long as their
associated ID is provided. Conseqently, Reset() gains the capability to
reject all pending promises, while tasks with the corresponding IDs can
retrieve the promises to resolve or reject them based on their
requirements.

Depends on D206933

Comment 12

[C.M.Chang[:chunmin]]

Attachment: Bug 1839993 - Add a ForEach function in SimpleMap

This patch introduces a new function that iterates throught all elements
of the SimpleMap and invokes the provided function for each element. The
provided function can get the reference of the elements during
iteration.

Depends on D206933

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f276ba336bb3
Remove mandatory low-latency mode for WebCodecs decoders r=media-playback-reviewers,padenot
https://hg.mozilla.org/integration/autoland/rev/698fe5fca343
Return non-EOS errors in FFmpegDataDecoder::Drain r=media-playback-reviewers,padenot
https://hg.mozilla.org/integration/autoland/rev/1a3261a15a3c
Align EOS error handling in FFmpegVideoDecoder with FFmpegAudioDecoder r=media-playback-reviewers,alwu
https://hg.mozilla.org/integration/autoland/rev/3dad9fed9cde
Alphabetize headers in DecoderTemplate.h r=media-playback-reviewers,padenot
https://hg.mozilla.org/integration/autoland/rev/fd18bad7b5f0
Refine variable names and their comments r=media-playback-reviewers,padenot
https://hg.mozilla.org/integration/autoland/rev/ed34cc115fc8
Add a ForEach function in SimpleMap r=media-playback-reviewers,padenot
https://hg.mozilla.org/integration/autoland/rev/a2771b11654c
Allow Reset() to reject promises during Flush()'s callback r=media-playback-reviewers,padenot

Comment 14

[Natalia Csoregi [:nataliaCs]]

Backed out for causing build bustages on DecoderTemplate.cpp

• backout: https://hg.mozilla.org/integration/autoland/rev/ccb9c8f23156006e2ab90a628e467c03809f53b0
• push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=a2771b11654cb41511f0daa478c500ec4e7ca414
• failure log: https://treeherder.mozilla.org/logviewer?job_id=454935894&repo=autoland&lineNumber=57796

[task 2024-04-18T02:23:09.877Z] 02:23:09     INFO -  In file included from Unified_cpp_dom_media_webcodecs0.cpp:38:
[task 2024-04-18T02:23:09.878Z] 02:23:09    ERROR -  /builds/worker/checkouts/gecko/dom/media/webcodecs/DecoderTemplate.cpp:232:60: error: data argument not used by format string [-Werror,-Wformat-extra-args]
[task 2024-04-18T02:23:09.879Z] 02:23:09     INFO -    231 |   LOG("%s %p enqueues %s, with unique id " PRId64, DecoderType::Name.get(),
[task 2024-04-18T02:23:09.879Z] 02:23:09     INFO -        |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2024-04-18T02:23:09.879Z] 02:23:09     INFO -    232 |       this, mControlMessageQueue.back()->ToString().get(), flushPromiseId);
[task 2024-04-18T02:23:09.879Z] 02:23:09     INFO -        |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
[task 2024-04-18T02:23:09.879Z] 02:23:09     INFO -  /builds/worker/checkouts/gecko/dom/media/webcodecs/DecoderTemplate.cpp:40:50: note: expanded from macro 'LOG'
[task 2024-04-18T02:23:09.879Z] 02:23:09     INFO -     40 | #define LOG(msg, ...) LOG_INTERNAL(Debug, msg, ##__VA_ARGS__)
[task 2024-04-18T02:23:09.880Z] 02:23:09     INFO -        |                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
[task 2024-04-18T02:23:09.880Z] 02:23:09     INFO -  /builds/worker/checkouts/gecko/dom/media/webcodecs/DecoderTemplate.cpp:35:51: note: expanded from macro 'LOG_INTERNAL'
[task 2024-04-18T02:23:09.880Z] 02:23:09     INFO -     35 |   MOZ_LOG(gWebCodecsLog, LogLevel::level, (msg, ##__VA_ARGS__))
[task 2024-04-18T02:23:09.880Z] 02:23:09     INFO -        |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
[task 2024-04-18T02:23:09.880Z] 02:23:09     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Logging.h:290:56: note: expanded from macro 'MOZ_LOG'
[task 2024-04-18T02:23:09.880Z] 02:23:09     INFO -    290 |                                    MOZ_LOG_EXPAND_ARGS _args); \
[task 2024-04-18T02:23:09.880Z] 02:23:09     INFO -        |                                    ~~~~~~~~~~~~~~~~~~~~^~~~~
[task 2024-04-18T02:23:09.880Z] 02:23:09     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Logging.h:227:34: note: expanded from macro 'MOZ_LOG_EXPAND_ARGS'
[task 2024-04-18T02:23:09.880Z] 02:23:09     INFO -    227 | #define MOZ_LOG_EXPAND_ARGS(...) __VA_ARGS__
[task 2024-04-18T02:23:09.881Z] 02:23:09     INFO -        |                                  ^~~~~~~~~~~
[task 2024-04-18T02:23:09.881Z] 02:23:09     INFO -  /builds/worker/checkouts/gecko/dom/media/webcodecs/DecoderTemplate.cpp:897:16: note: in instantiation of member function 'mozilla::dom::DecoderTemplate<mozilla::dom::VideoDecoderTraits>::Flush' requested here
[task 2024-04-18T02:23:09.881Z] 02:23:09     INFO -    897 | template class DecoderTemplate<VideoDecoderTraits>;
[task 2024-04-18T02:23:09.881Z] 02:23:09     INFO -        |                ^
[task 2024-04-18T02:23:09.882Z] 02:23:09    ERROR -  /builds/worker/checkouts/gecko/dom/media/webcodecs/DecoderTemplate.cpp:232:60: error: data argument not used by format string [-Werror,-Wformat-extra-args]
[task 2024-04-18T02:23:09.882Z] 02:23:09     INFO -    231 |   LOG("%s %p enqueues %s, with unique id " PRId64, DecoderType::Name.get(),
[task 2024-04-18T02:23:09.882Z] 02:23:09     INFO -        |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2024-04-18T02:23:09.882Z] 02:23:09     INFO -    232 |       this, mControlMessageQueue.back()->ToString().get(), flushPromiseId);
[task 2024-04-18T02:23:09.883Z] 02:23:09     INFO -        |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
[task 2024-04-18T02:23:09.883Z] 02:23:09     INFO -  /builds/worker/checkouts/gecko/dom/media/webcodecs/DecoderTemplate.cpp:40:50: note: expanded from macro 'LOG'
[task 2024-04-18T02:23:09.883Z] 02:23:09     INFO -     40 | #define LOG(msg, ...) LOG_INTERNAL(Debug, msg, ##__VA_ARGS__)
[task 2024-04-18T02:23:09.884Z] 02:23:09     INFO -        |                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
[task 2024-04-18T02:23:09.884Z] 02:23:09     INFO -  /builds/worker/checkouts/gecko/dom/media/webcodecs/DecoderTemplate.cpp:35:51: note: expanded from macro 'LOG_INTERNAL'
[task 2024-04-18T02:23:09.884Z] 02:23:09     INFO -     35 |   MOZ_LOG(gWebCodecsLog, LogLevel::level, (msg, ##__VA_ARGS__))
[task 2024-04-18T02:23:09.884Z] 02:23:09     INFO -        |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
[task 2024-04-18T02:23:09.885Z] 02:23:09     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Logging.h:290:56: note: expanded from macro 'MOZ_LOG'
[task 2024-04-18T02:23:09.885Z] 02:23:09     INFO -    290 |                                    MOZ_LOG_EXPAND_ARGS _args); \
[task 2024-04-18T02:23:09.885Z] 02:23:09     INFO -        |                                    ~~~~~~~~~~~~~~~~~~~~^~~~~
[task 2024-04-18T02:23:09.885Z] 02:23:09     INFO -  /builds/worker/workspace/obj-build/dist/include/mozilla/Logging.h:227:34: note: expanded from macro 'MOZ_LOG_EXPAND_ARGS'
[task 2024-04-18T02:23:09.886Z] 02:23:09     INFO -    227 | #define MOZ_LOG_EXPAND_ARGS(...) __VA_ARGS__
[task 2024-04-18T02:23:09.886Z] 02:23:09     INFO -        |                                  ^~~~~~~~~~~
[task 2024-04-18T02:23:09.887Z] 02:23:09     INFO -  /builds/worker/checkouts/gecko/dom/media/webcodecs/DecoderTemplate.cpp:898:16: note: in instantiation of member function 'mozilla::dom::DecoderTemplate<mozilla::dom::AudioDecoderTraits>::Flush' requested here
[task 2024-04-18T02:23:09.887Z] 02:23:09     INFO -    898 | template class DecoderTemplate<AudioDecoderTraits>;
[task 2024-04-18T02:23:09.887Z] 02:23:09     INFO -        |                ^
[task 2024-04-18T02:23:09.888Z] 02:23:09     INFO -  2 errors generated.

Comment 15

[C.M.Chang[:chunmin]]

I forget to update my patches. It's done now.
This is the try yesterday: https://treeherder.mozilla.org/jobs?repo=try&revision=6cf7267ab7b9ab6e09e8484ba0e660a72357bc0b

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3cf12a0ade20
Remove mandatory low-latency mode for WebCodecs decoders r=media-playback-reviewers,padenot
https://hg.mozilla.org/integration/autoland/rev/83793727335b
Return non-EOS errors in FFmpegDataDecoder::Drain r=media-playback-reviewers,padenot
https://hg.mozilla.org/integration/autoland/rev/40b409bd3bda
Align EOS error handling in FFmpegVideoDecoder with FFmpegAudioDecoder r=media-playback-reviewers,alwu
https://hg.mozilla.org/integration/autoland/rev/2ef82b761efe
Alphabetize headers in DecoderTemplate.h r=media-playback-reviewers,padenot
https://hg.mozilla.org/integration/autoland/rev/452fb3b459b0
Refine variable names and their comments r=media-playback-reviewers,padenot
https://hg.mozilla.org/integration/autoland/rev/eb629fc48216
Add a ForEach function in SimpleMap r=media-playback-reviewers,padenot
https://hg.mozilla.org/integration/autoland/rev/32b98903fcfb
Allow Reset() to reject promises during Flush()'s callback r=media-playback-reviewers,padenot

Comment 17

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/3cf12a0ade20
https://hg.mozilla.org/mozilla-central/rev/83793727335b
https://hg.mozilla.org/mozilla-central/rev/40b409bd3bda
https://hg.mozilla.org/mozilla-central/rev/2ef82b761efe
https://hg.mozilla.org/mozilla-central/rev/452fb3b459b0
https://hg.mozilla.org/mozilla-central/rev/eb629fc48216
https://hg.mozilla.org/mozilla-central/rev/32b98903fcfb