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)
Tracking
()
VERIFIED
FIXED
mozilla58
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.
Updated•7 years ago
|
Comment 1•7 years ago
|
||
Regression window: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=fd57bcd5daf2e1dd79021f4fa7ec659ad4b40d21&tochange=09fb9831233593b5c88028ff9400993acd680559 Regressed by: Bug 1331329
Blocks: 1331329
Status: UNCONFIRMED → NEW
status-firefox55:
--- → wontfix
status-firefox56:
--- → affected
status-firefox57:
--- → affected
status-firefox-esr52:
--- → unaffected
Ever confirmed: true
Version: 55 Branch → 53 Branch
Updated•7 years ago
|
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.
tracking-firefox57:
--- → ?
Comment 3•7 years ago
|
||
(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.
Assignee | ||
Comment 5•7 years ago
|
||
interestingly, it works fine when the VP9 hardware decoder is on.
Flags: needinfo?(jyavenard)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jyavenard
Assignee | ||
Comment 6•7 years ago
|
||
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)
Comment 7•7 years ago
|
||
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)
Comment 8•7 years ago
|
||
Mass change P1->P2 to align with new Mozilla triage process
Priority: P1 → P2
Assignee | ||
Comment 10•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(jyavenard)
Comment 12•7 years ago
|
||
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.
Assignee | ||
Comment 14•7 years ago
|
||
(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)
Assignee | ||
Comment 15•7 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 20•7 years ago
|
||
mozreview-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/#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 21•7 years ago
|
||
mozreview-review |
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 22•7 years ago
|
||
mozreview-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 23•7 years ago
|
||
mozreview-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 24•7 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 33•7 years ago
|
||
mozreview-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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 37•7 years ago
|
||
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
Comment 38•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a22a2fcf6d94 https://hg.mozilla.org/mozilla-central/rev/d78ce15e4d6f https://hg.mozilla.org/mozilla-central/rev/71c088664c27 https://hg.mozilla.org/mozilla-central/rev/cc9d239ebed1
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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.
Updated•6 years ago
|
Flags: qe-verify+
Comment 40•6 years ago
|
||
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?
Comment 41•6 years ago
|
||
(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.
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•