Closed Bug 1400758 Opened 7 years ago Closed 7 years ago

Report detailed error description when initializing decoder fail

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Version:
Other Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: alwu, Assigned: alwu)

Details

Attachments

(3 files)

Bug 1400758 - part1 : report the error description with InitPromise for Apple's decoders.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1400758 - part2 : report the error description with InitPromise for agnostic decoders.
59 bytes, text/x-review-board-request
jya
: review+
Details
Bug 1400758 - part3 : report the error description with InitPromise for ffmpeg decoders.
59 bytes, text/x-review-board-request
jya
: review+
Details 
We could provide more information when initializing decoder fail.

eg. [1], we could return more information via InitializeSession() to know why initialization failed.

[1] http://searchfox.org/mozilla-central/source/dom/media/platforms/apple/AppleVTDecoder.cpp#72
Attachment #8909175 - Flags: review?(jyavenard)
Attachment #8909176 - Flags: review?(jyavenard)
Attachment #8909177 - Flags: review?(jyavenard)
Comment on attachment 8909175 [details]
Bug 1400758 - part1 : report the error description with InitPromise for Apple's decoders.

https://reviewboard.mozilla.org/r/180748/#review185870

::: dom/media/platforms/apple/AppleATDecoder.cpp:12
(Diff revision 1)
>  #include "AppleUtils.h"
>  #include "MP4Decoder.h"
>  #include "mp4_demuxer/Adts.h"
>  #include "MediaInfo.h"
> +#include "MediaResult.h"
>  #include "AppleATDecoder.h"

AppleATDecoder.h should be at the top while at it.

Having said that, all decoders include PlatformDecoderModule.h which includes MediaResult.h

The definition of a MediaDataDecoder being that it returns a MediaResult means it's always included.

::: dom/media/platforms/apple/AppleVTDecoder.h:18
(Diff revision 1)
>  #include "ReorderQueue.h"
>  #include "TimeUnits.h"
>  
>  #include "VideoToolbox/VideoToolbox.h"
>  
> +class MediaResult;

unnecessary

::: dom/media/platforms/apple/AppleVTDecoder.cpp:15
(Diff revision 1)
>  #include "AppleDecoderModule.h"
>  #include "AppleUtils.h"
>  #include "AppleVTDecoder.h"
>  #include "AppleVTLinker.h"
>  #include "MediaData.h"
> +#include "MediaResult.h"

unnecessary
Attachment #8909175 - Flags: review?(jyavenard) → review+
Comment on attachment 8909176 [details]
Bug 1400758 - part2 : report the error description with InitPromise for agnostic decoders.

https://reviewboard.mozilla.org/r/180750/#review185872

::: dom/media/platforms/agnostic/OpusDecoder.cpp:7
(Diff revision 1)
>  /* vim:set ts=2 sw=2 sts=2 et cindent: */
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include "MediaResult.h"

unnecessary.

And even if so, #include "OpusDecoder.h" should always be the first include as per coding style.

::: dom/media/platforms/agnostic/TheoraDecoder.cpp:8
(Diff revision 1)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +
> +#include "MediaResult.h"

unnecessary

and TheoraDecoder.h should always be first

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices

"Includes are split into three blocks and are sorted alphabetically in each block:

    The main header: #include "Foo.h" in Foo.cpp
    Standard library includes: #include <map>
    Mozilla includes: #include "mozilla/dom/Element.h"
"

::: dom/media/platforms/agnostic/TheoraDecoder.cpp:112
(Diff revision 1)
>    mTheoraDecoderContext = th_decode_alloc(&mTheoraInfo, mTheoraSetupInfo);
>    if (mTheoraDecoderContext) {
>      return InitPromise::CreateAndResolve(TrackInfo::kVideoTrack, __func__);
>    } else {
> -    return InitPromise::CreateAndReject(NS_ERROR_DOM_MEDIA_FATAL_ERR, __func__);
> +    return InitPromise::CreateAndReject(
> +      MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR,

NS_ERROR_OUT_OF_MEMORY

::: dom/media/platforms/agnostic/VorbisDecoder.cpp:80
(Diff revision 1)
>    if (!XiphExtradataToHeaders(headers, headerLens,
>                                mInfo.mCodecSpecificConfig->Elements(),
>                                mInfo.mCodecSpecificConfig->Length())) {
> -    return InitPromise::CreateAndReject(NS_ERROR_DOM_MEDIA_FATAL_ERR, __func__);
> +    return InitPromise::CreateAndReject(
> +      MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR,
> +                  RESULT_DETAIL("Could not get vorbis header!")),

remove the ending ! of all results thanks.
Attachment #8909176 - Flags: review?(jyavenard) → review+
Comment on attachment 8909177 [details]
Bug 1400758 - part3 : report the error description with InitPromise for ffmpeg decoders.

https://reviewboard.mozilla.org/r/180752/#review185882

You're also changing the error code, but that's okay.

::: dom/media/platforms/ffmpeg/FFmpegAudioDecoder.cpp:10
(Diff revision 1)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "mozilla/TaskQueue.h"
>  
>  #include "FFmpegAudioDecoder.h"
> +#include "MediaResult.h"

unnecessary

::: dom/media/platforms/ffmpeg/FFmpegDataDecoder.h:15
(Diff revision 1)
>  #include "PlatformDecoderModule.h"
>  #include "FFmpegLibWrapper.h"
>  #include "mozilla/StaticMutex.h"
>  #include "FFmpegLibs.h"
>  
> +class MediaResult;

unnecessary

::: dom/media/platforms/ffmpeg/FFmpegDataDecoder.cpp:15
(Diff revision 1)
>  #include <unistd.h>
>  #endif
>  
>  #include "FFmpegLog.h"
>  #include "FFmpegDataDecoder.h"
> +#include "MediaResult.h"

Unnecessary

::: dom/media/platforms/ffmpeg/FFmpegDataDecoder.cpp:56
(Diff revision 1)
>    }
>  
>    StaticMutexAutoLock mon(sMonitor);
>  
>    if (!(mCodecContext = mLib->avcodec_alloc_context3(codec))) {
> -    NS_WARNING("Couldn't init ffmpeg context");
> +    return MediaResult(NS_ERROR_DOM_MEDIA_FATAL_ERR,

NS_ERROR_OUT_OF_MEMORY

::: dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp:12
(Diff revision 1)
>  #include "FFmpegVideoDecoder.h"
>  #include "FFmpegLog.h"
>  #include "ImageContainer.h"
>  #include "MediaInfo.h"
>  #include "MP4Decoder.h"
> +#include "MediaResult.h"

unnecessary
Attachment #8909177 - Flags: review?(jyavenard) → review+
Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/67ce16d81d3c
part1 : report the error description with InitPromise for Apple's decoders. r=jya
https://hg.mozilla.org/integration/autoland/rev/1d0e5cf9e932
part2 : report the error description with InitPromise for agnostic decoders. r=jya
https://hg.mozilla.org/integration/autoland/rev/074e1de206f9
part3 : report the error description with InitPromise for ffmpeg decoders. r=jya
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: