With MediaRecorder we are recording realtime streams that may change resolution at any time. We are currently prohibiting it but it's fairly easy to fix on the recording side. However, playback of such files result in an error. I made some quick fixes to MediaRecorder to record a sample file (attached) which contains multiple resolution changes using this fiddle: http://jsfiddle.net/ot29yhy4/ Playing the sample file in Firefox 45 fails. ffplay (I have version 2.6.3) is the most reliable player of it that I have found.

Use media.autoplay.enabled=false

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #1) > Use media.autoplay.enabled=false Wrong bug.

I've added the same testcase, but run through mkclean. The original test case doesn't have cues, which can complicate the debugging. We should ideally be able to play both, but this test case will hopefully aid in isolating the resolution issues.

This issue is rather tricky as we have two cases to cover. To pass some particular reftest (like https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/reftests/webm-video/offset-1.xhtml) we had to support the offset/cropping information as defined in the container. Those offsets aren't defined in the VPx streams, only in the container FFmpeg and Libvpx will handle change of resolutions in the VP stream just fine, they will simply return an image with the new resolution. However, FFmpeg doesn't know anything about those offset/cropping values. so to pass the reftest, I made it so it always uses the display size as found in the demuxer. Obviously, this information is static, so when FFmpeg returns a picture of a different size, the original display size is no longer valid. So there are several ways we can handle it: 1- Detect change of VP streams in the demuxer, and using a similar mechanism to MSE, create a new decoder with the new display information 2- Have something like we do for the H264Converter, which monitor the h264 stream and detect change of resolutions. It can then tell the decoder that the configuration has changed via MediaDataDecoder::ConfigurationChanged , or just shutdown/recreate a decoder 3- Have FFmpeg reports not just the dimensions of the video, but also cropping and offset information 4- Hack around so we pass the reftest. 3 is unlikely, because as mentioned, this info is only found in the demuxer. 1 or 2 would be ideal, but requires more effort. offset/cropping in webm are likely super rare, so rare that the ffvp9 author had never heard about it until I asked him on how we could best handle it. The only file I've ever seen with those is that particular reftest so spending time just to pass that reftest seems like a waste to me. So what I propose is 4. We first start using the dimensions as defined in the demuxer. If FFmpeg ever returns a picture of a different size, then we use the dimensions as calculated by ffmpeg and forget about cropping/offset.

Hi Bryce, Turns out the bug I asked you about (Bug 1213775) is not a dup of this one. My bug results from the fact that ffmpeg decoder doesn't yet support resize; so I'm turning off resize for now (until ffmpeg is fixed). Is *this* bug likely to land before Firefox 47 goes to Dev Edition? The reason I ask: Soledad (from Tech Evangelism) and I are hoping to do a blog post on Media Recording in a couple of weeks pointing devs to Fx 47 (after it goes to Dev Edition), and it'd be great if this bug were fixed by then. If it's unlikely to be fixed in Fx 47, that's important for me to know. Thanks.

See Comment 5.

(In reply to Maire Reavy [:mreavy] (Plz ni?:mreavy) from comment #6) > See Comment 5. This the next item on my queue to pick up, I have some WIP on it, but am currently trying to knock out 657791, which is proving a bit vexatious. As a complete rough estimate I'm only somewhat confident (50% say) of having this fix landed in full before 47 moves into dev. There's still a number of unknowns in terms of my current work and this tick. If it would help I can update you as things become more clear.

Thanks, Bryce. An update when you know more (perhaps later this week?) would be *very* helpful and much appreciated.

Hey Bryce -- I have a much better understanding now of the big picture, and I think this bug should be low priority. Here's why: As mentioned above, we've turned off internal resizing since ffmpeg doesn't support it; even if we can make it play in Firefox (work around the ffmpeg bug), it would still break standalone players based on ffmpeg. Input resolution changes do not result in a change of resolution in the output stream - pehrson's patch a while back causes it to drop any frame that doesn't match the configured resolution. I don't believe files with purposeful output-resolution changes are common on the web, so these together are why I think this should be low priority. Independently we will modify the MediaRecorder to rescale input frames if the input resolution changes (right now we simply reject them), which will fix some MediaRecorder usecases (rotating an android phone, recording a webrtc call that may shift resolutions, etc). Bug 657791 (fixing seeking in recorded blobs) is definitely top priority, and is what I believe what you're focused on now. If you can let me know the status of *that* bug and if it will make the uplift, I'd really appreciate it. Thanks!

Update the WebMDemuxer to detect changes in resolution. When it does so it changes the streamID so that we get a new decoder created to handle the resolution change. The demuxer will also update media info in these cases, so the new decoder has the correct information. The demuxer will only handle resolution changes on key frames, files that attempt changes other times are not considered valid at this stage. If a resolution change cannot be performed because nest_egg cannot read track info, or because the new resolution is invalid, a change will not take place. Review commit: https://reviewboard.mozilla.org/r/39695/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/39695/

Review commit: https://reviewboard.mozilla.org/r/39697/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/39697/

Comment on attachment 8730145 [details] MozReview Request: Bug 1232045 - WebMDemuxer handles resolution changes r=jya https://reviewboard.mozilla.org/r/39695/#review36437 ::: dom/media/webm/WebMDemuxer.cpp:43 (Diff revision 1) > > // How far ahead will we look when searching future keyframe. In microseconds. > // This value is based on what appears to be a reasonable value as most webm > // files encountered appear to have keyframes located < 4s. > #define MAX_LOOK_AHEAD 10000000 > Don't forget to include mozilla/Atomics.h so that you don't break non-unified builds. ::: dom/media/webm/WebMDemuxer.cpp:582 (Diff revision 1) > + isResolutionChange = si.w != mLastSeenFrameWidth.value() || > + si.h != mLastSeenFrameWidth.value(); > + } > + if (isResolutionChange) { > + nestegg_video_params params; > + r = nestegg_track_video_params(mContext, mVideoTrack, &params); this seems unecessary to me. This will always return the same information as you can only have a single metadata in a webm. So there's no point in querying nestegg whenever a resolution change has occurred, you already have the info in mInfo. ::: dom/media/webm/WebMDemuxer.cpp:586 (Diff revision 1) > + nestegg_video_params params; > + r = nestegg_track_video_params(mContext, mVideoTrack, &params); > + if (r == -1) { > + WEBM_DEBUG("nestegg_track_video_params failed r=%d", r); > + return false; > + } else { can remove the else, seeing that you are returning before. ::: dom/media/webm/WebMDemuxer.cpp:624 (Diff revision 1) > } > + if (aType == TrackInfo::kVideoTrack) { > + if (isResolutionChange) { > + ++sStreamSourceID; > + } > + sample->mTrackInfo = new SharedTrackInfo(mInfo.mVideo, sStreamSourceID); you don't want to allocate a new object for every frames returned. SharedTrackInfo is a ref counted object for a reason. Keep a reference to the current SharedTrackInfo and only update it when the resolution change. All frames can use a reference to the same object

Comment on attachment 8730146 [details] MozReview Request: Bug 1232045 - Add test file for WebM with resolution changes r?jya https://reviewboard.mozilla.org/r/39697/#review36441 Please add a new test, one that check that the video element's width / height did change from the start by the time the video has ended.

Comment on attachment 8730145 [details] MozReview Request: Bug 1232045 - WebMDemuxer handles resolution changes r=jya Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39695/diff/1-2/

https://reviewboard.mozilla.org/r/39697/#review36441 Right now we're holding the display dimensions of the element at the same values as read from the metadata, but the resolution of the video frames can change. Is there a way to test the internal resolution changing? Or are you thinking we should resize the element?

(In reply to Bryce Van Dyk (:SingingTree) from comment #17) > https://reviewboard.mozilla.org/r/39697/#review36441 > > Right now we're holding the display dimensions of the element at the same > values as read from the metadata, but the resolution of the video frames can > change. Is there a way to test the internal resolution changing? Or are you > thinking we should resize the element? as I mentioned before, display dimensions should be reset and completely ignored because the values in the metadata are no longer relevant. Display dimensions aren't used by the MediaRecorder anyway. Once the metadata has been read (via MediaTrackDemuxer::GetInfo()) they won't be read again anyway. But you certainly don't want to have the old clipping bounds in use with the new frames pass the resolution change. The element will be resized automatically as new frames come from the decoder. there's nothing you need to do in the demuxer for this to happen. In your mochitest however, simply read the video element's width&height and see if they have been updated.

The behaviour I'm seeing is that the demuxer's interaction with mInfo.mVideo.mDisplay affects the size of the media element on the page. If I update that to the new frame size we will get resizing of the element happening (akin to ffmpeg based players I've tested with), if I leave it as the old value the element will remain static and the frames with be stretched to fit (I believe chrome does this). In the former case I see elem.videoHight and elem.videoWidth change.

(In reply to Bryce Van Dyk (:SingingTree) from comment #19) > The behaviour I'm seeing is that the demuxer's interaction with > mInfo.mVideo.mDisplay affects the size of the media element on the page. If only because it is passed to the decoder's constructor. if the sample contains a different SharedMediaInfo then it won't And if you're seeing what you describe, it's because you create your new SharedMediaInfo with the old display resolution which is wrong

Comment on attachment 8730145 [details] MozReview Request: Bug 1232045 - WebMDemuxer handles resolution changes r=jya https://reviewboard.mozilla.org/r/39695/#review36537 ::: dom/media/webm/WebMDemuxer.cpp:368 (Diff revision 2) > uint64_t duration = 0; > r = nestegg_duration(mContext, &duration); > if (!r) { > mInfo.mVideo.mDuration = media::TimeUnit::FromNanoseconds(duration).ToMicroseconds(); > } > + mSharedVideoTrackInfo = new SharedTrackInfo(mInfo.mVideo, sStreamSourceID); you don't need this.. you only need to create it once a change of resolution actuallly occurs. until then setting sample.mTrackInfo to null is sufficient. ::: dom/media/webm/WebMDemuxer.cpp:588 (Diff revision 2) > + // We ignore cropping information on resizes during streams. > + // Cropping alone is rare, and we do not consider cropping to > + // still be valid after a resolution change > + nsIntRect newPictureRect(0, 0, si.w, si.h); > + nsIntSize frameSize(si.w, si.h); > + if (IsValidVideoRegion(frameSize, newPictureRect, mInfo.mVideo.mDisplay)) { we now that mVideo.mDisplay is to be disregarded once a resolution change is occurring. But you're still using it to ensure that the frame isn't too big. IsValidVideoRegion(frameSize, newPictureRect, newPictureRect). you can also reset mInfo.mVideo.mDisplay even though this serve no purpose as mInfo is never re-read once GetInfo() has been called. ::: dom/media/webm/WebMDemuxer.cpp:615 (Diff revision 2) > BigEndian::writeInt64(&c[0], discardPadding); > sample->mExtraData = new MediaByteBuffer; > sample->mExtraData->AppendElements(&c[0], 8); > } > + if (aType == TrackInfo::kVideoTrack) { > + if (isResolutionChange) { a webm block may contains multiple packets. if it happens that you have multiple blocks and that the first packet is a key frame, you will incorrectly increment sStreamSourceID for every packet in the block. you need to set isResolutionChange to false

(In reply to Jean-Yves Avenard [:jya] from comment #20) > And if you're seeing what you describe, it's because you create your new > SharedMediaInfo with the old display resolution which is wrong This appears to be something that something that players do not handle in a uniform way. All the stand alone players I've tested resize, though they're inconsistent in how much they resize. Chrome, Opera, and IE (with WebM extension) all retain the initial display dimensions. Do we have any desire to have parity with the other browsers in this regard? > > + // We ignore cropping information on resizes during streams. > > + // Cropping alone is rare, and we do not consider cropping to > > + // still be valid after a resolution change > > + nsIntRect newPictureRect(0, 0, si.w, si.h); > > + nsIntSize frameSize(si.w, si.h); > > + if (IsValidVideoRegion(frameSize, newPictureRect, mInfo.mVideo.mDisplay)) { > > we now that mVideo.mDisplay is to be disregarded once a resolution change is > occurring. > > But you're still using it to ensure that the frame isn't too big. > > IsValidVideoRegion(frameSize, newPictureRect, newPictureRect). > > you can also reset mInfo.mVideo.mDisplay even though this serve no purpose > as mInfo is never re-read once GetInfo() has been called. > I'm not sure I entirely follow. My understanding is that mDisplay needs to be repopulated with whatever we decide the new display resolution is as the part of the new decoder creation (also so the info shared with the samples in consistent). Is that correct? Are you saying it shouldn't be repopulated, or that it should be reset at some point after that repopulation? > > + mSharedVideoTrackInfo = new SharedTrackInfo(mInfo.mVideo, sStreamSourceID); My main desire for doing this is to ensure that mSharedVideoTrackInfo is populated in a consistent way -- always populated if we have video info, rather than only following a resolution change. It makes the runtime logic more easily understood to me, though I'm open to if you disagree.

(In reply to Bryce Van Dyk (:SingingTree) from comment #22) > (In reply to Jean-Yves Avenard [:jya] from comment #20) > > And if you're seeing what you describe, it's because you create your new > > SharedMediaInfo with the old display resolution which is wrong > > This appears to be something that something that players do not handle in a > uniform way. All the stand alone players I've tested resize, though they're > inconsistent in how much they resize. Chrome, Opera, and IE (with WebM > extension) all retain the initial display dimensions. Do we have any desire > to have parity with the other browsers in this regard? you're confusing the cropping information found in the webm metadata mInfo.mVideo.mDisplay (and independent from the VP9/VP8 stream itself) and the actual picture size (mInfo.mVideo.mImage). All of this is also different to the dimension used to display the video element itself. When the resolution change, you do need to adjust both mDisplay and mPicture, such as mDisplay == mImage. Your changes do not reset mDisplay and continue to use the original dimension: this is wrong There's no one to copy, because no one was handling mDisplay properly to start with, we were the only one to do it right. However, the concept of cropping/display size only exists in the webm metadata and seeing that webm is defined to only handle a single VP9 stream it only applies to the first stream found. But when we now allow a change of VPn content part way, we break that assumption. The stream is no longer a valid webm stream and that only thing we can now decide is that the display/cropping information is no longer valid nor relevant. How the video element will be displayed is a different matter anyway. You can change the resolution of the element yet the visual dimensions of the player remain the same. Go you youtube.com, start playback and change the resolution : the element's dimensions remain the same, yet the resolution has been changed. > I'm not sure I entirely follow. My understanding is that mDisplay needs to > be repopulated with whatever we decide the new display resolution is as the > part of the new decoder creation (also so the info shared with the samples > in consistent). Is that correct? Are you saying it shouldn't be repopulated, > or that it should be reset at some point after that repopulation? if course it needs to be repopulated, but you aren't ! Where in your patch do you update mDisplay ?? You write "// We ignore cropping information on resizes during streams." but how can you be ignoring cropping information if you do not update mDisplay? What you need is update mDisplay but only so that the new mTrackInfo.mImage == mTrackInfo.mDisplay I'm not sure where any of my earlier comment could induce confusion. > My main desire for doing this is to ensure that mSharedVideoTrackInfo is > populated in a consistent way -- always populated if we have video info, this is different to the issue above. Of course you can set mSharedVideoTrackInfo whenever you want. All I'm saying is that if the original mInfo.mVideo == sample.mSharedVideoTrackInfo , then you don't need to set mSharedVideoTrackInfo. What I didn't want to see however was that you were allocating a complete new object on the stack for every single sample!

I thought that we use the DisplayWidth/Height from the WebM to set the display area in the browser (which appears to be what the other browsers are doing). In this case pixelWidth/Height are used to derive the frameSize (uncropped) as well as the cropping rectangle. In this case I'm reading the Display* element solely to hint to us the desired display size in browser. Is this not the case? The reason I'm not updating the mDisplay in the current revision is based on the above and that I am uncertain as to if we wished to change the size of the element. I believe we apply cropping to the pixelWidth/Height to obtain our mImage. So here I'm only dropping the crop from the peeked frame width and height, as my current understand is that crop does not interact directly with displayWidth/Height.

(In reply to Bryce Van Dyk (:SingingTree) from comment #24) > I thought that we use the DisplayWidth/Height from the WebM to set the > display area in the browser (which appears to be what the other browsers are > doing). In this case pixelWidth/Height are used to derive the frameSize > (uncropped) as well as the cropping rectangle. In this case I'm reading the > Display* element solely to hint to us the desired display size in browser. > Is this not the case? you're reading wrong. https://dxr.mozilla.org/mozilla-central/source/dom/media/platforms/agnostic/VPXDecoder.cpp#155 https://dxr.mozilla.org/mozilla-central/source/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp#276 you need to update mDisplay. mDisplay contains the cropping information as well as how you should offset the data from the top left corner. You do *not* want to crop the data. Say you open a 320x240 video with no offset, so mDisplay will be (0,0, 320, 240) now you change the resolution so that mImage is now 1920x1080. Your 1920x1080 will be cropped to 320x240.

I should add: the original reading of the metadata is what set the original size of the video element. New pictures coming out will be *scaled* based on those dimensions. This is independent of the *cropping* info found in mDisplay and passed to the MediaDataDecoder in VideoConfig cropping != scaling.

I believe the above case will result in scaling of the image rather than cropping. In the decoder if I force a mDisplay smaller than the video resolution I see the entire frame, but scaled Smaller. If I alter mImage, I see cropping of the frames, which are then scaled to be drawn at the mDisplay dimensions. Is it not the case that mImage contains the cropping information?

Not updating mDisplay also mean the resulting image will be displayed with the wrong aspect ratio (as it will keep using the original aspect ratio) Having said that, anything that doesn't have a PAR of 1:1 will be displayed wrong anyway

Comment on attachment 8730145 [details] MozReview Request: Bug 1232045 - WebMDemuxer handles resolution changes r=jya Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39695/diff/2-3/

Added a test WebM file containing resolution changes. This file has been added to gPlayTests to test playback of such files. A new test has been added, test_resolution_change.html, which reads a new test array, gResolutionChangeTests, of media files containing resolution changes. This new test check that after playing a file through at least one resolution change has taken place. Review commit: https://reviewboard.mozilla.org/r/40599/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40599/

Comment on attachment 8731436 [details] MozReview Request: Bug 1232045 - Add tests for WebM with resolution changes r=jya https://reviewboard.mozilla.org/r/40599/#review37153 ::: dom/media/test/test_resolution_change.html:29 (Diff revision 1) > + v.prevVideoWidth = v.videoWidth; > + v.prevVideoHeight = v.videoHeight; > + } > + > + function timeUpdate(e) { > + var v = e.target; What's the reason behind checking timeupdate ? seems irrelevant to me. You have a resize event that is fired if the dimensions are changing, you need to ensure that this event is fired. https://html.spec.whatwg.org/multipage/embedded-content.html#event-media-resize So you can check that the resize event was received prior ended. resize will be received twice (one for the first setting prior loadedmetadata) and once if the resolution change later. May want to only listen for the resize event once playback has started or canplay has been received.

Comment on attachment 8730145 [details] MozReview Request: Bug 1232045 - WebMDemuxer handles resolution changes r=jya https://reviewboard.mozilla.org/r/39695/#review37147 ::: dom/media/webm/WebMDemuxer.cpp:368 (Diff revision 3) > uint64_t duration = 0; > r = nestegg_duration(mContext, &duration); > if (!r) { > mInfo.mVideo.mDuration = media::TimeUnit::FromNanoseconds(duration).ToMicroseconds(); > } > + mSharedVideoTrackInfo = new SharedTrackInfo(mInfo.mVideo, sStreamSourceID); I still think it is unecessary to allocate that object at this point. because you can do it when the first frame is retrieved (which will always be a keyframe). Later you test if the resolution has changed, simply change your test below ::: dom/media/webm/WebMDemuxer.cpp:578 (Diff revision 3) > } > isKeyframe = si.is_kf; > + if (isKeyframe) { > + // We only look for resolution changes on keyframes for both VP8 and > + // VP9. Other resolution changes are invalid. > + if (mLastSeenFrameWidth.isSome() && mLastSeenFrameHeight.isSome()) { we know that the first frame demuxed (or after a reset) will always be a keyframe. So if you really want to always set mSharedVideoTrack for all samples and not just after a resolution change, you can change the test to: if ((mLastSeenFrameWidth.isNothing() || mLastSeenFrameHeight.isNothing()) || ((mLastSeenFrameWidth.isSome() && mLastSeenFrameHeight.isSome()) so all allocations of mSharedVideoTrackInfo is done in the same place, makes it easier to follow up ::: dom/media/webm/WebMDemuxer.cpp:616 (Diff revision 3) > uint8_t c[8]; > BigEndian::writeInt64(&c[0], discardPadding); > sample->mExtraData = new MediaByteBuffer; > sample->mExtraData->AppendElements(&c[0], 8); > } > + if (aType == TrackInfo::kVideoTrack) { move that above, in the if (resolutionChange) I don't see why you separate things like that. isResolutionChange is not necessary that way either. You don't even have to test for the track type either, seeing that mSharedVideoTrackInfo will be null there anyway.

https://reviewboard.mozilla.org/r/39695/#review37147 > move that above, in the if (resolutionChange) > > I don't see why you separate things like that. > > isResolutionChange is not necessary that way either. > > You don't even have to test for the track type either, seeing that mSharedVideoTrackInfo will be null there anyway. The reason I did this here was to group the assignment of the new info with the with it's assignment to the sample. However I've now moved that up and consoladated the Info assignments into the video checking section above for the next revision. mSharedVideoTrack info may be populated if we're hitting this in the audio path, yes? So a check on the type is still needed. Alternatively we could allocate the sample earlier and have the mTrackInfo assignment be handled in the video conditional.

Comment on attachment 8730145 [details] MozReview Request: Bug 1232045 - WebMDemuxer handles resolution changes r=jya Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39695/diff/3-4/

Comment on attachment 8731436 [details] MozReview Request: Bug 1232045 - Add tests for WebM with resolution changes r=jya Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40599/diff/1-2/

(In reply to Bryce Van Dyk (:SingingTree) from comment #34) > mSharedVideoTrack info may be populated if we're hitting this in the audio > path, yes? So a check on the type is still needed. Alternatively we could > allocate the sample earlier and have the mTrackInfo assignment be handled in > the video conditional. well, no it won't as you're already in a else if (aType == TrackInfo::kVideoTrack) { ... } block, if you aren't then your patch is wrong :)

Comment on attachment 8730145 [details] MozReview Request: Bug 1232045 - WebMDemuxer handles resolution changes r=jya https://reviewboard.mozilla.org/r/39695/#review37177 ::: dom/media/webm/WebMDemuxer.cpp:577 (Diff revisions 3 - 4) > } > isKeyframe = si.is_kf; > if (isKeyframe) { > // We only look for resolution changes on keyframes for both VP8 and > // VP9. Other resolution changes are invalid. > + bool isFirstFrame = false; you don't need this variable. ::: dom/media/webm/WebMDemuxer.cpp:582 (Diff revisions 3 - 4) > + bool isFirstFrame = false; > if (mLastSeenFrameWidth.isSome() && mLastSeenFrameHeight.isSome()) { > isResolutionChange = si.w != mLastSeenFrameWidth.value() || > si.h != mLastSeenFrameHeight.value(); > + } else { > + isFirstFrame = true; you're here if neither lastSeenFrameWidth or mLastSeenFrameHeight is defined. I would simplify that entire thing with: if (mLastSeenFrameHeight.isNothing() || (si.w != mLastSeenFrameWidth.value() || si.h != mLastSeenFrameHeight.value()) { ... } and create mSharedVideoTrackInfo there then no need for isFirstFrame / isResolutionChange and all this unecessary nesting. that becomes if (isKeyframe) { // We only look for resolution changes on keyframes for both VP8 and // VP9. Other resolution changes are invalid. if (mLastSeenFrameWidth.isNothing() || (si.w != mLastSeenFrameWidth.value() || si.h != mLastSeenFrameHeight.value()) { // We ignore cropping information on resizes during streams. // Cropping alone is rare, and we do not consider cropping to // still be valid after a resolution change nsIntRect newPictureRect(0, 0, si.w, si.h); nsIntSize frameSize(si.w, si.h); nsIntSize displaySize(si.w, si.h); if (IsValidVideoRegion(frameSize, newPictureRect, displaySize)) { mInfo.mVideo.mDisplay = displaySize; mInfo.mVideo.mImage = newPictureRect; } else { WEBM_DEBUG("IsValidVideoRegion returned false during resolution change"); return false; } mSharedVideoTrackInfo = new SharedTrackInfo(mInfo.mVideo, sStreamSourceID++); mLastSeenFrameWidth = Some(si.w); mLastSeenFrameHeight = Some(si.h); } }

Comment on attachment 8731436 [details] MozReview Request: Bug 1232045 - Add tests for WebM with resolution changes r=jya https://reviewboard.mozilla.org/r/40599/#review37181 ::: dom/media/test/test_resolution_change.html:23 (Diff revision 2) > + v.token = token; > + v.src = test.name; > + v.seenResolutionChange = false; > + > + function canplay(e) { > + var v = e.target; indentation. 6 spaces ? ::: dom/media/test/test_resolution_change.html:33 (Diff revision 2) > + var v = e.target; > + v.seenResolutionChange = true; > + } > + > + function ended(e) { > + var v = e.target; indentation inconsistent ::: dom/media/test/test_resolution_change.html:44 (Diff revision 2) > + v.addEventListener("canplay", canplay, false) > + v.addEventListener("ended", ended, false); > + > + manager.started(token); > + document.body.appendChild(v); > + v.play(); don't call play() here. I would call play() when you get loadeddata as then you're certain you don't have a race with playback going which may have fired the 2nd resize already I didn't pay attention earlier that your preload was metadata. So you're not guaranteed that you will receive "canplay" as preload=metadata indicates that you only need to retrieve the metadata and you're only guarantee is the readyState == HAVE_METADATA. However, because it's webm we need to retrieve the first frame to determine the starting time, so you will get at least readyState == HAVE_CURRENT_DATA but you will receive loadeddata however. and by then you're guaranteed that resize would have occurred.

Comment on attachment 8730145 [details] MozReview Request: Bug 1232045 - WebMDemuxer handles resolution changes r=jya Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39695/diff/4-5/

Comment on attachment 8731436 [details] MozReview Request: Bug 1232045 - Add tests for WebM with resolution changes r=jya Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40599/diff/2-3/

Comment on attachment 8730145 [details] MozReview Request: Bug 1232045 - WebMDemuxer handles resolution changes r=jya Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39695/diff/5-6/

Comment on attachment 8731436 [details] MozReview Request: Bug 1232045 - Add tests for WebM with resolution changes r=jya Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40599/diff/3-4/

Comment on attachment 8730145 [details] MozReview Request: Bug 1232045 - WebMDemuxer handles resolution changes r=jya https://reviewboard.mozilla.org/r/39695/#review37855 ::: dom/media/webm/WebMDemuxer.cpp:151 (Diff revision 6) > , mHasVideo(false) > , mHasAudio(false) > , mNeedReIndex(true) > , mLastWebMBlockOffset(-1) > , mIsMediaSource(aIsMediaSource) > + , mLastSeenFrameWidth(Nothing()) Unecessary, this os the default constructor ::: dom/media/webm/WebMDemuxer.cpp:585 (Diff revision 6) > + // Cropping alone is rare, and we do not consider cropping to > + // still be valid after a resolution change > + nsIntRect newPictureRect(0, 0, si.w, si.h); > + nsIntSize frameSize(si.w, si.h); > + nsIntSize displaySize(si.w, si.h); > + if (IsValidVideoRegion(frameSize, newPictureRect, displaySize)) { IMHO, this shouldnt be there really. it should be up to the decoder to handle whatever is valid or not. not in the demuxer

Comment on attachment 8731436 [details] MozReview Request: Bug 1232045 - Add tests for WebM with resolution changes r=jya https://reviewboard.mozilla.org/r/40599/#review37857

Comment on attachment 8731436 [details] MozReview Request: Bug 1232045 - Add tests for WebM with resolution changes r=jya https://reviewboard.mozilla.org/r/40599/#review37859

https://reviewboard.mozilla.org/r/39695/#review37855 > IMHO, this shouldnt be there really. it should be up to the decoder to handle whatever is valid or not. not in the demuxer Just remove this then? Do you think current checks done in the decoder are sufficient?

yes.(In reply to Bryce Van Dyk (:SingingTree) from comment #50) > Just remove this then? Do you think current checks done in the decoder are > sufficient? I believe so error handling of frames should be done in the decoder. when we attempt to create the image, we already check for overflow there: https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaData.cpp#302 I've dropped the issue, seeing that there's already check of frames in the existing demuxer. so I'm fine either way and we can take this in another bug.

Comment on attachment 8730145 [details] MozReview Request: Bug 1232045 - WebMDemuxer handles resolution changes r=jya Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39695/diff/6-7/

Comment on attachment 8731436 [details] MozReview Request: Bug 1232045 - Add tests for WebM with resolution changes r=jya Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40599/diff/4-5/

has problems to apply: patching file dom/media/webm/WebMDemuxer.cpp Hunk #1 succeeded at 7 with fuzz 1 (offset 0 lines). Hunk #3 FAILED at 562 1 out of 3 hunks FAILED -- saving rejects to file dom/media/webm/WebMDemuxer.cpp.rej patch failed to apply abort: fix up the working directory and run hg transplant --continue

Comment on attachment 8730145 [details] MozReview Request: Bug 1232045 - WebMDemuxer handles resolution changes r=jya Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39695/diff/7-8/

Comment on attachment 8731436 [details] MozReview Request: Bug 1232045 - Add tests for WebM with resolution changes r=jya Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40599/diff/5-6/

Rebased changes in review onto inbound to remedy merge issues. Please let me know if there are any further issues. Cheers.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a8fb7e6f6f9 https://hg.mozilla.org/integration/mozilla-inbound/rev/3d31cc25fd3b

https://hg.mozilla.org/mozilla-central/rev/7a8fb7e6f6f9 https://hg.mozilla.org/mozilla-central/rev/3d31cc25fd3b

Added to Firefox 48 for developers.