Closed Bug 1304252 Opened 3 years ago Closed 3 years ago

Provide more detailed error for decoders part 2.

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(3 files)

Missed some decoders with bug 1303673..

complete the missing ones.
Comment on attachment 8793152 [details]
Bug 1304252: P1. Provide error details for ffmpeg decoder.

https://reviewboard.mozilla.org/r/79946/#review78678
Attachment #8793152 - Flags: review?(gsquelart) → review+
Comment on attachment 8793153 [details]
Bug 1304252: P2. Provide further error details for android decoder.

https://reviewboard.mozilla.org/r/79948/#review78682
Attachment #8793153 - Flags: review?(gsquelart) → review+
Comment on attachment 8793154 [details]
Bug 1304252: P3. Further error details for agnostic decoders.

https://reviewboard.mozilla.org/r/79950/#review78684

r+ with nit:

::: dom/media/platforms/agnostic/VorbisDecoder.cpp:180
(Diff revision 1)
>    }
>  
> -  if (vorbis_synthesis_blockin(&mVorbisDsp,
> -                               &mVorbisBlock) != 0) {
> -    return NS_ERROR_DOM_MEDIA_DECODE_ERR;
> +  err = vorbis_synthesis_blockin(&mVorbisDsp, &mVorbisBlock);
> +  if (err) {
> +    return MediaResult(NS_ERROR_DOM_MEDIA_DECODE_ERR,
> +                       RESULT_DETAIL("Vorbis synthesis blockin:%d", err));

"blockin" is not a real word.
How about "Vorbis synthesis block assembly error:%d"?
Attachment #8793154 - Flags: review?(gsquelart) → review+
Comment on attachment 8793154 [details]
Bug 1304252: P3. Further error details for agnostic decoders.

https://reviewboard.mozilla.org/r/79950/#review78684

> "blockin" is not a real word.
> How about "Vorbis synthesis block assembly error:%d"?

it's the name of the function causing the error. wouldn't this be preferred?
Comment on attachment 8793154 [details]
Bug 1304252: P3. Further error details for agnostic decoders.

https://reviewboard.mozilla.org/r/79950/#review78684

> it's the name of the function causing the error. wouldn't this be preferred?

I was trying to be consistent with the previous error at line 174, where you seemed to transform the function name into a sentence related to the function meaning, and added "error".
In that case I think you should use the exact function name, and add "error", i.e.: "vorbis_synthesis_blockin error:%d".
Or something else as you see fit; I only object to "blockin" being used as an English noun when it is not. -- Well, apart from what the Urban Dictionary says, but I don't think it fits with the Vorbis function.
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/69738c146f7f
P1. Provide error details for ffmpeg decoder. r=gerald
https://hg.mozilla.org/integration/autoland/rev/4b9917e55cf3
P2. Provide further error details for android decoder. r=gerald
https://hg.mozilla.org/integration/autoland/rev/8a87c36b6596
P3. Further error details for agnostic decoders. r=gerald
Comment on attachment 8793152 [details]
Bug 1304252: P1. Provide error details for ffmpeg decoder.

Approval request is for all changes.

Approval Request Comment
[Feature/regressing bug #]: None
[User impact if declined]: Making harder to troubleshoot current decoding error seen with amazon and netflix
[Describe test coverage new/current, TreeHerder]: In central
[Risks and why]: None. We're just adding details to help identify where failures occur
[String/UUID change made/needed]: None

This is a followup on bug 1303673
Attachment #8793152 - Flags: approval-mozilla-aurora?
Comment on attachment 8793152 [details]
Bug 1304252: P1. Provide error details for ffmpeg decoder.

This patch provides more detailed error information for troubleshooting. Take it in 51 aurora.
Attachment #8793152 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.