Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve codec unsupported detection #1711

Merged
merged 8 commits into from
Oct 13, 2019
Merged

Improve codec unsupported detection #1711

Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5023651
Improve codec unsupported detection
hicom150 Sep 25, 2019
d4a8d9a
Apply review changes
hicom150 Oct 7, 2019
5280fc7
Merge branch 'master' into fix_codec_unsupported_detection
hicom150 Oct 7, 2019
3d85803
Improve codec unsupported detection
hicom150 Sep 25, 2019
f5e186d
Apply review changes
hicom150 Oct 7, 2019
b47d335
Merge branch 'fix_codec_unsupported_detection' of github.com:hicom150…
hicom150 Oct 7, 2019
287bcfd
Merge branch 'master' into fix_codec_unsupported_detection
hicom150 Oct 11, 2019
f2c531b
use videoTracks and audioTracks to check unsupported codecs
hicom150 Oct 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 4 additions & 18 deletions src/renderer/pages/player-page.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
/* global HTMLMediaElement */

const React = require('react')
const Bitfield = require('bitfield')
const prettyBytes = require('prettier-bytes')
Expand Down Expand Up @@ -56,6 +54,10 @@ function renderMedia (state) {
mediaElement.pause()
} else if (!state.playing.isPaused && mediaElement.paused) {
mediaElement.play()
.then(
() => dispatch('mediaSuccess'),
() => dispatch('mediaError', 'Codec unsupported')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe?

        .then(
          () => dispatch('mediaSuccess'),
          err => dispatch('mediaError', err.name === 'NotSupportedError' ? 'Codec unsupported' : `${err.name}: ${err.message}`)
        )

I put it on a branch: https://github.com/webtorrent/webtorrent-desktop/tree/issue-1711-improve-error-handling

For some reason I would send PR to your clone, I think select your fork as the base. Maybe a bug in GitHub, to many forks of this repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we should dispatch mediaError when error type is NotSupportedError (original code) or when any error case (like the latest code change).

Given that the possible exceptions are NotSupportedError or NotAllowedError I wouldn't show this last one to the final user 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't show this last one to the final user

why not?

Copy link
Member

@Borewit Borewit Oct 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also says other exceptions may be reported,. I suppose these are not very likely to be thrown, but it may be good user gets an error if something odd happens.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the current code, in the unlikely event of an error named NotAllowedError, or any other exception, it will prompt the user with 'Codec unsupported' right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the current code, in the unlikely event of an error named NotAllowedError, or any other exception, it will prompt the user with 'Codec unsupported' right?

Yes that is right 😅 Although it is not 100% correct for the final user, the action should be the same: open an external player to be able to play the file, otherwise it would see the message The request is not allowed by the user agent or the platform in the current context, possibly because the user denied permission. which I found a bit confusing for the user as it doesn't give any "call to action" to solve the problem.

On the other hand, it is true that we should log the proper error code, so we can debug future problems easily 😉

What do you think @Borewit ?

)
}
// When the user clicks or drags on the progress bar, jump to that position
if (state.playing.jumpToTime != null) {
Expand Down Expand Up @@ -127,10 +129,8 @@ function renderMedia (state) {
onLoadedMetadata={onLoadedMetadata}
onEnded={onEnded}
onStalled={dispatcher('mediaStalled')}
onError={dispatcher('mediaError')}
onTimeUpdate={dispatcher('mediaTimeUpdate')}
onEncrypted={dispatcher('mediaEncrypted')}
onCanPlay={onCanPlay}
>
{trackTags}
</MediaTagName>
Expand Down Expand Up @@ -168,20 +168,6 @@ function renderMedia (state) {
if (state.window.isFullScreen) dispatch('toggleFullScreen')
}
}

function onCanPlay (e) {
const elem = e.target
if (elem.readyState < HTMLMediaElement.HAVE_FUTURE_DATA) return
if (state.playing.type === 'video' &&
elem.webkitVideoDecodedByteCount === 0) {
dispatch('mediaError', 'Video codec unsupported')
} else if (elem.webkitAudioDecodedByteCount === 0) {
dispatch('mediaError', 'Audio codec unsupported')
} else {
dispatch('mediaSuccess')
elem.play()
}
}
}

function renderOverlay (state) {
Expand Down