Improve codec unsupported detection #1711

hicom150 · 2019-09-25T20:29:16Z

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[x] Bug fix
[ ] New feature
[ ] Other, please explain:

What changes did you make? (Give an overview)
It seems that from time to time we get some "codec unsupported" false positive. This PR changes unsupported codec detection to use HTMLMediaElement.play() Promise based on what it is explained in this article.

Is there anything you'd like reviewers to focus on?
Check that we don't get false positives caused by timing issues.

feross · 2019-10-01T04:15:14Z

Wow, can it really be this simple? If this actually works better, let's merge it. Haven't had time to test it.

Borewit

Playing back a movie with just an AC3-audio (AC3 is not supported by the Chromium engine) track:

master branch:

PR #1711 :

Do we still get any false positives as indicated in issue #1668?:

master branch:

PR #1711:

Could no longer reproduce the false positive. 👍

src/renderer/pages/player-page.js

Borewit · 2019-10-06T19:23:46Z

Great work @hicom150.

src/renderer/pages/player-page.js

hicom150

All the changes done! 👍

Borewit · 2019-10-07T17:30:35Z

src/renderer/pages/player-page.js

- })
+ .then(
+ () => dispatch('mediaSuccess'),
+ () => dispatch('mediaError', 'Codec unsupported')


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.

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 😅

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

why not?

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.

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?

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 ?

…/webtorrent-desktop into fix_codec_unsupported_detection

RecoX · 2019-10-08T01:40:05Z

I believe the current implementation using the audio/video api for manage this flow is ok, I tried in the past use this approach but I ended up leaving as is because it seems wiser anyways this pr looks simple.

I would like to add some details I found.

I put a console.log, to see that element as a lot of times is throwing me a false positive

  function onCanPlay (e) {
    const elem = e.target
    console.log(666, elem.webkitAudioDecodedByteCount)
    
    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()
    }
  }

Both are mp3's so im not sure if this webkitAudioDecodedByteCount is really trustable

Borewit · 2019-10-08T05:38:56Z

@RecoX

I believe the current implementation using the audio/video api for manage this flow is ok,

With the current implementation we get false positives as described in #1668

I tried in the past use this approach but I ended up leaving

Why? If you had an issue with PR approach, is that still valid? Can you test the PR @RecoX ? Or do you advice to merge, it is not clear to me.

anyways this pr looks simple

Looks very clean and simple indeed.

Borewit · 2019-10-08T05:46:45Z

As there is some discussion, I suggest to keep the PR open for 2 day. Please use the review process fellow contributors.

Sorry for the delay @hicom150.

hicom150 · 2019-10-08T07:02:35Z

Sorry for the delay @hicom150.

Don't worry @Borewit 😉

I find it super helpful to get feedback from other developers, I think that the code gets better when we discus and combine different point of views.

BTW, thank you for your review and great feedback @Borewit 💪

RecoX · 2019-10-08T20:35:57Z

@RecoX

I believe the current implementation using the audio/video api for manage this flow is ok,

With the current implementation we get false positives as described in #1668

I tried in the past use this approach but I ended up leaving

Why? If you had an issue with PR approach, is that still valid? Can you test the PR @RecoX ? Or do you advice to merge, it is not clear to me.

anyways this pr looks simple

Looks very clean and simple indeed.

I tested it and it works ok, but I found a problem.

When I tried to reproduce a wma it throws the modal with the unsupported codec and that is ok but as you see in the console is doing that in an infinite loop so the program freezes.

https://github.com/refreex/refreex-webtorrent-desktop/blob/master/src/renderer/pages/audio-player.js

here the link of the file where i implement it just in case you want to take a look.

btw im using this patch in the meantime.

  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) {

      // Nasty patch to avoid entering here for valid files, we need to address it better.
      const audioExtensions = ['.aac', '.amr', '.alac', '.flac', '.mp3', '.oga', '.ogg', '.opus', '.wav', '.webm'];
      if (audioExtensions.includes(elem.src)) {
        dispatch('mediaError', 'Audio codec unsupported')
      }
      
    } else {
      dispatch('mediaSuccess')
      elem.play()
    }
  }

hicom150 · 2019-10-08T21:28:26Z

@RecoX I can't reproduce your error case 😓 but I noticed that you didn't delete onCanPlay and onError as it is in this PR code 😅

RecoX · 2019-10-09T00:06:59Z

@RecoX I can't reproduce your error case 😓 but I noticed that you didn't delete onCanPlay and onError as it is in this PR code 😅

You don't see them because i didn't push the changes, probably is because the player in my fork is in the main window instead of the player window. Anyways is strange that occurs to me with the new code and with the older no.

