Precondition : device is connected via 3G STR 1. Start browser 2. play any video and watch Most of behavior work much better after patch from Bug 880601. However sometimes, there's so many jitters while playing. The video frames show up like a slow motion and sound is cut off frequently. As my opinion, when network connection is not good, player doesn't wait until there's enough data to be decoded in stable. As soon as the data is available, decoder consumes all of them. So, buffering occur repeatedly.

I saw behavior like this and filed bug 882552 about it.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #1) > I saw behavior like this and filed bug 882552 about it. I'm going to dupe this to the root cause bug then.

After adapting patch from Bug 882552, the problem occurred, also. I'am not sure but it feels like the frequency decreased than before.

(In reply to leo.bugzilla.gecko from comment #0) > Precondition : device is connected via 3G > > STR > 1. Start browser > 2. play any video and watch > > Most of behavior work much better after patch from Bug 880601. > However sometimes, there's so many jitters while playing. > The video frames show up like a slow motion and sound is cut off frequently. > > As my opinion, when network connection is not good, player doesn't wait > until there's enough data to be decoded in stable. > As soon as the data is available, decoder consumes all of them. > So, buffering occur repeatedly. Rob: Could the patch from bug 880601 change the behavior of the waiting/playing events? Leo: if your build contained trial patches from bug 881474 or bug 865122 before those patches landed, you might be running code that resumes playback on the 'canplay' event, which is exactly what you hypothesized. I gave r- to the patches that use canplay and they have not landed, but you might check to be sure that your build of the video app is current.

Over to roc to reassign, since this is milestoned for 6/24.

I don't have anyone available to fix this in the next two days.

(In reply to David Flanagan [:djf] from comment #4) > Leo: if your build contained trial patches from bug 881474 or bug 865122 > before those patches landed, you might be running code that resumes playback > on the 'canplay' event, which is exactly what you hypothesized. I gave r- to > the patches that use canplay and they have not landed, but you might check > to be sure that your build of the video app is current. The trial patch that you mentioned is applied even though it's r-. I will ask it to engineer who apply the patch before review.

I have tested with reviewed patch of bug 881474 or bug 865122. However, the problem still occur. When it occurred, there was no spinner on the video frame. And when a video frame lagged severely, sometime audio or video codec stopped by timeout. [OMX.qcom.video.decoder.avc] Timed out waiting for output buffers: 0/3

It seems that this bug is dup of Bug 884678. The symptom could happen in mechanism in Bug 884678 comment #2.

I can reproduce a problem that seems like this bug when I throttle the data rate for http://people.mozilla.com/~roc/troy.3gp to 200kbs. 300kbs is no problem. troy.3gp has a "real" data rate of about 250kbs so during playback we should enter the BUFFERING state periodically, stop all decoding, wait for a significant quantity of data to arrive, and resume playback --- but we don't; we never enter the buffering state. That seems like the underlying bug here. It leads to us running out of data, flipping between HAVE_CURRENT_DATA and HAVE_FUTURE_DATA, and playing audio jerkily and video jerkily or not at all.

It looks to me like we fail to enter the BUFFERING state because GetUndecodedData() consistently returns a too-high value. In the log I'm looking at, even when progress is blocked on network reads, GetUndecodedData() is returning 8.5s. I strongly suspect this is because nsMediaOmxReader::GetBuffered isn't accurate enough. It converts a byte range to a time simply by scaling by duration/totalBytes.

This seems like the right thing to do for b2g18. I think for central, we might want to make this reader-type-specific. When the reader's implementation of GetBuffered is perfectly accurate, we should use it instead of this method.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12) > Created attachment 768161 [details] [diff] [review] > fix buffering calculations > > This seems like the right thing to do for b2g18. > > I think for central, we might want to make this reader-type-specific. When > the reader's implementation of GetBuffered is perfectly accurate, we should > use it instead of this method. I checked the patch and it can make StateMachine to enter the BUFFERING state. It's definitely more fluent than before in and out BUFFERING state. But I have one question about it. Sometimes video codec return time out error and video frame moves suddenly fast to cat up current time stamp after buffering. HasLowUndecodedData() seems like working. But HasLowDecodedData() consider only decoded audio data. (StateMachine can enter BUFFERING state only if both of them is true.) Is it possible that full of cache, enough decoded audio and lack of decoded video can make the problem? https://github.com/bitwiseworks/mozilla-os2/blob/master/content/media/nsBuiltinDecoderStateMachine.cpp#L2281 And in the case of BUFFERING -> DECODING, what happen in the same case?https://github.com/bitwiseworks/mozilla-os2/blob/master/content/media/nsBuiltinDecoderStateMachine.cpp#L2078

(In reply to leo.bugzilla.gecko from comment #13) > But I have one question about it. > Sometimes video codec return time out error and video frame moves suddenly > fast to cat up current time stamp after buffering. > HasLowUndecodedData() seems like working. But HasLowDecodedData() consider > only decoded audio data. (StateMachine can enter BUFFERING state only if > both of them is true.) Is it possible that full of cache, enough decoded > audio and lack of decoded video can make the problem? > https://github.com/bitwiseworks/mozilla-os2/blob/master/content/media/ > nsBuiltinDecoderStateMachine.cpp#L2281 Yes. The problem is that we shouldn't enter the BUFFERING state just to avoid problems with slow video decoding. The purpose of the BUFFERING state is to build up a large amount of undecoded data from the network, so we can then proceed to play that data in a reasonably long burst of playable video before we run out of data gain. This isn't helpful to avoid problems with slow video decoding (since we can only store a few decoded frames). The problem of fast video playback while video "catches up" is really bug 884678, isn't it? Do you think we can close this bug based on my patch? > And in the case of BUFFERING -> DECODING, what happen in the same > case?https://github.com/bitwiseworks/mozilla-os2/blob/master/content/media/ > nsBuiltinDecoderStateMachine.cpp#L2078 I'm not sure what you're asking here. Quick-buffering is not usually what you encounter here so we should be checking undecoded data, not HasLowDecodedData.

Thanks for answer. I asked because your patch is better than before but I think it can be improved more. Because there is one more condition that can block state transition. And more, I think symptom in Bug 884678 can be affected the condition. If you want to handle it here, that's OK

I don't think we should fix 884678 by transitioning to the BUFFERING state. I'll discuss it over there.

I changed my mind. Let's try this to fix HasLowDecodedData. This should be OK because to enter the BUFFERING state, we need to be low on either decoded audio and video, *and* low on undecoded data (or just exited quickbuffering). I.e. just slow video decoding will still not trigger BUFFERING by itself.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16) > I don't think we should fix 884678 by transitioning to the BUFFERING state. > I'll discuss it over there. Sorry, my last comment is mis-typed. The correct one is "If you don't want to handle here....." :) (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #17) > Created attachment 768288 [details] [diff] [review] > Part 2: Allow buffering to be triggered when we're low on decoded audio or > video (as long as we are also low on undecoded data) > > I changed my mind. Let's try this to fix HasLowDecodedData. > > This should be OK because to enter the BUFFERING state, we need to be low on > either decoded audio and video, *and* low on undecoded data (or just exited > quickbuffering). I.e. just slow video decoding will still not trigger > BUFFERING by itself. It's too late now in Korea. So I will give you the feedback of your patch tomorrow. Thanks a lot.

Comment on attachment 768161 [details] [diff] [review] fix buffering calculations Review of attachment 768161 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/nsBuiltinDecoderStateMachine.cpp @@ +1800,4 @@ > > + MediaResource* stream = mDecoder->GetResource(); > + int64_t currentPos = stream->Tell(); > + int64_t requiredPos = currentPos + int64_t((aUsecs/1000000.0)*bytesPerSecond); Divide by double(USECS_PER_S) rather than a literal. @@ +2400,5 @@ > if (mState == DECODER_STATE_DECODING && > mDecoder->GetState() == nsBuiltinDecoder::PLAY_STATE_PLAYING && > HasLowDecodedData(remainingTime + EXHAUSTED_DATA_MARGIN_USECS) && > !resource->IsDataCachedToEndOfResource(mDecoder->mDecoderPosition) && > + !resource->IsSuspended()) { Why move the (JustExitedQuickBuffering() || HasLowUndecodedData()) check out of the preceding "if" condition and into a new one below? You're just adding more indentation. Just move the curly brace from line 2414 up if you prefer.

Comment on attachment 768288 [details] [diff] [review] Part 2: Allow buffering to be triggered when we're low on decoded audio or video (as long as we are also low on undecoded data) Review of attachment 768288 [details] [diff] [review]: ----------------------------------------------------------------- It's true that the return value of HasLowDecodedData() is only used to affect buffering decisions when HasLowUndecodedData() returns true, but HasLowDecodedData() doesn't enforce this. i.e. your commit message is misleading. (And for the record, the reason the !HasAudio() check that you're deleting existed, was because if we're playing on hardware which isn't powerful enough to decode video in real time, we don't want to keep stopping the playback every time the video decode falls behind (which typically happens once we've played all the frames that we decoded and enqueued when we stopped to buffer, i.e. after we've played 10 frames (3 on b2g)). The callers of HasLowDecodedData() now call HasLowUndecodedData(), so this isn't an issue).

https://hg.mozilla.org/releases/mozilla-b2g18/rev/5cbc62dcdf0c https://hg.mozilla.org/releases/mozilla-b2g18/rev/17482b17d2e8

These patches are remove all symptoms here and Bug 884678.

Excellent!

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/5cbc62dcdf0c https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/17482b17d2e8 Closing under the assumption that this was b2g18-only since I'm not seeing any m-c landings.

Something is needed on m-c, so reopening.

Remove leo flag

I think V1.2 is needed too.

I think V1.2 branch is needed too.

Thanks for notifying! I am going to create a new bug for it. This bug was already fixed long time ago.

(In reply to Sotaro Ikeda [:sotaro] from comment #30) > Thanks for notifying! I am going to create a new bug for it. This bug was > already fixed long time ago. Bug 935118 is a bug for it.

bug 935118 has been filed to track porting the patch to bug 935118, so I'm closing this bug out.