1404250 - Check failed: streams[i].max_bitrate_bps >= streams[i].target_bitrate_bps (10000 vs. 30000)

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, enhancement, P3)

Description

[Andreas Pehrson [:pehrsons]]

I took fippo's fiddle from bug 1404039 comment 2 and tightened the b=TIAS a bit, and switched to fake media in [1].

The tightened TIAS causes the max_bitrate to go below the target_bitrate and so we fail the assert at [2].

We should fix this in VideoConduit and land a testcase based on the fiddle.


[1] https://jsfiddle.net/pehrsons/yxbLvjm6/15/
[2] http://searchfox.org/mozilla-central/rev/20e41d4a61a8f5e34c9cf357304b78b3e9bced8a/media/webrtc/trunk/webrtc/modules/video_coding/video_codec_initializer.cc#192

Comment 1

[Dan Minor [:dminor]]

Attachment: Bug 1404250 - Ensure that target bitrate is between minimum and maximum bitrates in VideoConduit;

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

Comment 2

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8917034 [details]
Bug 1404250 - Ensure that target bitrate is between minimum and maximum bitrates in VideoConduit;

https://reviewboard.mozilla.org/r/188048/#review193498

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:1674
(Diff revision 1)
>    }
> -  out_start = std::max(out_start, out_min);
>  
> +  // Ensure that min <= start <= max
> +  if (out_max < out_min) {
> +    std::swap(out_min, out_max);

Hmm, so with min=30, max=25 we end up with a bitrate range of [25, 30]. I would have expected one of the two to take precedence as the stronger limit, i.e., [25, 25] or [30, 30].

Do you have better justification? Any spec to rely on?

Comment 3

[Dan Minor [:dminor]]

(In reply to Andreas Pehrson [:pehrsons] from comment #2)
> Comment on attachment 8917034 [details]
> Bug 1404250 - Ensure that target bitrate is between minimum and maximum
> bitrates in VideoConduit;
> 
> https://reviewboard.mozilla.org/r/188048/#review193498
> 
> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:1674
> (Diff revision 1)
> >    }
> > -  out_start = std::max(out_start, out_min);
> >  
> > +  // Ensure that min <= start <= max
> > +  if (out_max < out_min) {
> > +    std::swap(out_min, out_max);
> 
> Hmm, so with min=30, max=25 we end up with a bitrate range of [25, 30]. I
> would have expected one of the two to take precedence as the stronger limit,
> i.e., [25, 25] or [30, 30].
> 
> Do you have better justification? Any spec to rely on?

It looks like upstream webrtc.org makes the maximum take precedence [1]. Since their present is our future, I think it makes sense to do what they are doing. I couldn't find anything spec related for this, but perhaps I was just looking in the wrong places.

[1] https://cs.chromium.org/chromium/src/third_party/webrtc/call/call.cc?type=cs&sq=package:chromium&l=972

Comment 4

[Dan Minor [:dminor]]

Comment on attachment 8917034 [details]
Bug 1404250 - Ensure that target bitrate is between minimum and maximum bitrates in VideoConduit;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188048/diff/1-2/

Comment 5

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8917034 [details]
Bug 1404250 - Ensure that target bitrate is between minimum and maximum bitrates in VideoConduit;

https://reviewboard.mozilla.org/r/188048/#review194818

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3d089604ad0d
Ensure that target bitrate is between minimum and maximum bitrates in VideoConduit; r=pehrsons

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3d089604ad0d