Both RemoteDataDecoder and MediaCodecDataDecoder on Android always return a frame with the resolution set to the initial frame seen. https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/dom/media/platforms/android/MediaCodecDataDecoder.cpp#112 https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/dom/media/platforms/android/RemoteDataDecoder.cpp# This create an image with the resolution set from the original VideoConfig first seen. This is wrong, as it means all video frames coming out of the decoder will always have the same size. While there's a mechanism in place in the RemoteDataDecoder to detect change of content format, there's none for MediaCodecDataDecoder. We used to have a mechanism to inform the MediaDataDecoder that a change of format was coming (bug 1217226), but this was removed in bug 1297311. We need to have something back in place. We have mochitest to test, but they are disabled on android which is why the regression wasn't picked up

This cause test dom/media/mediasource/test/test_FrameSelection.html to timeout

Jya, Thanks for firing this bug. It looks like we need to enable mochitest (mediasource) on Android to prevent this kind of regression. Ni John to let him aware of this bug.

Actually, it works on try because they are very slow machines and the mConfig object stored as a reference is modified in another thread. Of course this is racy and it happens to work on try. On a real machine it doesn't. The h264 test is disabled however on android

There are several issues at play when using the recycled mode: 1) VideoInfo mConfig is accessed in a non-thread safe manner and is racy. It works on try because the machines are very slow. On a real Sony Z3C any mochitests checking the video resolution will actually fail. 2) The surface texture is only ever allocated once, during init. So regardless of the new resolution, the same texture will always be used. This is probably okay when the resolution drops, but likely very bad if the resolution increases. 3) If the aspect ratio changes during the change of resolution, this will be completely ignored. Additionally, the only way to guarantee that the decoded frames are matched to a valid VideoConfig, is to ensure, as specified in the Android documentation that the decoder is first drained *and* flushed (or shutdown). None of this is done currently. Whichever branch saw bug 1299105 uplifted to, must be fixes ASAP.

(In reply to Jean-Yves Avenard [:jya] from comment #4) > 1) VideoInfo mConfig is accessed in a non-thread safe manner and is racy. It > works on try because the machines are very slow. On a real Sony Z3C any > mochitests checking the video resolution will actually fail. Yes. race condition should be fixed. > 2) The surface texture is only ever allocated once, during init. So > regardless of the new resolution, the same texture will always be used. This > is probably okay when the resolution drops, but likely very bad if the > resolution increases. KEY_MAX_(WIDTH|HEIGHT) are set [1] so I guess it's okay if the resolution is under 1920x1080. Perhaps forcing decoder recreation when resolution is larger than that? > 3) If the aspect ratio changes during the change of resolution, this will be > completely ignored. It looks fine with Alfredo's aspect ratio change test video [2]. Do we have other tests to verify it? > Additionally, the only way to guarantee that the decoded frames are matched > to a valid VideoConfig, is to ensure, as specified in the Android > documentation that the decoder is first drained *and* flushed (or shutdown). > None of this is done currently. > AIUI, decoders that support adaptive playback don't need the process you mentioned. It only requires an IDR frame. [3] [1] http://searchfox.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/media/Codec.java#347 [2] https://people-mozilla.org/~ayang/mp4/aspect_mp4.html [3] Sec. "For decoders that support and are configured for adaptive playback" in https://developer.android.com/reference/android/media/MediaCodec.html

The aspect ratio appears to work because of the race. If you fixed the race, there are two ways we can do the detection of change of resolution. 1- Using the HandleOutputFormatChanged mechanism. The issue here however is that the MediaFormat structure only provides the image resolution, not how it should be displayed. So that's not really usable 2- Use the H264Converter to parse the SPS prior calling the Decoder and tell the decoder that a new format is coming. Here however, we must ensure that we can match the right VideoInfo with the right decoded data. The easiest way to ensure this is when a change of resolution is detected, we first drain the decoder; that way we know we only have one VideoInfo object to worry about. Per the doc, once you've drained the decoder you must either flush it or shut it down. https://developer.android.com/reference/android/media/MediaCodec.html#BUFFER_FLAG_END_OF_STREAM and in the States chapter: " In this state the codec no longer accepts further input buffers, but still generates output buffers until the end-of-stream is reached on the output. You can move back to the Flushed sub-state at any time while in the Executing state using flush(). " Flushing after draining isn't something we currently do; it appears that we should. This should be handled by the MFR. Currently, if the recycle flag is set, no draining occurs when changing resolution.

Tracking for 52 as the first version with bug 1299105. Let me know if that should get backed out of beta.

Jean-Yves/John, sounds like one of you would end up being assigned to this?

James, would you like to take this one?

I think jya has the preliminary solution for this bug, should confirm with jya if he planed to work on this.

Comment on attachment 8836869 [details] Bug 1336431: P2. Asynchronously flush and shutdown decoder when SPS changes. https://reviewboard.mozilla.org/r/112168/#review113462

Comment on attachment 8836868 [details] Bug 1336431: P1. Fix coding style. https://reviewboard.mozilla.org/r/112166/#review113482

Comment on attachment 8836870 [details] Bug 1336431: P3. Don't attempt to decode non-keyframe. https://reviewboard.mozilla.org/r/112170/#review113484

Comment on attachment 8836871 [details] Bug 1336431: P4. Rename SharedTrackInfo. https://reviewboard.mozilla.org/r/112172/#review113492 The simplest patches are the best for bike-shedding! :-) r+ as it does the job, though I have some reservations... Here's a suggestion, see what you think: ::: dom/media/MediaInfo.h:559 (Diff revision 1) > uint32_t GetID() const > { > return mStreamSourceID; > } > > + operator const TrackInfo*() const This means that any SharedTrackInfo *object* can implicitly be converted into a TrackInfo *pointer*. This seems a bit awkward and dangerous! Suggestion: Why not have a `const TrackInfo* GetTrackInfo()` function instead, it's consistent with the other `GetAs...()` functions below, and it would make things much more obvious at the call sites.

Comment on attachment 8836872 [details] Bug 1336431: P5. Re-add ConfigurationChanged API. https://reviewboard.mozilla.org/r/112174/#review113538

Comment on attachment 8836873 [details] Bug 1336431: P6. Handle change of resolution in RemoteDataDecoder. https://reviewboard.mozilla.org/r/112176/#review113542 Shall we add ConfigurationChanged API in MediaCodecDataDecoder http://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/dom/media/platforms/android/MediaCodecDataDecoder.cpp#126

Comment on attachment 8836873 [details] Bug 1336431: P6. Handle change of resolution in RemoteDataDecoder. https://reviewboard.mozilla.org/r/112176/#review113542 We should. But considering it's going to be disabled, I think we should remove it altogether

Comment on attachment 8836871 [details] Bug 1336431: P4. Rename SharedTrackInfo. https://reviewboard.mozilla.org/r/112172/#review113492 > This means that any SharedTrackInfo *object* can implicitly be converted into a TrackInfo *pointer*. This seems a bit awkward and dangerous! > > Suggestion: Why not have a `const TrackInfo* GetTrackInfo()` function instead, it's consistent with the other `GetAs...()` functions below, and it would make things much more obvious at the call sites. As discussed, renamed SharedTrackInfo into TrackInfoSharedPtr

Pushed by jyavenard@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c5ae7452946b P1. Fix coding style. r=gerald https://hg.mozilla.org/integration/autoland/rev/e6f6b3736d93 P2. Asynchronously flush and shutdown decoder when SPS changes. r=cpearce https://hg.mozilla.org/integration/autoland/rev/355c87406681 P3. Don't attempt to decode non-keyframe. r=gerald https://hg.mozilla.org/integration/autoland/rev/07c07a331cd2 P4. Rename SharedTrackInfo. r=gerald https://hg.mozilla.org/integration/autoland/rev/ad29077e3c80 P5. Re-add ConfigurationChanged API. r=JamesCheng https://hg.mozilla.org/integration/autoland/rev/ae2b48854492 P6. Handle change of resolution in RemoteDataDecoder. r=JamesCheng

https://hg.mozilla.org/mozilla-central/rev/c5ae7452946b https://hg.mozilla.org/mozilla-central/rev/e6f6b3736d93 https://hg.mozilla.org/mozilla-central/rev/355c87406681 https://hg.mozilla.org/mozilla-central/rev/07c07a331cd2 https://hg.mozilla.org/mozilla-central/rev/ad29077e3c80 https://hg.mozilla.org/mozilla-central/rev/ae2b48854492

Please request Aurora and Beta (if you're comfortable with it riskwise) approval on this when you get a chance.

These changes can't be applied as-is. It will require a significant rewrite for aurora and beta.

I still think this is something that should be fixed in 52 and 53. Just can't use these patches as-is. Personally, I would vote for backing out the original change that broke this feature. I have voiced my concern in the past about it: it's racy, and makes our MSE/HTML5 implementation buggy.

(In reply to Jean-Yves Avenard [:jya] from comment #41) > I still think this is something that should be fixed in 52 and 53. Just > can't use these patches as-is. > > Personally, I would vote for backing out the original change that broke this > feature. I have voiced my concern in the past about it: it's racy, and makes > our MSE/HTML5 implementation buggy. If bug 1340124 is really serious, we may need to considering backing out. I am going to take a deeper view in bug 1340124. John, What do you think? Should we back out the original change?

(In reply to Blake Wu [:bwu][:blakewu] from comment #42) > If bug 1340124 is really serious, we may need to considering backing out. I > am going to take a deeper view in bug 1340124. > > John, > What do you think? Should we back out the original change? With 52 going to be released with ~2 weeks, I don't think it's worth backing out. Of course there will be some contents that looks wrong, but it's hard to tell how serious this problem is in the field. On the other hand, constantly dropping frames every now and then seems worse to me. My suggestion would be leaving it as is in 52, and address this in 53+.

(In reply to John Lin [:jolin][:jhlin] from comment #43) > (In reply to Blake Wu [:bwu][:blakewu] from comment #42) > > > John, > > What do you think? Should we back out the original change? > > With 52 going to be released with ~2 weeks, I don't think it's worth > backing out. Of course there will be some contents that looks wrong, but > it's hard to tell how serious this problem is in the field. On the other > hand, constantly dropping frames every now and then seems worse to me. > > My suggestion would be leaving it as is in 52, and address this in 53+. ???? It's not a matter of looking wrong. It's a security risk, how can we still be discussing this?

(In reply to Jean-Yves Avenard [:jya] from comment #44) > > ???? > It's not a matter of looking wrong. > > It's a security risk, how can we still be discussing this? Oops! My bad. Didn't noticed Blake was actually talking about bug 1340124 instead of this one. Sorry about that! Please see bug 1340124 comment 2 for my suggestion with respect to the security issue.

Yeah. Let's focus on bug 1340124 and check the severity of it.

Fixed on 52 by the backout in bug 1340480. Seems like we could still use a better decision for 53 one way or another so we're not left in an inconsistent state across releases. https://hg.mozilla.org/releases/mozilla-beta/rev/b70b7650fa1d