In vie_encoder.cc, it only passes the content_analysis structure to AddFrame if the codec is VP8. That's silly (no idea why upstream did it that way). The fix is obvious.

Experimentation has shown the current OpenH264 codec doesn't change SPS/PPS when input resolution changes; it insets. Since we can't easily reconfigure a codec, shut down and re-init the codec on a resolution change, which is expensive. We should rev the API in the future (35) to a) let the codec tell us if it needs reiniting for resolution changes and/or b) provide an API to tell it to reconfigure dynamically (or at least tell it a new resolution).

Comment on attachment 8481093 [details] [diff] [review] handle incoming resolution changes in GMP video encode Normally I'd use cpearce for review, but he's on PTO I believe.

[Blocking Requested - why for this release]: See bug 972639 - we realized we're not adjusting framerates and resolutions depending on load and bitrate available

https://hg.mozilla.org/integration/mozilla-inbound/rev/c52e50a27c8c https://hg.mozilla.org/integration/mozilla-inbound/rev/7b7cb046d45f

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2b60f30db362 for something in that push causing b2g build bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=47078117&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8055a6d7d10 https://hg.mozilla.org/integration/mozilla-inbound/rev/ad4f2dc3be44

https://hg.mozilla.org/mozilla-central/rev/f8055a6d7d10 https://hg.mozilla.org/mozilla-central/rev/ad4f2dc3be44

