Closed Bug 1393087 Opened 7 years ago Closed 7 years ago

Error jumping to the end of Webm videos in recent versions of Firefox

Categories

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

53 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 - wontfix
firefox58 --- verified
firefox59 --- verified

People

(Reporter: ekorenblit, Assigned: jya)

References

Details

(Keywords: compat, regression)

Attachments

(4 files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.101 Safari/537.36

Steps to reproduce:

I have a Webm video on my page and upon page load I use Javascript to go to a point towards the end of the video (by changing 'currentTime').

I created a page that lets you see the problem here:
http://rebbephoto.com/test/ffwebm/?time=1000

You can change the time in the URL to select how many seconds into the video to start at.
If you start towards the beginning of the video (use a time less than or equal to 937 seconds,) it plays fine all the way until the end.
But if you start at 938 seconds or later it will throw an error and not play at all.

This plays: http://rebbephoto.com/test/ffwebm/?time=900
This does not play: http://rebbephoto.com/test/ffwebm/?time=1000

I experienced this problem on a Desktop Windows computer running Firefox 55.0.1 as well as 55.0.2.

This bug was introduced in a recent version of Firefox.

I tested it on Firefox 50.1.0 and it works fine.


Actual results:

The video player said: 
"No video with supported format and MIME type found."

The console said: 

Media resource video.webm could not be decoded.

All candidate resources failed to load. Media load paused.

Media resource video.webm could not be decoded, error: Error Code: NS_ERROR_DOM_MEDIA_DECODE_ERR (0x806e0004)

Details: class mozilla::MediaResult __cdecl mozilla::VPXDecoder::DecodeAlpha(struct vpx_image **,const class mozilla::MediaRawData *): VPX decode alpha error: Bitstream not supported by this decoder



Expected results:

It should have played the video.
Component: Untriaged → Audio/Video: Playback
Keywords: regression
Product: Firefox → Core
Flags: needinfo?(jwwang)
Priority: -- → P1
[Tracking Requested - why for this release]: Breaking webm seeking is sufficient to be a problem. I'm also suspicious that it is not container specific.
(In reply to ekorenblit from comment #0)
> Media resource video.webm could not be decoded, error: Error Code:
> NS_ERROR_DOM_MEDIA_DECODE_ERR (0x806e0004)
> 
> Details: class mozilla::MediaResult __cdecl
> mozilla::VPXDecoder::DecodeAlpha(struct vpx_image **,const class
> mozilla::MediaRawData *): VPX decode alpha error: Bitstream not supported by
> this decoder

Hi jya,
Do you have any idea about this decode error? It happens when we seek to a far position (t>=1000s) after calling MediaFormatReader::ReleaseResources().
Flags: needinfo?(jwwang) → needinfo?(jyavenard)
Until we root cause this, I'd like to track it as a 57 blocking issue.
interestingly, it works fine when the VP9 hardware decoder is on.
Flags: needinfo?(jyavenard)
Assignee: nobody → jyavenard
nestegg identifies this part of the video as having an alpha channel.
This alpha channel doesn't decode.

if we can't decode the alpha channel, then we treat it as fatal error.
So I guess we should either ignore the alpha channel info, or make alpha channel decoding error non fatal.

This video is also a VP8, I guess we could also ignore alpha stream if it's for VP8 codec.

:kinetik, what do you think?
Flags: needinfo?(kinetik)
Why does that track fail to decode for us and not Chrome?  We should be able to play it if they can.

If you let it play forward from 900s, it seems to decode fine.  There's only an issue when when seeking near that specific point.
Flags: needinfo?(kinetik)
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
jya: comment 7
Flags: needinfo?(jyavenard)
I'm getting to it... 

it's probably because the first alpha sample decoded isn't a keyframe.

I believe a work around would be to seek to the previous keyframe taking into account the alpha channel.
so we seek to min(last_previous_normal_keyframe, last_previous_normal_alpha_keyframe)

here the code always assumed the keyframes coincide.
Flags: needinfo?(jyavenard)
Flags: needinfo?(jyavenard)
Any progress on this bug?
Flags: needinfo?(bwu)
Per discussing with jya via #media, this file on comment 0 is not a normal file whose the first frame of the alpha plane isn't a keyframe. It would be good to be fixed in 57, but it should not be a blocking issue.
Flags: needinfo?(bwu)
Keywords: compat
Put that 57 tracking back.
(In reply to Matthew Gregan [:kinetik] from comment #7)
> Why does that track fail to decode for us and not Chrome?  We should be able
> to play it if they can.
> 
> If you let it play forward from 900s, it seems to decode fine.  There's only
> an issue when when seeking near that specific point.

Because the webm demuxer uses nestegg_track_seek to seek, which takes no account of the alpha plane. The first sample demuxed at the offset returned by nestegg_track_seek isn't a keyframe and that cause a decoding error.

I could work around nestegg_track_seek and ignore the alpha plane until we see a new keyframe, but then we would no longer see the timed counter that shows up.

The best way to fix it is in nestegg IMHO.

There's also many assumptions made in nestegg that aren't really valid with alpha (at least for this video) that is the cluster starts with a keyframe. that's not the case for the alpha bit.
Flags: needinfo?(jyavenard) → needinfo?(kinetik)
Chrome doesn't seek in this file properly either.

Using Chrome 62, drag the time slider to anywhere between 15:38 and 17:52 or between 14:10 and 14:20 see that it's completely corrupted content until a new keyframe in the alpha channel is found.
Flags: needinfo?(kinetik)
Comment on attachment 8911942 [details]
Bug 1393087 - P1. Only consider a video frame as keyframe if both channels are keyframes.

https://reviewboard.mozilla.org/r/183350/#review188530

Fly-by:

::: commit-message-18b86:3
(Diff revision 1)
> +Bug 1393087 - P1. Only consider a video frame as keyframe if both channels are keyframes. r?kinetik
> +
> +We want both the normal and alpha channels are keyframe.

'are' -> 'to be'
Comment on attachment 8911943 [details]
Bug 1393087 - P2. Add methods for range-based loops.

https://reviewboard.mozilla.org/r/183352/#review188548

::: dom/media/webm/WebMDemuxer.h:99
(Diff revision 1)
>    {
>      return mQueue.back();
>    }
>  
> +    // Methods for range-based for loops.
> +  typename ContainerType::iterator begin()

I don't think you need to specify `typename` here, because ContainerType is fully defined and not dependent on external template arguments, so it should be unambiguous what `iterator` refers to.

(I tested my theory on https://godbolt.org/g/GVgcVF -- but of course if that doesn't work for you, please keep them.)
Attachment #8911943 - Flags: review?(gsquelart) → review+
Comment on attachment 8911942 [details]
Bug 1393087 - P1. Only consider a video frame as keyframe if both channels are keyframes.

https://reviewboard.mozilla.org/r/183350/#review188606
Attachment #8911942 - Flags: review?(kinetik) → review+
Comment on attachment 8911944 [details]
Bug 1393087 - P3: Retry backward to find keyframe.

https://reviewboard.mozilla.org/r/183354/#review188608
Attachment #8911944 - Flags: review?(kinetik) → review+
Comment on attachment 8911945 [details]
Bug 1393087 - P4. Remove soft assertion.

https://reviewboard.mozilla.org/r/183356/#review188610
Attachment #8911945 - Flags: review?(kinetik) → review+
Comment on attachment 8911944 [details]
Bug 1393087 - P3: Retry backward to find keyframe.

https://reviewboard.mozilla.org/r/183354/#review188716

::: dom/media/webm/WebMDemuxer.cpp:1133
(Diff revision 3)
> -  }
> +      }
> +    }
> +    if (mType == TrackInfo::kVideoTrack &&
> +        !mInfo->GetAsVideoInfo()->HasAlpha()) {
> +      // We only perform a search for a keyframe on videos with alpha layer to
> +      // prevent potential regression for normal video (even though invalid)

I got cold feet in thinking this could cause performance regression in files that do not have keyframe at the start of a cluster.

We could in theory go back all the way to the start of the video and start decoding from there...

so just to be safe, I've only enabled the changes on videos with alpha.

Even though on the video that first caused us to check for keyframe, the seek is now more accurate and we seek to the proper frame
(see bug https://bugzilla.mozilla.org/show_bug.cgi?id=1262727)
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a22a2fcf6d94
P1. Only consider a video frame as keyframe if both channels are keyframes. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/d78ce15e4d6f
P2. Add methods for range-based loops. r=gerald
https://hg.mozilla.org/integration/autoland/rev/71c088664c27
P3: Retry backward to find keyframe. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/cc9d239ebed1
P4. Remove soft assertion. r=kinetik
Given 1) the size of the change, 2) the fact that this is not a new regression and 3) the fix has not baked in central enough, I'd like to mark this as a wontfix for 57. I would prefer this fix ride the 58 train.
Flags: qe-verify+
I managed to reproduce the bug using an older version of Firefox (2017-08-23) on Windows 10 x64.
I retested everything on latest Nightly 59 and beta 58.0b4 using Windows 10 x64, MacOS 10.11 and Ubuntu 16.04 x64 and the bug is not reproducing anymore. The video starts to play.

The only problem is that it takes a while for the video to load (10~20 seconds). Is that expected behaviour or is some other issue?
(In reply to Oana Botisan from comment #40)
> I managed to reproduce the bug using an older version of Firefox
> (2017-08-23) on Windows 10 x64.
> I retested everything on latest Nightly 59 and beta 58.0b4 using Windows 10
> x64, MacOS 10.11 and Ubuntu 16.04 x64 and the bug is not reproducing
> anymore. The video starts to play.
> 
> The only problem is that it takes a while for the video to load (10~20
> seconds). Is that expected behaviour or is some other issue?
No. it is not reasonable to take 10-20 seconds to load. Chrome can load it quite fast. 
Can you help file another bug for this problem?
Thanks.
Blocks: 1417911
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.