Closed Bug 1343437 Opened 7 years ago Closed 7 years ago

Demuxer InitPromise success value can include a warning, to be forwarded to the media element

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Version:
53 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mozbugz, Assigned: mozbugz)

References

Details

Attachments

(6 files, 2 obsolete files)

Bug 1343437 - MediaDataDemuxer::InitPromise resolve-value is now a MediaResult -
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1343437 - MP4Demuxer::Init resolves with a MediaResult -
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1343437 - MFR::OnDemuxerInitDone forwards non-NS_OK MediaResult to HTMLMediaElement::DecodeWarning -
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1343437 - TBM::OnDemuxer{Init,Reset}Done forward non-NS_OK MediaResult to HTMLMediaElement::DecodeWarning -
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1343437 - 'media.playback.warnings-as-errors' pref -
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1343437 - HTMLMediaElement::DecodeWarning() should store a warning instead of an error -
59 bytes, text/x-review-board-request
jya
: review+
Details 
The resolve-value of MediaDataDemuxer::InitPromise is currently an nsresult that is always set to true, and is never actually used.

It could be turned into a more useful MediaResult, which could carry a warning, and this warning could be forwarded to the appropriate HTMLMediaElement::DecodeWarning().
Comment on attachment 8842757 [details]
Bug 1343437 - MediaDataDemuxer::InitPromise resolve-value is now a MediaResult -

https://reviewboard.mozilla.org/r/116524/#review118142

I am dubious to the need to report errors but that aren't errors when an init promise resolved.
Either it works, or it doesn't.

What information could be of any use to the user (or even developer) for a file we accepted to open.

Providing details is good, but it has to be of some use.
Reporting on the why we refused to open a file is one thing. Reporting that we opened a file but really we may not have is useless IMHO.

So r+ if you really want this, but I don't think it should go in: information overload.
Attachment #8842757 - Flags: review?(jyavenard) → review+
Comment on attachment 8842758 [details]
Bug 1343437 - MP4Demuxer::Init resolves with a MediaResult -

https://reviewboard.mozilla.org/r/116526/#review118144

samre reserve as earlier
Attachment #8842758 - Flags: review?(jyavenard) → review+
Comment on attachment 8842759 [details]
Bug 1343437 - MP4Demuxer rejects InitPromise is there is no valid track -

https://reviewboard.mozilla.org/r/116528/#review118146

This serves no purpose, and is redundant. The MediaFormatReader already reject the MetadataPromise if there's no valid track
Attachment #8842759 - Flags: review?(jyavenard)
Comment on attachment 8842760 [details]
Bug 1343437 - MFR::OnDemuxerInitDone forwards non-NS_OK MediaResult to HTMLMediaElement::DecodeWarning -

https://reviewboard.mozilla.org/r/116530/#review118150
Attachment #8842760 - Flags: review?(jyavenard) → review+
Comment on attachment 8842761 [details]
Bug 1343437 - TBM::OnDemuxer{Init,Reset}Done forward non-NS_OK MediaResult to HTMLMediaElement::DecodeWarning -

https://reviewboard.mozilla.org/r/116532/#review118152
Attachment #8842761 - Flags: review?(jyavenard) → review+
Comment on attachment 8842759 [details]
Bug 1343437 - MP4Demuxer rejects InitPromise is there is no valid track -

https://reviewboard.mozilla.org/r/116528/#review118146

As I explained in the commit description, the MFR rejects if there's no valid *first* tracks!
The new test here reports an error if there's not valid tracks *at all*, which I think would be a useful addition, it's not totally redundant.

But it's not that critical I suppose, so I'm fine if you really don't want this extra bit of information.
Attachment #8842759 - Attachment is obsolete: true
Comment on attachment 8843185 [details]
Bug 1343437 - 'media.playback.warnings-as-errors' pref -

https://reviewboard.mozilla.org/r/116952/#review118964