Comment on attachment 8481093 [details] [diff] [review] handle incoming resolution changes in GMP video encode For both patches here: Approval Request Comment [Feature/regressing bug #]: OpenH264 [User impact if declined]: on low bandwidth or on an overloaded machine, the video quality or framerate will go down too far, and on an overloaded machine the call may have problems with delay and other overload-induced problems. [Describe test coverage new/current, TBPL]: In use for VP8 in release; extends it to H.264. TBPL tests will exercise this to a degree using the fake plugin, though they can't easily force overloads or low bandwidth. Tested manually by running builds in the background and watching the video drop resolution (get fuzzier). Also 2.0? so will get tested there (minus the GMP stuff) with OMX 264. [Risks and why]: low; only risk would be exposing an existing bug in the GMP code when you close and re-open an encoder (which we would want to find in any case). [String/UUID change made/needed]: none

Comment on attachment 8481093 [details] [diff] [review] handle incoming resolution changes in GMP video encode See comment 9.

Randell, I ran some more test with Bug 1060249 with https://hg.mozilla.org/mozilla-central/rev/f8055a6d7d10. It apparently triggers resolution change which requires the reconfigure of the codec and this operation causes the corruption of the encoded data. Does it actually stop the encoder and re-start the encoder with new configuration? As encoder doesn't support the run-time resolution change, it needs to be brought down and re-started,again. Here is partial log: 17940 211:09-03 01:09:21.155 1070 1405 I PRLog : 2014-09-03 01:09:21.165607 UTC - 25194136[b10fcd00]: [ViECaptureThrea|WebrtcOMXH264VideoCodec] WebrtcOMXH264VideoCodec.cpp:788: WebrtcOMXH264VideoEncoder:0xb3844470 reconfiguring encoder 240x320 @ 30 fps 17941 209:09-03 01:09:21.155 1070 1405 I PRLog : 2014-09-03 01:09:21.165739 UTC - 25194136[b10fcd00]: [ViECaptureThrea|WebrtcOMXH264VideoCodec] WebrtcOMXH264VideoCodec.cpp:829: WebrtcOMXH264VideoEncoder:0xb3844470 configuring encoder 240x320 @ 30 fps, rate 242 kbps 24639 211:09-03 01:09:25.625 1070 1405 I PRLog : 2014-09-03 01:09:25.640700 UTC - 25194136[b10fcd00]: [ViECaptureThrea|WebrtcOMXH264VideoCodec] WebrtcOMXH264VideoCodec.cpp:788: WebrtcOMXH264VideoEncoder:0xb3844470 reconfiguring encoder 180x240 @ 30 fps

(In reply to Jay from comment #12) > Randell, > > I ran some more test with Bug 1060249 with > https://hg.mozilla.org/mozilla-central/rev/f8055a6d7d10. It apparently > triggers resolution change which requires the reconfigure of the codec and > this operation causes the corruption of the encoded data. Does it actually > stop the encoder and re-start the encoder with new configuration? As encoder > doesn't support the run-time resolution change, it needs to be brought down > and re-started,again. The log quoted does show an encoder reconfigure.... We use the same encoder reconfigure to handle large framerate changes. Question: does the *decoder* require reconfig on a resolution change? Software decoders generally don't (and are configured for the maximum resolution expected, or resolution isn't part of config). We config the OMX Decoder on the first SPS, but never again. I don't know if this is the issue (I would hope the hardware decoder allows for resolution changes), but if it doesn't I'll open a bug and attach a patch.

(In reply to Randell Jesup [:jesup] from comment #13) > (In reply to Jay from comment #12) > > Randell, > > > > I ran some more test with Bug 1060249 with > > https://hg.mozilla.org/mozilla-central/rev/f8055a6d7d10. It apparently > > triggers resolution change which requires the reconfigure of the codec and > > this operation causes the corruption of the encoded data. Does it actually > > stop the encoder and re-start the encoder with new configuration? As encoder > > doesn't support the run-time resolution change, it needs to be brought down > > and re-started,again. > > The log quoted does show an encoder reconfigure.... We use the same encoder > reconfigure to handle large framerate changes. > > Question: does the *decoder* require reconfig on a resolution change? > Software decoders generally don't (and are configured for the maximum > resolution expected, or resolution isn't part of config). > > We config the OMX Decoder on the first SPS, but never again. > > I don't know if this is the issue (I would hope the hardware decoder allows > for resolution changes), but if it doesn't I'll open a bug and attach a > patch. Decoder does allow the resolution change. I will try to capture the video dump to see if it is encoder or decoder problem.

(In reply to Jay from comment #14) > (In reply to Randell Jesup [:jesup] from comment #13) > > (In reply to Jay from comment #12) > > > Randell, > > > > > > I ran some more test with Bug 1060249 with > > > https://hg.mozilla.org/mozilla-central/rev/f8055a6d7d10. It apparently > > > triggers resolution change which requires the reconfigure of the codec and > > > this operation causes the corruption of the encoded data. Does it actually > > > stop the encoder and re-start the encoder with new configuration? As encoder > > > doesn't support the run-time resolution change, it needs to be brought down > > > and re-started,again. > > > > The log quoted does show an encoder reconfigure.... We use the same encoder > > reconfigure to handle large framerate changes. > > > > Question: does the *decoder* require reconfig on a resolution change? > > Software decoders generally don't (and are configured for the maximum > > resolution expected, or resolution isn't part of config). > > > > We config the OMX Decoder on the first SPS, but never again. > > > > I don't know if this is the issue (I would hope the hardware decoder allows > > for resolution changes), but if it doesn't I'll open a bug and attach a > > patch. > > Decoder does allow the resolution change. I will try to capture the video > dump to see if it is encoder or decoder problem. I am seeing that the video from the encoder output has wrong resolution. I need to check with team if the sequence of resolution change is same as FPS or bitrate change.

Randell, It looks like the encoder looks ok. However, it is generated image with 192x240 as you pointed in the email it tries to make it the multiple of marcroblock (16). So, this cause decoder to generate 192x240 image instead of 180x240. Can webRTC stack reconfigure the rendering path to crop the image or re-configure it to 192x240. From the webRTC log, I see that the correct resolution is passed to webRTC. I PRLog : 2014-09-04 22:52:25.079866 UTC - 7102456[b05fff80]: [CodecLooper|WebrtcOMXH264VideoCodec] WebrtcOMXH264VideoCodec.cpp:479: Decoder NewFrame: 192 bytes, 240x192, timestamp -5634231832259067664, renderTimeMs 3618084532

Also answered by email: the Decoder side doesn't know if the encode side was fed 180x240 or 192x240; it just knows that the output frame is 192x240. The decode side can handle decode resolution changes (vp8 or H.264). There's no signaling of the frame size; the decode size can change at any time as far as the rendering pipeline is concerned.

Randell, Actually, the problem seems to be in other place. I just captured the decoder input stream (H.264) and the decoder input stream get corrupted. However, the encoder output stream looks ok. This is a surprise to me. So, the issue might be in the upper stack. Any idea where the problem can be? Is there any hook to dump the video stream into a file from webRTC TX and RX port? Please, take a look at the attached mp4 file. dec_input.mp4 is the input stream to decoder. enc_output.mp4 is the output stream from the encoder

WIP to force resolutions to macroblock multiples

Per jay, these patches (with one !=/== fix) fix content resolution changing(!) I'll spin off a separate bug to land them on. With those, we should be able to land patch one here on b2g32. The cause was that the encoder, unlike the start-of-stream, generated a single buffer with SPS/PPS/iframe, with the SYNCFRAME flag but NOT the CODECCONFIG flag. Normally it starts with SPS/PPS with CODECCONFIG, then an iframe with SYNCFRAME. We record the SPS/PPS on a CODECCONFIG to insert later if an iframe is generated without SPS/PPS. We had code to explicitly handle a set of SPS/PPS w/ CODECCONFIG, and iframe with SYNCFRAME in mid-stream, and a MOZ_ASSERT() to fire on CODECCONFIG+SYNCFRAME in one buffer, because the code couldn't (and from what I remember being told, didn't need to) handle SPS/PPS/iframe in one buffer. However, since the CODECCONFIG flag wasn't set, it wouldn't assert. The net result was we didn't realize the SPS/PPS were there; we inserted the old SPS/PPS, sent that and the iframe, and then on all future iframes resent the old SPS/PPS before them (instead of the new one, which we didn't realize was there). Support for the config tag to always generate SPS/PPS would help simplify this in the future.