mathiasvr · 2019-10-09T17:45:12Z

Video with AC3 audio plays silently (like Chrome), and does not give the user the option to open in an external player.
I also had a false positive (saw the Codec unsupported error on a H264 file), which worked fine all other times I tried.

hicom150 · 2019-10-09T20:38:40Z

Video with AC3 audio plays silently (like Chrome), and does not give the user the option to open in an external player.
I also had a false positive (saw the Codec unsupported error on a H264 file), which worked fine all other times I tried.

Ups! I didn't expect that result 😓

In order to help the debugging process can we share the test files where we get the unexpected behaviors? @mathiasvr @RecoX

Here I share the some files a what should be the expected result:

Video: mpeg2video. Audio: ac3
expected result: Codec Unsupported dialog.
Video: h264 Audio: none
expected result: Plays video normally.

mathiasvr · 2019-10-10T16:06:52Z

@hicom150 I just went ahead and generated a bunch of different samples with various video/audio codecs, because why not? 😃

https://drive.google.com/open?id=1ACw6hOooQNfc-oniyL0Gp-z4OSq-5Mc7

Before this PR, a supported video file (such as h264) with AC3 audio codec displays 'Audio codec not supported', and after this PR it plays the video with audio muted. It would be nice if we could still detect unsupported audio.

hicom150 · 2019-10-10T20:31:09Z

After some further testing with the awesome @mathiasvr video samples 😉 it seems that HTMLMediaElement.play() Promise is not as fine grained as the previous implementation 😭

So unless someone comes with a better solution, I think that the only proper option is to try to optimize the previous onCanPlay function 😅

RecoX · 2019-10-10T20:35:12Z

After some further testing with the awesome @mathiasvr video samples 😉 it seems that HTMLMediaElement.play() Promise is not as fine grained as the previous implementation 😭

So unless someone comes with a better solution, I think that the only proper option is to try to optimize the previous onCanPlay function 😅

Agreed :) +1

mathiasvr · 2019-10-10T20:52:30Z

@hicom150 Did you see other problems than muted audio? Otherwise I think the play() Promise could still be a good solution, if we can detect the audio problem (maybe using onCanPlay) as well.

hicom150 · 2019-10-12T16:00:50Z

I changed the approach and instead of using HTMLMediaElement.play() Promise I try with experimental videoTracks and audioTracks which describe the number of available (correctly decoded) tracks.

Having this, I see two options:

If we use the same approach we use before, we can say that for a video element we expect at least one video track and one audio track and for an audio element we expect at least one audio track. (this has the caveat that videos with no audio track are treated like error WebTorrent Desktop can't play media files that don't contain audio #759)
The other option could be to allow to play videos without any audio track either because it has not any audio stream or because the audio stream codec is not supported. (this has the caveat that we directly play videos without supported audio without giving the option to open in VLC)

To sum up, the advantage of this method is that we can check available tracks early in onLoadedMetadata event and that we don't rely on webkitVideoDecodedByteCount and webkitAudioDecodedByteCount that sometimes have timing issues.

mathiasvr · 2019-10-12T20:07:07Z

Nice work @hicom150!
I haven’t tested it yet, but definitely sounds more stable than the current approach.
I think not supporting video with no audio is fine for now, we could consider adding a “play without audio” button on the “Audio codec not supported” error dialog? But maybe it’s a little too hacky 😉

RecoX · 2019-10-13T06:27:54Z

Nice work @hicom150!
I haven’t tested it yet, but definitely sounds more stable than the current approach.
I think not supporting video with no audio is fine for now, we could consider adding a “play without audio” button on the “Audio codec not supported” error dialog? But maybe it’s a little too hacky 😉

I feel its too hacky too. If we can't play the video as we should we simply can offer to open it with a an external player and that's it. Keep it simple.

hicom150 · 2019-10-13T20:12:36Z

After some back and forth I think that finally I'm going to merge this PR a go forward 😉
Thank you for your awesome help in this issue guys! 💪

Improve codec unsupported detection

5023651

This was referenced Sep 25, 2019

Got an "Audio codec unsupported" from time to time #1668

Closed

WebTorrent Desktop can't play media files that don't contain audio #759

Open

feross added the enhancement label Oct 1, 2019

hicom150 requested a review from Borewit October 6, 2019 17:51

Borewit requested changes Oct 6, 2019

View reviewed changes

Borewit reviewed Oct 6, 2019

View reviewed changes

src/renderer/pages/player-page.js Outdated Show resolved Hide resolved

Borewit reviewed Oct 6, 2019

View reviewed changes

src/renderer/pages/player-page.js Outdated Show resolved Hide resolved

Borewit mentioned this pull request Oct 6, 2019

Add support for multiple audio tracks #1712

Merged

hicom150 added 2 commits October 7, 2019 17:54

Apply review changes

d4a8d9a

Merge branch 'master' into fix_codec_unsupported_detection

5280fc7

hicom150 commented Oct 7, 2019

View reviewed changes

Borewit requested changes Oct 7, 2019

View reviewed changes

hicom150 added 3 commits October 7, 2019 22:06

Improve codec unsupported detection

3d85803

Apply review changes

f5e186d

Merge branch 'fix_codec_unsupported_detection' of github.com:hicom150…

b47d335

…/webtorrent-desktop into fix_codec_unsupported_detection

Borewit approved these changes Oct 8, 2019

View reviewed changes

hicom150 added 2 commits October 11, 2019 16:56

Merge branch 'master' into fix_codec_unsupported_detection

287bcfd

use videoTracks and audioTracks to check unsupported codecs

f2c531b

hicom150 merged commit c33acd0 into webtorrent:master Oct 13, 2019

hicom150 deleted the fix_codec_unsupported_detection branch October 13, 2019 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve codec unsupported detection #1711

Improve codec unsupported detection #1711

hicom150 commented Sep 25, 2019

feross commented Oct 1, 2019

Borewit left a comment •

edited

Loading

Borewit commented Oct 6, 2019

hicom150 left a comment

Borewit Oct 7, 2019

hicom150 Oct 7, 2019

Borewit Oct 7, 2019

Borewit Oct 7, 2019 •

edited

Loading

Borewit Oct 7, 2019

hicom150 Oct 7, 2019

RecoX commented Oct 8, 2019 •

edited

Loading

Borewit commented Oct 8, 2019

Borewit commented Oct 8, 2019

hicom150 commented Oct 8, 2019

RecoX commented Oct 8, 2019 •

edited

Loading

hicom150 commented Oct 8, 2019 •

edited

Loading

RecoX commented Oct 9, 2019 •

edited

Loading

mathiasvr commented Oct 9, 2019

hicom150 commented Oct 9, 2019

mathiasvr commented Oct 10, 2019 •

edited

Loading

hicom150 commented Oct 10, 2019 •

edited

Loading

RecoX commented Oct 10, 2019

mathiasvr commented Oct 10, 2019 •

edited

Loading

hicom150 commented Oct 12, 2019

mathiasvr commented Oct 12, 2019

RecoX commented Oct 13, 2019

hicom150 commented Oct 13, 2019

Improve codec unsupported detection #1711

Improve codec unsupported detection #1711

Conversation

hicom150 commented Sep 25, 2019

feross commented Oct 1, 2019

Borewit left a comment • edited Loading

Choose a reason for hiding this comment

Borewit commented Oct 6, 2019

hicom150 left a comment

Choose a reason for hiding this comment

Borewit Oct 7, 2019

Choose a reason for hiding this comment

hicom150 Oct 7, 2019

Choose a reason for hiding this comment

Borewit Oct 7, 2019

Choose a reason for hiding this comment

Borewit Oct 7, 2019 • edited Loading

Choose a reason for hiding this comment

Borewit Oct 7, 2019

Choose a reason for hiding this comment

hicom150 Oct 7, 2019

Choose a reason for hiding this comment

RecoX commented Oct 8, 2019 • edited Loading

Borewit commented Oct 8, 2019

Borewit commented Oct 8, 2019

hicom150 commented Oct 8, 2019

RecoX commented Oct 8, 2019 • edited Loading

hicom150 commented Oct 8, 2019 • edited Loading

RecoX commented Oct 9, 2019 • edited Loading

mathiasvr commented Oct 9, 2019

hicom150 commented Oct 9, 2019

mathiasvr commented Oct 10, 2019 • edited Loading

hicom150 commented Oct 10, 2019 • edited Loading

RecoX commented Oct 10, 2019

mathiasvr commented Oct 10, 2019 • edited Loading

hicom150 commented Oct 12, 2019

mathiasvr commented Oct 12, 2019

RecoX commented Oct 13, 2019

hicom150 commented Oct 13, 2019

Borewit left a comment •

edited

Loading

Borewit Oct 7, 2019 •

edited

Loading

RecoX commented Oct 8, 2019 •

edited

Loading

RecoX commented Oct 8, 2019 •

edited

Loading

hicom150 commented Oct 8, 2019 •

edited

Loading

RecoX commented Oct 9, 2019 •

edited

Loading

mathiasvr commented Oct 10, 2019 •

edited

Loading

hicom150 commented Oct 10, 2019 •

edited

Loading

mathiasvr commented Oct 10, 2019 •

edited

Loading