::: dom/media/MediaFormatReader.cpp:1267
(Diff revision 1)
>  MediaFormatReader::OnDemuxerInitDone(const MediaResult& aResult)
>  {
>    MOZ_ASSERT(OnTaskQueue());
>    mDemuxerInitRequest.Complete();
>  
> +  if (aResult != NS_OK && MediaPrefs::MediaWarningsAsErrors()) {

NS_SUCCEEDED

as thats how all MediaResult are always used to check for success.

::: dom/media/mediasource/TrackBuffersManager.cpp:863
(Diff revision 1)
>  TrackBuffersManager::OnDemuxerResetDone(const MediaResult& aResult)
>  {
>    MOZ_ASSERT(OnTaskQueue());
>    mDemuxerInitRequest.Complete();
> +
> +  if (aResult != NS_OK && MediaPrefs::MediaWarningsAsErrors()) {

2 spaces indent

::: dom/media/mediasource/TrackBuffersManager.cpp:951
(Diff revision 1)
>    MOZ_DIAGNOSTIC_ASSERT(mInputDemuxer, "mInputDemuxer has been destroyed");
>  
>    mDemuxerInitRequest.Complete();
>  
> +  if (aResult != NS_OK && MediaPrefs::MediaWarningsAsErrors()) {
> +      RejectAppend(aResult, __func__);

2 spaces indent

::: modules/libpref/init/all.js:611
(Diff revision 1)
>  
>  // Log level for cubeb, the audio input/output system. Valid values are
>  // "verbose", "normal" and "" (log disabled).
>  pref("media.cubeb.log_level", "");
>  
> +// Set to true to force demux/decode warnings to be treated as errors.

The comment about decode errors doesnt match the use. 

its all about demuxing warnings right now.

or you can also make it for non fatal decode errors as well, and quite easily so. Modify the MFR so that the test checking if an error is fatal to always return true if the pref is set.
Attachment #8843185 - Flags: review?(jyavenard) → review+
Comment on attachment 8843185 [details]
Bug 1343437 - 'media.playback.warnings-as-errors' pref -

https://reviewboard.mozilla.org/r/116952/#review118964

> The comment about decode errors doesnt match the use. 
> 
> its all about demuxing warnings right now.
> 
> or you can also make it for non fatal decode errors as well, and quite easily so. Modify the MFR so that the test checking if an error is fatal to always return true if the pref is set.

I'm not sure I understand your comment.

To make sure we're talking about the same things:
What I call "warning" is an error code that comes through a promise *resolution* (as opposed to a real error that comes through a promise *rejection*).

If this pref "media.playback.warnings-as-errors" is set to true, warnings that come into promise resolution callbacks will be handled the same was as errors in rejection callbacks.
For example, in `MFR::OnDemuxerInitDone(MediaResult aResult)`, if NS_FAILED(aResult) and MediaWarningsAsErrors, we just reject the mMetadataPromise and return, which is what happens in MFR::OnDemuxerInitFailed.

So I think the comment matches the code. Don't you think? Or should I rephrase the comment to be more understandable?


About fatal errors in the MFR, are you talking about HasFatalError() allowing a certain number of NS_ERROR_DOM_MEDIA_DECODE_ERR's?
If yes, I could indeed use that pref to make them immediately fatal. -- Is that what you meant when saying the comment didn't match the use?
And looking at it, maybe I should treat these non-fatal NS_ERROR_DOM_MEDIA_DECODE_ERR errors as warnings otherwise. I.e., I could show a user notification if they happen.
Comment on attachment 8843185 [details]
Bug 1343437 - 'media.playback.warnings-as-errors' pref -

https://reviewboard.mozilla.org/r/116952/#review118964

> I'm not sure I understand your comment.
> 
> To make sure we're talking about the same things:
> What I call "warning" is an error code that comes through a promise *resolution* (as opposed to a real error that comes through a promise *rejection*).
> 
> If this pref "media.playback.warnings-as-errors" is set to true, warnings that come into promise resolution callbacks will be handled the same was as errors in rejection callbacks.
> For example, in `MFR::OnDemuxerInitDone(MediaResult aResult)`, if NS_FAILED(aResult) and MediaWarningsAsErrors, we just reject the mMetadataPromise and return, which is what happens in MFR::OnDemuxerInitFailed.
> 
> So I think the comment matches the code. Don't you think? Or should I rephrase the comment to be more understandable?
> 
> 
> About fatal errors in the MFR, are you talking about HasFatalError() allowing a certain number of NS_ERROR_DOM_MEDIA_DECODE_ERR's?
> If yes, I could indeed use that pref to make them immediately fatal. -- Is that what you meant when saying the comment didn't match the use?
> And looking at it, maybe I should treat these non-fatal NS_ERROR_DOM_MEDIA_DECODE_ERR errors as warnings otherwise. I.e., I could show a user notification if they happen.

Hopefully I've interpreted your review correctly:
- A new patch stores non-fatal errors as warnings.
- 'media.playback.warnings-as-errors' doesn't allow multiple decode errors before failing.
Please let me know if you were thinking of other things to do.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=024c40d932d106e6329a36302554bfb87741f44f

It contains printf's to see what kind of errors/warnings would be reported; search for "::Decode" in non-e10s logs.
Comment on attachment 8843842 [details]
Bug 1343437 - MFR::NotifyError reports non-fatal MediaResult as warning -

https://reviewboard.mozilla.org/r/117444/#review119074

you now have duplicated code in the MFR, this code should be placed where the handling of error is done in MFR::Update

::: dom/media/MediaFormatReader.cpp:1742
(Diff revision 1)
>  MediaFormatReader::NotifyError(TrackType aTrack, const MediaResult& aError)
>  {
>    MOZ_ASSERT(OnTaskQueue());
>    NS_WARNING(aError.Description().get());
>    LOGV("%s Decoding error", TrackTypeToStr(aTrack));
> -  auto& decoder = GetDecoderData(aTrack);
> +  auto& decoderData = GetDecoderData(aTrack);

throughout the code, the temporary is called decoder.
please leave as is, or change them all
Attachment #8843842 - Flags: review?(jyavenard)
Comment on attachment 8843185 [details]
Bug 1343437 - 'media.playback.warnings-as-errors' pref -

https://reviewboard.mozilla.org/r/116952/#review118964

> Hopefully I've interpreted your review correctly:
> - A new patch stores non-fatal errors as warnings.
> - 'media.playback.warnings-as-errors' doesn't allow multiple decode errors before failing.
> Please let me know if you were thinking of other things to do.

the comment is "// Set to true to force demux/decode warnings to be treated as errors."

But only demux warning are treated as errors in the patch queue at the time you asked me for review.
There was nothing related to decoding error being treated as error.

All you had to do was change HasFatalError to test for the pref and return true when needed. A small change.
Attachment #8843842 - Attachment is obsolete: true
Comment on attachment 8846659 [details]
Bug 1343437 - HTMLMediaElement::DecodeWarning() should store a warning instead of an error -

https://reviewboard.mozilla.org/r/119676/#review121952
Attachment #8846659 - Flags: review?(jyavenard) → review+
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/07744b3914c7
HTMLMediaElement::DecodeWarning() should store a warning instead of an error - r=jya
https://hg.mozilla.org/integration/autoland/rev/fe99ecf674b8
MediaDataDemuxer::InitPromise resolve-value is now a MediaResult - r=jya
https://hg.mozilla.org/integration/autoland/rev/f90ddaaea394
MP4Demuxer::Init resolves with a MediaResult - r=jya
https://hg.mozilla.org/integration/autoland/rev/545bdc6c8ca3
MFR::OnDemuxerInitDone forwards non-NS_OK MediaResult to HTMLMediaElement::DecodeWarning - r=jya
https://hg.mozilla.org/integration/autoland/rev/1c76c26c779c
TBM::OnDemuxer{Init,Reset}Done forward non-NS_OK MediaResult to HTMLMediaElement::DecodeWarning - r=jya
https://hg.mozilla.org/integration/autoland/rev/3b5b46e78a82
'media.playback.warnings-as-errors' pref - r=jya
Depends on: 1366208
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: