Closed Bug 1232043 Opened 9 years ago Closed 7 years ago

Handle resolution changes for VP8 in MediaRecorder

Categories

(Core :: Audio/Video: Recording, defect, P2)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: pehrsons, Assigned: achronop)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

Bug 1232043 - Update/create mochitests for recorder resolution change.
58 bytes, text/x-review-board-request
pehrsons
: review+
Details
Bug 1232043 - Reconfigure VP8 encoder when resolution change.
59 bytes, text/x-review-board-request
pehrsons
: review+
Details 
We'll need to handle resolution changes in MediaRecorder. I'm for now thinking of a basic case where we are just recording frames in their native resolution. We might want do some spec work to also allow for scaling the frames before they're encoded.

There are two things that must be done for this:

1. When we detect a resolution change, reconfigure the encoder.

2. When we are recording a track containing a video format that has to be converted to YUV420 before being fed to the encoder, we'll have to resize the conversion output buffer on resolution changes (at least when it becomes higher than its current capacity).
See Also: → 1232044
See Also: → 1232045
This is some stuff I wrote to be able to record the sample in bug 1232045. It is in no way complete.
Rank: 25
Priority: -- → P2
Assignee: nobody → achronop
Attachment #8739414 - Flags: review?(pehrsons)
Comment on attachment 8739414 [details]
Bug 1232043 - Reconfigure VP8 encoder when resolution change.

https://reviewboard.mozilla.org/r/45267/#review41951

::: dom/media/encoder/VP8TrackEncoder.h:75
(Diff revision 1)
> +  // Destroy the context and image wrapper. Does not de-allocate the structs.
> +  void DestroyVPXEncoder();
> +  // Initialize a new encoder. No synchronization applied.
> +  nsresult InitVPXEncoder(int32_t aWidth, int32_t aHeight, int32_t aDisplayWidth,
> +                          int32_t aDisplayHeight,TrackRate aTrackRate);
> +  // Helper method to set the values on a VPX configuration.
> +  bool SetConfigurationValues(int32_t aWidth, int32_t aHeight, int32_t aDisplayWidth,
> +                              int32_t aDisplayHeight,TrackRate aTrackRate, vpx_codec_enc_cfg_t& config);

Add a newline after each of these declarations.

::: dom/media/encoder/VP8TrackEncoder.h:80
(Diff revision 1)
> +  // Helper method to set the values on a VPX configuration.
> +  bool SetConfigurationValues(int32_t aWidth, int32_t aHeight, int32_t aDisplayWidth,
> +                              int32_t aDisplayHeight,TrackRate aTrackRate, vpx_codec_enc_cfg_t& config);

Return nsresult like the other methods.

Also, whitespace and linewidth.

::: dom/media/encoder/VP8TrackEncoder.cpp:50
(Diff revision 1)
> +VP8TrackEncoder::DestroyVPXEncoder()
> +{
>    if (mInitialized) {

You need to either assert that the monitor is held, or take it here. It's reentrant so should be ok to take.

::: dom/media/encoder/VP8TrackEncoder.cpp:62
(Diff revision 1)
> +nsresult
> +VP8TrackEncoder::InitVPXEncoder(int32_t aWidth, int32_t aHeight, int32_t aDisplayWidth,
> +                                int32_t aDisplayHeight,TrackRate aTrackRate)
> +{
> +  assert(!mInitialized);

Same here. Take the monitor.

::: dom/media/encoder/VP8TrackEncoder.cpp:69
(Diff revision 1)
> +                                int32_t aDisplayHeight,TrackRate aTrackRate)
> +{
> +  assert(!mInitialized);
> +  // Encoder configuration structure.
> +  vpx_codec_enc_cfg_t config;
> +  if (SetConfigurationValues(aWidth, aHeight, aDisplayWidth, aDisplayHeight, aTrackRate, config) == false) {

`== false` -> `!`

::: dom/media/encoder/VP8TrackEncoder.cpp:94
(Diff revision 1)
>  nsresult
>  VP8TrackEncoder::Init(int32_t aWidth, int32_t aHeight, int32_t aDisplayWidth,
>                        int32_t aDisplayHeight,TrackRate aTrackRate)
>  {

Should this `assert(!mInitialized)`?

::: dom/media/encoder/VP8TrackEncoder.cpp:106
(Diff revision 1)
>    }
>  
>    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
> +  nsresult rv = InitVPXEncoder(aWidth, aHeight, aDisplayWidth,
> +                               aDisplayHeight, aTrackRate);
> +  NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);

s/NS_ERROR_FAILURE/rv/

::: dom/media/encoder/VP8TrackEncoder.cpp:139
(Diff revision 1)
> +  if (SetConfigurationValues(aWidth, aHeight, aDisplayWidth,
> +                             aDisplayHeight, aTrackRate, config) == false) {

`if (aBool == false)` equals `if (!aBool)`.

::: dom/media/encoder/VP8TrackEncoder.cpp:141
(Diff revision 1)
> +  mInitialized = false;
> +  // Encoder configuration structure.
> +  vpx_codec_enc_cfg_t config;
> +  if (SetConfigurationValues(aWidth, aHeight, aDisplayWidth,
> +                             aDisplayHeight, aTrackRate, config) == false) {
> +    return NS_ERROR_FAILURE;

Add a log explaining that something (and what) went wrong here.

::: dom/media/encoder/VP8TrackEncoder.cpp:145
(Diff revision 1)
> +                             aDisplayHeight, aTrackRate, config) == false) {
> +    return NS_ERROR_FAILURE;
> +  }
>  
> +  if (vpx_codec_enc_config_set(mVPXContext.get(), &config) != VPX_CODEC_OK) {
> +    return NS_ERROR_FAILURE;

Same here. Log the error case.

::: dom/media/encoder/VP8TrackEncoder.cpp:221
(Diff revision 1)
> -                    VP8_ONE_TOKENPARTITION);
> -
> -  mInitialized = true;
> -  mon.NotifyAll();
> -
> +  gfx::IntSize intrinsicSize = aFrame.GetIntrinsicSize();
> +  gfx::IntSize imgSize = aFrame.GetImage()->GetSize();
> +  // Workaround: If the new size is less than or equal
> +  // to old, the existing encoder instance can continue.
> +  // Otherwise, re-create encoder.
> +  if (imgSize <= IntSize(mFrameWidth, mFrameHeight) &&

This checks that both width and height are smaller. Is that what we want to check or number of pixels?

Also, workaround for what?
IMO you can just drop "Workaround: ", and put the comment inside the if block to make it clear what it applies to.

::: dom/media/encoder/VP8TrackEncoder.cpp:222
(Diff revision 1)
> +      ReConfigure(imgSize.width, imgSize.height, intrinsicSize.width,
> +                  intrinsicSize.height, mTrackRate) == NS_OK) {

Indentation.

This seems like the only use of ReConfigure. Inline?

You have to handle the nsresult that it returns.

::: dom/media/encoder/VP8TrackEncoder.cpp:229
(Diff revision 1)
> +  return ReInit(imgSize.width, imgSize.height,
> +                intrinsicSize.width, intrinsicSize.height, mTrackRate);

This seems like the only use of ReInit. Inline?

::: dom/media/encoder/VP8TrackEncoder.cpp:350
(Diff revision 1)
>      VP8LOG("Dynamic resolution changes (was %dx%d, now %dx%d) are unsupported\n",
>             mFrameWidth, mFrameHeight, img->GetSize().width, img->GetSize().height);

Change this to log how the dimensions are changing.

::: dom/media/encoder/VP8TrackEncoder.cpp:353
(Diff revision 1)
>  
>    if (img->GetSize() != IntSize(mFrameWidth, mFrameHeight)) {
>      VP8LOG("Dynamic resolution changes (was %dx%d, now %dx%d) are unsupported\n",
>             mFrameWidth, mFrameHeight, img->GetSize().width, img->GetSize().height);
> -    return NS_ERROR_FAILURE;
> +    nsresult rv = ConfigureNewFrameSize(aChunk.mFrame);
> +    NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);

s/NS_ERROR_FAILURE/rv/
Comment on attachment 8739415 [details]
Bug 1232043 - Update/create mochitests for recorder resolution change.

https://reviewboard.mozilla.org/r/45269/#review41989

Looks pretty good. Some minor things to fix.

I think you should also add another test that uses screen capture (gUM) and `applyConstraints` for resizing so we know that both paths work.

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:36
(Diff revision 1)
> -
> -  mediaRecorder = new MediaRecorder(stream);
> +  var numResizeRaised = 0;
> +  // Recorded data that will be playback.
> +  var blob;
> +
> +  // Media recorder for VP8 and canvas stream.
> +  var config = { mimeType: 'video/webm;codecs="vp8,opus"' };

Is the mimeType needed here? If you omit this it should automatically pick a default that works fine.

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:43
(Diff revision 1)
>    is(mediaRecorder.stream, stream,
>       "Media recorder stream = canvas stream at the start of recording");
>  
> +  // Not expected events.
>    mediaRecorder.onwarning = () => ok(false, "onwarning unexpectedly fired");
> +  mediaRecorder.onerror = err => ok(false, "onerror unexpectedly fired");

Do we get "stop" in case of "error"? Otherwise we'll need a `SimpleTest.finish()` here too.

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:54
(Diff revision 1)
>    mediaRecorder.onstart = () => {
> -    canvas.width = canvas.height = canvas.width * 1.1;
> -    ctx.fillStyle = "blue";
> +    info('onstart fired successfully');
> +    // Resize and recapture frame. This will resize the stream
> +    // and will reproduce the problem.
> +    canvas.width = canvas.height = changed_canvas_size;
> +    ctx.fillStyle = "red";
>      ctx.fillRect(0, 0, canvas.width, canvas.height);
>      stream.requestFrame();
> +    // Stop recorder. Set timeout due to a bug inside recorder.
> +    setTimeout(() => mediaRecorder.stop(), 100);

Do you have a bug number for this bug?

If you in `onstart` change the canvas size, `requestFrame()` and `stop()` the mediaRecorder, you're saying:
1. Capture a frame on the next render of the canvas.
2. Stop recording now.

I.e., you're stopping recording of the stream before the new frame has come in.


You could just use `setTimeout` like you are now (I hope it doesn't go intermittent!). I think you'd be safer off setting an interval for the recorder though (i.e., time between issuing blobs). Then you can resize after the first blob, and stop on the second one. You can merge them by doing `new Blob([blobs])`.

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:76
(Diff revision 1)
> +    video.onerror = err => {
> +      ok(false, "Should be able to play the recording. Got error. code=" + video.error.code);
> +    };

Also add a `SimpleTest.finish()` here.

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:87
(Diff revision 1)
> +      ++numResizeRaised;
> +      if (numResizeRaised == 1) {
> +        is(this.videoWidth, canvas_size, "On 1st resize event width should be at the original value");
> +        is(this.videoHeight, canvas_size, "On 1st resize event height should be at the original value");
> +      } else if (numResizeRaised == 2) {
> +        is(this.videoWidth, changed_canvas_size, "On 2nd resize event width should be at the original value");

s/original/changed/

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:99
(Diff revision 1)
> +    video.onended = () => {
> +      is(numResizeRaised, 2, "Expected 2 resize event");
> -    SimpleTest.finish();
> +      SimpleTest.finish();
> +    }
> +    document.getElementById("content").appendChild(video);
> +    // Start palyback.

I think we can do without this comment.

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:107
(Diff revision 1)
>  
> +  // Start here by stream recorder.
>    mediaRecorder.start();
>    is(mediaRecorder.state, "recording", "Media recorder should be recording");
> +
> +  SimpleTest.requestFlakyTimeout("This is required due to Bug 1262851");

I'm not convinced you need the setTimeout due to that bug.

However, to work around that bug you can capture 3 frames and the duration is correct.

This also makes sense to do, so we can test resizing both up and down. Perhaps some more are desired, like changing aspect ratio, and trying some extremes, like height or width of 0 or 1 and what happens on playback.

Is width or height of 0 handled in the spec? How _should_ it be handled?
Attachment #8739415 - Flags: review?(pehrsons)
Another (hacky?) alternative to configuring the resolution to be recorded is to render to and capture from a canvas. This is the most flexible option (as one can do other things in a canvas) although perhaps not the simplest/most convenient and I don't know if it is the most performant either.
That's true capture from canvas is very easy and straight forward. Actually that way is implemented in the MozTest attached on this bug. I cannot speak about performance but the only performance penalty I see is capturing from canvas. For the media recorder point of view should make no different.
I pushed the review comments as we discussed them. Also I have created the 2 extra mochitests. Please note that I found handy to modify the CaptureStreamTestHelper to be able to resize it's internal canvas. Please let me know what you think.
Please test if players can play back the recorded streams with resolution changes... last time I tried doing this, almost all players either froze or crashed on a resolution change; some glitched but played it.  We discussed enforcing no resolution changes (scaling and cropping/letterboxing as needed (on rotations) unless the application said it wanted them encoded (which would be a spec change).  (or a flag to disable changes, though that would cause lots of existing examples to break, I'd think).

Andreas - thoughts on the usability of resolution changes at the moment?
Flags: needinfo?(pehrson)
Flags: needinfo?(achronop)
I have tested that the playback works after resolution change. Also mochitest verify if we get the correct frames. BTW I solved a crash issue after resolution change but it wasn't in playback. Just note that it is supported by Chrome.
Flags: needinfo?(achronop)
Comment on attachment 8739414 [details]
Bug 1232043 - Reconfigure VP8 encoder when resolution change.

https://reviewboard.mozilla.org/r/45267/#review100208

I got a crash from this when testing.

STR:
Go to https://mozilla.github.io/webrtc-landing/gum_test.html
Request audio+video
On the video track, apply constraints per below (several seconds in between each call):
v.applyConstraints({width: {exact: 160}})
v.applyConstraints({width: {exact: 640}})
v.applyConstraints({width: {exact: 960}})

Stack:
Assertion failure: false, at /Users/pehrsons/Comoyo/mozilla-central/dom/media/encoder/VP8TrackEncoder.cpp:99
#01: mozilla::VP8TrackEncoder::Init(int, int, int, int)[/Users/pehrsons/Comoyo/mozilla-central/obj-x86_64-apple-darwin16.1.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3dd5c70]
#02: mozilla::VideoTrackEncoder::Init(mozilla::VideoSegment const&)[/Users/pehrsons/Comoyo/mozilla-central/obj-x86_64-apple-darwin16.1.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3dd519d]
#03: mozilla::VideoTrackEncoder::SetCurrentFrames(mozilla::VideoSegment const&)[/Users/pehrsons/Comoyo/mozilla-central/obj-x86_64-apple-darwin16.1.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3dcfda4]
#04: mozilla::MediaStreamVideoRecorderSink::SetCurrentFrames(mozilla::VideoSegment const&)[/Users/pehrsons/Comoyo/mozilla-central/obj-x86_64-apple-darwin16.1.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3dcfcda]
Attachment #8739414 - Flags: review?(pehrson)
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #14)
> Comment on attachment 8739414 [details]
> Bug 1232043 - Reconfigure VP8 encoder when resolution change.
> 
> https://reviewboard.mozilla.org/r/45267/#review100208
> 
> I got a crash from this when testing.
> 
> STR:
> Go to https://mozilla.github.io/webrtc-landing/gum_test.html
> Request audio+video
> On the video track, apply constraints per below (several seconds in between
> each call):
> v.applyConstraints({width: {exact: 160}})
> v.applyConstraints({width: {exact: 640}})
> v.applyConstraints({width: {exact: 960}})

Actually I requested a new audio+video stream from the console, so you could basically do this from about:blank.
(In reply to Randell Jesup [:jesup] from comment #12)
> Andreas - thoughts on the usability of resolution changes at the moment?

As Alex mentioned, both us and Chrome support playback of them now.

And I think any application that is serious about supporting many platforms will transcode anyway due to fragmentation.

What are the arguments against doing this? Native player support?
Flags: needinfo?(pehrson)
Comment on attachment 8739415 [details]
Bug 1232043 - Update/create mochitests for recorder resolution change.

Clearing this for now.
Attachment #8739415 - Flags: review?(pehrson)
Comment on attachment 8739415 [details]
Bug 1232043 - Update/create mochitests for recorder resolution change.

https://reviewboard.mozilla.org/r/45269/#review121990

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:27
(Diff revisions 1 - 3)
>    var ctx = canvas.getContext("2d");
>    ctx.fillStyle = "red";
>    ctx.fillRect(0, 0, canvas.width, canvas.height);
>  
>    // The recorded stream coming from canvas.
>    var stream = canvas.captureStream(0);

You could make this `captureStream()` and get rid of the `requestFrame()`s.

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:36
(Diff revisions 1 - 3)
>    var numResizeRaised = 0;
>    // Recorded data that will be playback.
>    var blob;
>  
>    // Media recorder for VP8 and canvas stream.
> -  var config = { mimeType: 'video/webm;codecs="vp8,opus"' };
> +  var config = { mimeType: 'video/webm;codecs="vp8"' };

Just drop this line. Same with the other two tests.

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:79
(Diff revisions 1 - 3)
>  
>      // Check here that resize is correct in the playback stream.
>      video.onresize = function() {
>        ++numResizeRaised;
> -      if (numResizeRaised == 1) {
> -        is(this.videoWidth, canvas_size, "On 1st resize event width should be at the original value");
> +      //We might have less resize event than the resize occured.
> +      var expected_results = [canvas_size, changed_canvas_size, 1];

This is a good idea. Could you make it a deterministic array?

I.e., put it at top scope and use it for both drawing and checking the result, ala `is(this.wideoWidth, size[numResizeRaised-1])`.

Same idea for the other two tests as well.

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:103
(Diff revisions 1 - 3)
>    // Start here by stream recorder.
>    mediaRecorder.start();
>    is(mediaRecorder.state, "recording", "Media recorder should be recording");
> +  requestAnimationFrame(draw);
>  
> -  SimpleTest.requestFlakyTimeout("This is required due to Bug 1262851");
> +  // Change resolution every 5 frames

Why not change on every 1 frame?

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:124
(Diff revisions 1 - 3)
> +    if (countFrames == 20) {
> +      mediaRecorder.stop();
> +    }

I think it would make sense to exit early in this if. It could probably also be made into an `else`.

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:130
(Diff revisions 1 - 3)
> +  function resizeAndDrawCanvas(size, color) {
> +    canvas.width = canvas.height = size;
> +    ctx.fillStyle = color;
>      ctx.fillRect(0, 0, canvas.width, canvas.height);
>      stream.requestFrame();
> -  }, 100);
> -}
> +  }

I think code could be simpler (one less indirection) if you inlined the size and color parts of this, and moved the drawing part to outside of the big if statement in `draw()`.

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:136
(Diff revisions 1 - 3)
> +    ctx.fillStyle = color;
>      ctx.fillRect(0, 0, canvas.width, canvas.height);
>      stream.requestFrame();
> -  }, 100);
> -}
> +  }
>  

Remove this newline.

::: dom/canvas/test/captureStream_common.js:188
(Diff revision 3)
> +
> +  /* Resize output canvas */
> +  resize: function(width, height) {
> +    this.cout.width = width;
> +    this.cout.height = height;
> +  },

This shouldn't be needed. `cout` is only used for getting the color information from a pixel in a video element.

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:107
(Diff revision 3)
> +    if (countFrames < 20){
> +      // Register draw to be called on next rendering
> +      requestAnimationFrame(draw);
> +    }

Remove this if and move the rAF to the end of the function.

::: dom/media/test/test_mediarecorder_record_downsize_resolution.html:133
(Diff revision 3)
> +    countFrames++;
> +  }
> +
> +  function resizeAndDrawCanvas(size, color) {
> +    canvas.width = canvas.height = size;
> +    helper.resize(size, size);

Not needed.

::: dom/media/test/test_mediarecorder_record_upsize_resolution.html:133
(Diff revision 3)
> +    countFrames++;
> +  }
> +
> +  function resizeAndDrawCanvas(size, color) {
> +    canvas.width = canvas.height = size;
> +    helper.resize(size, size);

Not needed.
Attachment #8739415 - Flags: review?(pehrson)
Comment on attachment 8739414 [details]
Bug 1232043 - Reconfigure VP8 encoder when resolution change.

https://reviewboard.mozilla.org/r/45267/#review122024

::: dom/media/encoder/VP8TrackEncoder.h:67
(Diff revision 3)
> +  // Helper method to set the values on a VPX configuration.
> +  nsresult SetConfigurationValues(int32_t aWidth, int32_t aHeight, int32_t aDisplayWidth,
> +                                  int32_t aDisplayHeight, vpx_codec_enc_cfg_t& config);
> +
> +  // Configure encoder when new frame size (resolution).
> +  nsresult ConfigureNewFrameSize(const VideoFrame& aFrame);

ConfigureNewFrameSize only has one user. I think it makes sense to inline it at the callsite.

::: dom/media/encoder/VP8TrackEncoder.cpp:98
(Diff revision 3)
> +
> +
> +

Two of these should go.

::: dom/media/encoder/VP8TrackEncoder.cpp:156
(Diff revision 3)
> +nsresult
> +VP8TrackEncoder::SetConfigurationValues(int32_t aWidth, int32_t aHeight, int32_t aDisplayWidth,
> +                                        int32_t aDisplayHeight, vpx_codec_enc_cfg_t& config)
> +{
> -  mFrameWidth = aWidth;
> +   mFrameWidth = aWidth;
> -  mFrameHeight = aHeight;
> +   mFrameHeight = aHeight;
> -  mDisplayWidth = aDisplayWidth;
> +   mDisplayWidth = aDisplayWidth;
> -  mDisplayHeight = aDisplayHeight;
> +   mDisplayHeight = aDisplayHeight;

All lines in this method are indented wrong (3 spaces, should be 2).

::: dom/media/encoder/VP8TrackEncoder.cpp:199
(Diff revision 3)
> -    config.g_threads = 1; // 1 thread for VGA or less
> +     config.g_threads = 1; // 1 thread for VGA or less
> -  }
> +   }
>  
> -  // rate control settings
> +   // rate control settings
> -  config.rc_dropframe_thresh = 0;
> +   config.rc_dropframe_thresh = 0;
> -  config.rc_end_usage = VPX_VBR;
> +   config.rc_end_usage = VPX_CBR;

Failed rebase.

::: dom/media/encoder/VP8TrackEncoder.cpp:201
(Diff revision 3)
> -  // ffmpeg doesn't currently support streams that use resize.
> +   // ffmpeg doesn't currently support streams that use resize.
> -  // Therefore, for safety, we should turn it off until it does.
> +   // Therefore, for safety, we should turn it off until it does.
> -  config.rc_resize_allowed = 0;
> +   config.rc_resize_allowed = 0;

Do we have to care about this?

::: dom/media/encoder/VP8TrackEncoder.cpp:218
(Diff revision 3)
> -  vpx_codec_control(mVPXContext, VP8E_SET_CPUUSED, -6);
> -  vpx_codec_control(mVPXContext, VP8E_SET_TOKEN_PARTITIONS,
> -                    VP8_ONE_TOKENPARTITION);
> +VP8TrackEncoder::ConfigureNewFrameSize(const VideoFrame& aFrame)
> +{
> +  gfx::IntSize intrinsicSize = aFrame.GetIntrinsicSize();
> +  gfx::IntSize imgSize = aFrame.GetImage()->GetSize();
> +  if (imgSize <= IntSize(mFrameWidth, mFrameHeight)) { // check buffer size instead
> +    // If the new size is less than or equal to old,
> +    // the existing encoder instance can continue.
> +    if (NS_SUCCEEDED(Reconfigure(imgSize.width, imgSize.height, intrinsicSize.width,
> +                                 intrinsicSize.height))) {
> +      VP8LOG(LogLevel::Verbose, "ReConfigure VP8 encoder.");
> +      return NS_OK;
> +    }
> +  }
>  
> -  mInitialized = true;
> -  mon.NotifyAll();
> +  // New frame size is larger; re-create the encoder.
> +  Destroy();
>  
> -  return NS_OK;
> +  return Init(imgSize.width, imgSize.height,
> +              intrinsicSize.width, intrinsicSize.height);
>  }

Should this grab the monitor to avoid races?

::: dom/media/encoder/VP8TrackEncoder.cpp:376
(Diff revision 3)
>      img = mMuteFrame;
>    } else {
>      img = aChunk.mFrame.GetImage();
>    }
>  
>    if (img->GetSize() != IntSize(mFrameWidth, mFrameHeight)) {

`ConfigureNewFrameSize()` also uses the intrinsic size. Should we check that here as well?
Attachment #8739414 - Flags: review?(pehrson)
Comment on attachment 8739415 [details]
Bug 1232043 - Update/create mochitests for recorder resolution change.

https://reviewboard.mozilla.org/r/45269/#review43655

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:36
(Diff revision 1)
> -
> -  mediaRecorder = new MediaRecorder(stream);
> +  var numResizeRaised = 0;
> +  // Recorded data that will be playback.
> +  var blob;
> +
> +  // Media recorder for VP8 and canvas stream.
> +  var config = { mimeType: 'video/webm;codecs="vp8,opus"' };

I want to use VP8 in particular. Right now it happens VP8 be the default but this might change in the future and also it makes it more obvious. I could remove the audio codec though.

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:43
(Diff revision 1)
>    is(mediaRecorder.stream, stream,
>       "Media recorder stream = canvas stream at the start of recording");
>  
> +  // Not expected events.
>    mediaRecorder.onwarning = () => ok(false, "onwarning unexpectedly fired");
> +  mediaRecorder.onerror = err => ok(false, "onerror unexpectedly fired");

According to the spec we should: "If recording has been started and not yet stopped when the error occurs, then after raising the error, the UA must fire a dataavailable event, containing any data that it has gathered, and then a stop event."

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:103
(Diff revisions 1 - 3)
>    // Start here by stream recorder.
>    mediaRecorder.start();
>    is(mediaRecorder.state, "recording", "Media recorder should be recording");
> +  requestAnimationFrame(draw);
>  
> -  SimpleTest.requestFlakyTimeout("This is required due to Bug 1262851");
> +  // Change resolution every 5 frames

No reason. Given that resize events are skipped under overload, it might be better to keep it like that.

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:130
(Diff revisions 1 - 3)
> +  function resizeAndDrawCanvas(size, color) {
> +    canvas.width = canvas.height = size;
> +    ctx.fillStyle = color;
>      ctx.fillRect(0, 0, canvas.width, canvas.height);
>      stream.requestFrame();
> -  }, 100);
> -}
> +  }

I drop the comment because I do not understand exactly what you want me to do. I know that will be a second round so I could fix it there. Can you please provide more details on this?
Comment on attachment 8739415 [details]
Bug 1232043 - Update/create mochitests for recorder resolution change.

https://reviewboard.mozilla.org/r/45269/#review41989

> Do you have a bug number for this bug?
> 
> If you in `onstart` change the canvas size, `requestFrame()` and `stop()` the mediaRecorder, you're saying:
> 1. Capture a frame on the next render of the canvas.
> 2. Stop recording now.
> 
> I.e., you're stopping recording of the stream before the new frame has come in.
> 
> 
> You could just use `setTimeout` like you are now (I hope it doesn't go intermittent!). I think you'd be safer off setting an interval for the recorder though (i.e., time between issuing blobs). Then you can resize after the first blob, and stop on the second one. You can merge them by doing `new Blob([blobs])`.

I removed setTimeout and I am using the requestAnimationFrame() as you pruposed.

> I'm not convinced you need the setTimeout due to that bug.
> 
> However, to work around that bug you can capture 3 frames and the duration is correct.
> 
> This also makes sense to do, so we can test resizing both up and down. Perhaps some more are desired, like changing aspect ratio, and trying some extremes, like height or width of 0 or 1 and what happens on playback.
> 
> Is width or height of 0 handled in the spec? How _should_ it be handled?

I removed setTimeout from the code.

I added up and down resize. 

About extreme ratio. Height and width of 1 are accepted as normal and they work like recording any other ratio.
Height or width is 0 the context return an 1x1 cleared image and we are able to record that as normaly. I will add a case to test it.
Comment on attachment 8739415 [details]
Bug 1232043 - Update/create mochitests for recorder resolution change.

https://reviewboard.mozilla.org/r/45269/#review41989

> Is the mimeType needed here? If you omit this it should automatically pick a default that works fine.

IMHO this should be added when we add support for another video codec then. Preferrably that would make the test loop over all codecs.

> I removed setTimeout from the code.
> 
> I added up and down resize. 
> 
> About extreme ratio. Height and width of 1 are accepted as normal and they work like recording any other ratio.
> Height or width is 0 the context return an 1x1 cleared image and we are able to record that as normaly. I will add a case to test it.

Right, but should we test those extremes here?
Comment on attachment 8739415 [details]
Bug 1232043 - Update/create mochitests for recorder resolution change.

https://reviewboard.mozilla.org/r/45269/#review43655

> No reason. Given that resize events are skipped under overload, it might be better to keep it like that.

Then I think it's better to put a guard at the top that allows some time (100ms?) to pass between `draw()`s (not size changes) -- the callback passed to `requestAnimationFrame()` gets passed a timestamp as first argument.

> I drop the comment because I do not understand exactly what you want me to do. I know that will be a second round so I could fix it there. Can you please provide more details on this?

Remove `resizeAndDrawCanvas`.
Comment on attachment 8847732 [details]
Bug 1232043 - Reconfigure VP8 encoder when resolution change.

https://reviewboard.mozilla.org/r/120656/#review125276

::: dom/canvas/test/captureStream_common.js:189
(Diff revision 1)
>    /* Resize output canvas */
>    resize: function(width, height) {
> -    this.cout.width = width;
> -    this.cout.height = height;
> +    this.width = width;
> +    this.height = height;
>    },

Is this still used?

::: dom/media/encoder/VP8TrackEncoder.cpp:353
(Diff revision 1)
>      VP8LOG(LogLevel::Error,
>             "Dynamic resolution changes (was %dx%d, now %dx%d).",
>             mFrameWidth, mFrameHeight, img->GetSize().width, img->GetSize().height);

Nits:
s/Error/Info/, s/changes/change/

::: dom/media/encoder/VP8TrackEncoder.cpp:367
(Diff revision 1)
> +        // the existing encoder instance can continue.
> +        NS_SUCCEEDED(Reconfigure(imgSize.width,
> +                                 imgSize.height,
> +                                 intrinsicSize.width,
> +                                 intrinsicSize.height))) {
> +      VP8LOG(LogLevel::Verbose, "ReConfigure VP8 encoder.");

s/Verbose/Info/ s/ReConfigure/Reconfigured/

Also add a log to the other leg of this if.

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:26
(Diff revision 1)
> +  var expected_results = [canvas_size, changed_canvas_size, 1];
> +
> +

Nit: Make it const, and remove one of the newlines.

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:83
(Diff revision 1)
>          ok(expected_results.some((el) => this.videoWidth == el), "Resize width should at expected value " + this.videoWidth);
>          ok(expected_results.some((el) => this.videoHeight == el), "Resize height at expected value " + this.videoWidth);

Can we make it so these are like this instead?

```
is(this.videoWidth, expectedResults[numResizeRaised].width, "...");
is(this.videoHeight, expectedResults[numResizeRaised].height, "...");
```

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:121
(Diff revision 1)
> -      resizeAndDrawCanvas(canvas_size, "blue");
> +      resizeAndDrawCanvas(expected_results[0], "blue");
>      }  else if (countFrames < 20) {
>        // zero size
>        resizeAndDrawCanvas(0, "green");
>      }
>      if (countFrames == 20) {

Make this an else.
Attachment #8847732 - Flags: review?(pehrson)
Comment on attachment 8847732 [details]
Bug 1232043 - Reconfigure VP8 encoder when resolution change.

https://reviewboard.mozilla.org/r/120656/#review125276

> Is this still used?

No, I'll remove the change.

> Nits:
> s/Error/Info/, s/changes/change/

Done

> Nit: Make it const, and remove one of the newlines.

Done

> Can we make it so these are like this instead?
> 
> ```
> is(this.videoWidth, expectedResults[numResizeRaised].width, "...");
> is(this.videoHeight, expectedResults[numResizeRaised].height, "...");
> ```

No because we do not know in advance which event will be skipped. If for examplet the 2nd event is skipped on next event the numResizeRaised will reamain equal to 1 but we want to compare with 3rd element of the expected_results array (index 2).

> Make this an else.

Done
Attachment #8739414 - Attachment is obsolete: true
Attachment #8739414 - Flags: review?(pehrson)
Comment on attachment 8739415 [details]
Bug 1232043 - Update/create mochitests for recorder resolution change.

https://reviewboard.mozilla.org/r/45269/#review128036

This is looking pretty good now! I think this could be the last round of changes.

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:26
(Diff revisions 4 - 5)
>  
>    var ctx = canvas.getContext("2d");
>    ctx.fillStyle = "red";
>    ctx.fillRect(0, 0, canvas.width, canvas.height);
>  
> +  const expected_results = [canvas_size, changed_canvas_size, 1];

I think this should be an array of {width,height} tuples like so:

```
[ {width: 100, height: 100},
  {width: 150, height: 150}
]
```

Then you can drop `canvas_size` and `changed_canvas_size` and this is the sole source of truth.

With that done we can easily add more tests, such as odd corner cases, by just adding items to this array.

Also, what's the `1` at the end of the array for?

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:80
(Diff revisions 4 - 5)
>        if (numResizeRaised <= 4) {
>          is(this.videoWidth, this.videoHeight, "Expected video width = height");
>          ok(expected_results.some((el) => this.videoWidth == el), "Resize width should at expected value " + this.videoWidth);
>          ok(expected_results.some((el) => this.videoHeight == el), "Resize height at expected value " + this.videoWidth);
>        } else {
>          ok(false, "Got more resize events than expected");
>        }
>      }

As discussed before, this should check actual sizes against the authoritative array above.

There might be sync issues between the resize event and what size is currently displayed in the media element. We can probably work around that with the help of `seekToNextFrame`. Then you can also avoid the resize event altogether and use the Promise provided by `seekToNextFrame` instead.

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:117
(Diff revisions 4 - 5)
> +
> +    var size = 0;
> +    var color = "";
> +    if (countFrames < 1) {
>        // Initial size
> -      resizeAndDrawCanvas(canvas_size, "blue");
> +      size = expected_results[0];

These should be `expected_results[countFrames]` until the `countFrames` array has been iterated through.

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:118
(Diff revisions 4 - 5)
> +    var size = 0;
> +    var color = "";
> +    if (countFrames < 1) {
>        // Initial size
> -      resizeAndDrawCanvas(canvas_size, "blue");
> -    } else if (countFrames < 10) {
> +      size = expected_results[0];
> +      color = "blue";

Colors could be added to the objects in the array of sizes too, for fewer points of dependency.

::: dom/media/test/test_mediarecorder_record_downsize_resolution.html:34
(Diff revisions 4 - 5)
>    var numResizeRaised = 0;
>    // Recorded data that will be playback.
>    var blob;
>  
>    // Media recorder for VP8 and canvas stream.
> -  var config = { mimeType: 'video/webm;codecs="vp8"' };
> +  mediaRecorder = new MediaRecorder(stream);

Is `mediaRecorder` declared anywhere?
Attachment #8739415 - Flags: review?(pehrson)
Comment on attachment 8847732 [details]
Bug 1232043 - Reconfigure VP8 encoder when resolution change.

https://reviewboard.mozilla.org/r/120656/#review128042

Looks good, but that rc_resize_allowed parameter needs to be looked into.

::: dom/media/encoder/VP8TrackEncoder.cpp:131
(Diff revision 3)
> +
> +nsresult
> +VP8TrackEncoder::Reconfigure(int32_t aWidth, int32_t aHeight,
> +                             int32_t aDisplayWidth, int32_t aDisplayHeight)
> +{
> +  MOZ_ASSERT(mInitialized);

This should be a guard also in opt. E.g.,
```
if (!mInitialized) {
  MOZ_ASSERT(false);
  return NS_ERROR_FAILURE;
}
```

::: dom/media/encoder/VP8TrackEncoder.cpp:132
(Diff revision 3)
> +  MOZ_ASSERT(aWidth > 0 && aHeight > 0 && aDisplayWidth > 0 &&
> +             aDisplayHeight > 0);

Same here.

::: dom/media/encoder/VP8TrackEncoder.cpp:199
(Diff revision 3)
>    // ffmpeg doesn't currently support streams that use resize.
>    // Therefore, for safety, we should turn it off until it does.
>    config.rc_resize_allowed = 0;

As mentioned before, this parameter should be investigated.
Attachment #8847732 - Flags: review?(pehrson)
Comment on attachment 8739415 [details]
Bug 1232043 - Update/create mochitests for recorder resolution change.

https://reviewboard.mozilla.org/r/45269/#review121990

> This is a good idea. Could you make it a deterministic array?
> 
> I.e., put it at top scope and use it for both drawing and checking the result, ala `is(this.wideoWidth, size[numResizeRaised-1])`.
> 
> Same idea for the other two tests as well.

I.e., put it at top scope and use it for both drawing and checking the result

Will do.
 
ala `is(this.wideoWidth, size[numResizeRaised-1])`.

I am not sure what you mean. Sometimes I get less resize event than expected. This seems to be normal during overload of the system. I do it like that in order to check if the resize events contain any of the expected values

Same idea for the other two tests as well.

The other 2 tests check one resize event thus there is no need for something like this.

> Why not change on every 1 frame?

Discussion on this happens in a second comment below.
Comment on attachment 8739415 [details]
Bug 1232043 - Update/create mochitests for recorder resolution change.

https://reviewboard.mozilla.org/r/45269/#review129332

Looks good! Biggest thing is to make sure to wait for the pref to be set before starting.
As a followup we could also check the color of each frame in the recorded video.
Also make sure to file that bug on it being possible to miss resize events.

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:84
(Diff revisions 5 - 6)
> -      //We might have less resize event than the resize occured.
> -      if (numResizeRaised <= 4) {
> +        is(video.videoWidth, resolution_change[numResizeRaised].width, "onresize width should be as expected");
> +        is(video.videoHeight, resolution_change[numResizeRaised].height, "onresize height should be as expected");

nit: looks like more than 80 columns

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:93
(Diff revisions 5 - 6)
>        }
> -    }
> +      ++numResizeRaised;
> +    };
>  
>      video.onended = () => {
> -      ok(numResizeRaised <= 4, "Expected up to 4 resize event got " + numResizeRaised);
> +      video.finished = true;

I can't see this used anywhere.

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:101
(Diff revisions 5 - 6)
> +    video.onloadedmetadata = () => {
> +      callSeekToNextFrame();
> +    };
> +

This shouldn't be needed if you call `seekToNextFrame` from the 'resize' callback.

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:108
(Diff revisions 5 - 6)
> +          if (seekFrames < resolution_change.length) {
> +            ++seekFrames;
> +            callSeekToNextFrame();
> +          }

I think it makes sense to put the call to `seekToNextFrame()` in the resize callback so you don't seek *before* you've checked the previous frame.

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:124
(Diff revisions 5 - 6)
> -  // Change resolution every 5 frames
> +  // Change resolution in every frames
>    // Stop recoredr on last frame

nits:
s/every frames/every frame/
s/recoredr/recorder/

::: dom/media/test/test_mediarecorder_record_changing_video_resolution.html:151
(Diff revisions 5 - 6)
> +SpecialPowers.pushPrefEnv(
> +  {
> +    "set": [["media.seekToNextFrame.enabled", true ]],
> +    "set": [["media.video-queue.send-to-compositor-size", 1]]
> +  });

If I'm not mistaken `pushPrefEnv` takes a callback as a second argument. Pass `startTest` there.
Attachment #8739415 - Flags: review?(pehrson) → review+
Comment on attachment 8847732 [details]
Bug 1232043 - Reconfigure VP8 encoder when resolution change.

https://reviewboard.mozilla.org/r/120656/#review129344

I want `rc_resize_allowed` looked into before giving r+.

::: dom/media/encoder/VP8TrackEncoder.cpp:131
(Diff revisions 3 - 4)
>  
>  nsresult
>  VP8TrackEncoder::Reconfigure(int32_t aWidth, int32_t aHeight,
>                               int32_t aDisplayWidth, int32_t aDisplayHeight)
>  {
> -  MOZ_ASSERT(mInitialized);
> +  if(aWidth <= 0 && aHeight <= 0 && aDisplayWidth <= 0 &&aDisplayHeight <= 0) {

s/&&/||/g
Attachment #8847732 - Flags: review?(pehrson)
Comment on attachment 8847732 [details]
Bug 1232043 - Reconfigure VP8 encoder when resolution change.

https://reviewboard.mozilla.org/r/120656/#review128042

> As mentioned before, this parameter should be investigated.

This is an option for the decoder and it has nothing to do with resolution change we implement here[0]. Also chromium does not have it. 

The flag signals the encoder to compress lower resolution of the frames and upscale in decoder, in order to use lower data rates at the expense of CPU. Whether we want, something like that, or not is not on the scope of this bug. 

[0] https://www.webmproject.org/docs/webm-sdk/structvpx__codec__enc__cfg.html#a02a4e2f18fb0fdfff44df8b0d9a99d6c
Comment on attachment 8847732 [details]
Bug 1232043 - Reconfigure VP8 encoder when resolution change.

https://reviewboard.mozilla.org/r/120656/#review129366
Attachment #8847732 - Flags: review+
Attachment #8697698 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Depends on: 1354506
https://hg.mozilla.org/mozilla-central/rev/ced1178a1e12
https://hg.mozilla.org/mozilla-central/rev/6c4ba0b593aa
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1354841
Depends on: 1356113
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: