Closed Bug 1406503 Opened 7 years ago Closed 7 years ago

Intermittent dom/media/test/test_loop.html | Test timed out!

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: jya)

References

(Depends on 1 open bug)

Details

(Keywords: intermittent-failure)

Attachments

(5 files)

Bug 1406503 - P1. Abstract FFmpeg decoding so that an av_parser can also be used for audio.
59 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1406503 - P2. Use common draining mechanism for both audio and video decoder.
59 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1406503 - P3. Also drain av_parser when draining.
59 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1406503 - P4. Re-create av_parser when flushing.
59 bytes, text/x-review-board-request
jwwang
: review+
Details
Bug 1406503 - P5. Do not ignore small flac packets.
59 bytes, text/x-review-board-request
jwwang
: review+
Details
https://treeherder.mozilla.org/logviewer.html#?job_id=135388461&repo=mozilla-inbound&lineNumber=6288 flac-s24.flac-11 timed out! [flac @ 000001D88CF598A0] underread: 14 orig size: 3793 [flac @ 000001D88CF598A0] Multiple frames in a packet. [flac @ 000001D88CF598A0] underread: 14 orig size: 3793 [flac @ 000001D88CF598A0] underread: 14 orig size: 3793 It looks like the timeout is caused by flac decode error and recently we have several flac timeouts. Hi Jya, Do you have any idea about these errors?
Flags: needinfo?(jyavenard)
No. There were recent changed to the flac demuxer. I dont know if they are related. The message above are warnings however and wouldn't have caused a fatal decoding error.
Flags: needinfo?(jyavenard)
Rank: 29
Component: Audio/Video → Audio/Video: Playback
Priority: P5 → P3
Windows 10 timeout.
Depends on: 1407553
Comment on attachment 8922555 [details] Bug 1406503 - P1. Abstract FFmpeg decoding so that an av_parser can also be used for audio. https://reviewboard.mozilla.org/r/193640/#review198836 ::: dom/media/platforms/ffmpeg/FFmpegAudioDecoder.cpp:124 (Diff revision 1) > return audio; > } > > -RefPtr<MediaDataDecoder::DecodePromise> > -FFmpegAudioDecoder<LIBAV_VER>::ProcessDecode(MediaRawData* aSample) > +MediaResult > +FFmpegAudioDecoder<LIBAV_VER>::DoDecode(MediaRawData* aSample, > + uint8_t* aData, There is a redundancy to pass aData and aSize when they always come from: aData = const_cast<uint8_t*>(aSample->Data()); aSize = aSample->Size(); Can you just pass aSample and calculate aData and aSize in this function? ::: dom/media/platforms/ffmpeg/FFmpegAudioDecoder.cpp:144 (Diff revision 1) > } > > int64_t samplePosition = aSample->mOffset; > media::TimeUnit pts = aSample->mTime; > > - DecodedData results; > + if (aGotFrame) { Put this before any return statements so aGotFrame is always set to a predictable and sensible value. ::: dom/media/platforms/ffmpeg/FFmpegDataDecoder.cpp:65 (Diff revision 1) > return MediaResult(NS_ERROR_OUT_OF_MEMORY, > RESULT_DETAIL("Couldn't init ffmpeg context")); > } > > + if (NeedParser()) { > + mCodecParser = mLib->av_parser_init(mCodecID); Assert mCodecParser is null before we init it.
Attachment #8922555 - Flags: review?(jwwang) → review+
Comment on attachment 8922556 [details] Bug 1406503 - P2. Use common draining mechanism for both audio and video decoder. https://reviewboard.mozilla.org/r/193642/#review198838
Attachment #8922556 - Flags: review?(jwwang) → review+
Comment on attachment 8922557 [details] Bug 1406503 - P3. Also drain av_parser when draining. https://reviewboard.mozilla.org/r/193644/#review198840
Attachment #8922557 - Flags: review?(jwwang) → review+
Comment on attachment 8922559 [details] Bug 1406503 - P5. Do not ignore small flac packets. https://reviewboard.mozilla.org/r/193648/#review198842
Attachment #8922559 - Flags: review?(jwwang) → review+
Comment on attachment 8922558 [details] Bug 1406503 - P4. Re-create av_parser when flushing. https://reviewboard.mozilla.org/r/193646/#review198844
Attachment #8922558 - Flags: review+
Unfortunately, the ffmpeg flac parser introduces a 10 packets latency... So I'll see if I can find another method....
Comment on attachment 8922555 [details] Bug 1406503 - P1. Abstract FFmpeg decoding so that an av_parser can also be used for audio. https://reviewboard.mozilla.org/r/193640/#review198836 > There is a redundancy to pass aData and aSize when they always come from: > > aData = const_cast<uint8_t*>(aSample->Data()); > aSize = aSample->Size(); > > Can you just pass aSample and calculate aData and aSize in this function? well no, because DoDecode can be called (or more accurately will be) in a loop from ProcessDecode. We need the MediaRawData in order to access the time and timecode value. while for now with the current audio code aData is always the same as aSample->Data() this won't be always the case, and it's certainly not the case with vp9 video. the parser will extract sub-frames from the MediaRawData. > Put this before any return statements so aGotFrame is always set to a predictable and sensible value. I don't follow. it is done right at the start, the aGotFrame is always set to false... We only want it set to true in the case where a decoded frame got returned, which isn't always going to be the case if the decoder has some latency > Assert mCodecParser is null before we init it. Init can only ever be called once, but sure...
Comment on attachment 8922555 [details] Bug 1406503 - P1. Abstract FFmpeg decoding so that an av_parser can also be used for audio. https://reviewboard.mozilla.org/r/193640/#review198836 > I don't follow. it is done right at the start, the aGotFrame is always set to false... > We only want it set to true in the case where a decoded frame got returned, which isn't always going to be the case if the decoder has some latency There is a return statement at #136 which happens before you set aGotFrame. Unless stated in the contract of the API, we can't assume *aGotFrame is already set to false by the caller.
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #1) > https://treeherder.mozilla.org/logviewer.html#?job_id=135388461&repo=mozilla- > inbound&lineNumber=6288 > flac-s24.flac-11 timed out! > > [flac @ 000001D88CF598A0] underread: 14 orig size: 3793 > [flac @ 000001D88CF598A0] Multiple frames in a packet. > [flac @ 000001D88CF598A0] underread: 14 orig size: 3793 > [flac @ 000001D88CF598A0] underread: 14 orig size: 3793 The file generated that error is actually small-shot.flac it's badly truncated and the last frame is partial. the demuxer pass in a single packet, both the second last frame, and the partially truncated last frame. With flac, a frame ends when a new one starts, it is not possible to properly determine that the last frame isn't completed without decoding it... One way around this would be to ignore the last frame if we hit EOS. But that mean we would always play the file without the last frame. which would be unfortunate. The simplest alternative would be to re-encode that file so it's valid. I dont know how it got generated, my guess is that someone used a utility like dd to cut it. In any case, this isn't what's causing the intermittent decoding error. Looking at the time of when the errors started, suspiciously seem related to either bug 1388125 or bug 1399751. Bug 1399751 in particular is to do with GetOffset(). The Flac demuxer relies greatly on using Tell() to determine where in the stream it's parsing, so it can rewind back. Could this be related?
Flags: needinfo?(jwwang)
See Also: → 1399751, 1388125
Comment on attachment 8922555 [details] Bug 1406503 - P1. Abstract FFmpeg decoding so that an av_parser can also be used for audio. https://reviewboard.mozilla.org/r/193640/#review198836 > There is a return statement at #136 which happens before you set aGotFrame. Unless stated in the contract of the API, we can't assume *aGotFrame is already set to false by the caller. the return is handling the error, which will abort anything...
Comment on attachment 8922559 [details] Bug 1406503 - P5. Do not ignore small flac packets. So the small-shot.flac file contains an empty flac packet at the end which we were ignoring as it was smaller than 16 bytes. Re-did the FlacDemuxer to fully parse all and only stop after parsing the packet once we've reached EOS
Attachment #8922559 - Flags: review+ → review?(jwwang)
Assignee: nobody → jyavenard
(In reply to Jean-Yves Avenard [:jya] from comment #20) > Looking at the time of when the errors started, suspiciously seem related to > either bug 1388125 or bug 1399751. > > Bug 1399751 in particular is to do with GetOffset(). The Flac demuxer relies > greatly on using Tell() to determine where in the stream it's parsing, so it > can rewind back. > > Could this be related? I don't think so. Bug 1399751 moves the calculation of the block index from the callee to the caller. Since mChannelOffset won't change during the call, so it shouldn't matter whether the block index is calculated by the caller or the callee. We can add assertions to make sure the block index is the same no matter it is calculated by the caller or the callee.
Flags: needinfo?(jwwang)
Comment on attachment 8922559 [details] Bug 1406503 - P5. Do not ignore small flac packets. the review flag got reset somehow...
Attachment #8922559 - Flags: review?(jwwang)
Attachment #8922559 - Flags: review?(jwwang)
Comment on attachment 8922559 [details] Bug 1406503 - P5. Do not ignore small flac packets. https://reviewboard.mozilla.org/r/193648/#review199532 ::: dom/media/flac/FlacDemuxer.cpp:247 (Diff revision 4) > return i; > } > } > } > > - for (; i < aLength; i += 4) { > + for (; i < aLength - 1; i += 4) { Why changing |i < aLength| to |i < aLength - 1|?
Attachment #8922559 - Flags: review?(jwwang) → review+
Comment on attachment 8922559 [details] Bug 1406503 - P5. Do not ignore small flac packets. https://reviewboard.mozilla.org/r/193648/#review199532 > Why changing |i < aLength| to |i < aLength - 1|? to prevent an out of bound read. It didn't matter before because we always have the buffer length = aLength + 16. However, seems to me that it should be - 4 instead, we read by block of 4, and a flac header can't be less than 32 bits long (32 bits is the size of non-variable length data), so we could avoid that last 4 bytes read which will always make FrameHeader::Parse returns false.
Those changes fix the warnings found in comment 1. However, it seems to me that the actually cause for the timeout is due to bug 1407553. None of those messages would have caused a decoding error
Pushed by jyavenard@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5017b9883188 P1. Abstract FFmpeg decoding so that an av_parser can also be used for audio. r=jwwang https://hg.mozilla.org/integration/autoland/rev/a1f28a42c07f P2. Use common draining mechanism for both audio and video decoder. r=jwwang https://hg.mozilla.org/integration/autoland/rev/5f06da1362df P3. Also drain av_parser when draining. r=jwwang https://hg.mozilla.org/integration/autoland/rev/6a20de1a556f P4. Re-create av_parser when flushing. r=jwwang https://hg.mozilla.org/integration/autoland/rev/f96ce9a8172c P5. Do not ignore small flac packets. r=jwwang
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: