881935 - Support negotiation of video resolution

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Shian-Yow Wu [:swu]]

To avoid the performance problem caused by improper video resolution higher than a device can handle, we should support negotiation of video resolution.

We need to:
1. Support required subset RFC 6236.
2. Get the list of supported video resolution from device.
3. Use negitiated video resolution on the video stream.

Comment 1

[u459114]

Hi Adam,
This is the issue we memtioned last week. To respect SDP framesize and width on reciever side.

Comment 2

[Shian-Yow Wu [:swu]]

Attachment: Patch: SDP parsing/building for max-fs and max-fr parameters

SDP parsing/building code for max-fs and max-fr fmtp parameters described in:
http://tools.ietf.org/html/draft-ietf-payload-vp8-08

RFC 6236 and draft-ietf-payload-vp8-08 define negotiation on video resolution in different ways, and which one to use haven't been decided yet.  We can have both of them in the future and first implement the one that better fits our current need.

Comment 3

[Eric Rescorla (:ekr)]

CJ,

Was there a specific question for me above the one for Adam in c2?

Comment 4

[Adam Roach [:abr]]

Comment on attachment 767616 [details] [diff] [review]
Patch: SDP parsing/building for max-fs and max-fr parameters

Review of attachment 767616 [details] [diff] [review]:
-----------------------------------------------------------------

All of your changes here look correct, and I think you're on the right path. It would appear that the next steps are adding the bridge code that allows specification of the parameters according to device capabilities, and reading the SDP values to adjust video stream encoding.

Please fix the following formatting issues:

gsm_sdp.c:1181: trailing whitespace
gsm_sdp.c:1193: Line is 85 characters long; please wrap to 80
gsm_sdp.c:1197: Line is 85 characters long; please wrap to 80
sdp.h:1555: tab character
sdp.h:1556: tab character
sdp.h:1557: tab character
sdp.h:1558: tab character
sdp_attr.c:1750: tab character
sdp_attr.c:1750: Line is 87 characters long; please wrap to 80
sdp_attr.c:1751: tab character
sdp_attr.c:1752: tab character
sdp_attr.c:1752: Line is 90 characters long; please wrap to 80
sdp_attr.c:1753: tab character
sdp_attr.c:1755: tab character
sdp_attr.c:1757: tab character
sdp_attr.c:1758: tab character
sdp_attr.c:1759: tab character
sdp_attr.c:1760: tab character
sdp_attr.c:1765: Line is 98 characters long; please wrap to 80
sdp_attr.c:1769: tab character
sdp_attr.c:1770: tab character
sdp_attr.c:1772: tab character
sdp_attr.c:1773: trailing whitespace

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +1182,5 @@
> +            /* TODO: Get max_fs & max_fr from device configuration */
> +            if (max_fs || max_fr) {
> +                if (sdp_add_new_attr(sdp_p, level, 0, SDP_ATTR_FMTP, &a_inst)
> +                    != SDP_SUCCESS) {
> +                    return;

We want to log an error here.

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
@@ +1750,5 @@
> +	    fmtp_ptr = sdp_getnextstrtok(fmtp_ptr, tmp, sizeof(tmp), "; \t", &result1);
> +	    if (result1 != SDP_SUCCESS) {
> +	        fmtp_ptr = sdp_getnextstrtok(fmtp_ptr, tmp, sizeof(tmp), " \t", &result1);
> +	        if (result1 != SDP_SUCCESS) {
> +                    sdp_attr_fmtp_no_value(sdp_p, "max_fr");

I see that other parameters (like max-fs) pass "max_fs" instead of "max-fs" in this error reporting. However, since this will be rendered to developers, I think we want to keep it as close to the actual SDP syntax as possible; I would use "max-fs" here (and below).

::: media/webrtc/signaling/test/sdp_unittests.cpp
@@ +513,5 @@
> +  AddNewFmtpMaxFr(level, 30);
> +  std::string body = SerializeSdp();
> +  ASSERT_NE(body.find("a=fmtp:120 max-fr=30"), std::string::npos);
> +}
> +

I think we need a combined test case here; one that produces both max-fs and max-fr on the same line.

Comment 5

[Shian-Yow Wu [:swu]]

Thanks for the comments, Adam.  I will work on the next steps and address comments in next patch.

When encoding parameters in "a=fmtp" for codec, we see some unwanted parameters.
For example:
a=fmtp:120 max-fs=300;parameter-add=1;usedtx=0;stereo=0;useinbandfec=0;cbr=0

This is a separate issue, and bug 888569 was filed to address it.

Comment 6

[Shian-Yow Wu [:swu]]

Attachment: Part 1: SDP parsing/building for max-fs and max-fr parameters

Addressed review comments.

Comment 7

[Shian-Yow Wu [:swu]]

Attachment: (WIP) Part 2: Device configuration for max-fs and max-fr

WIP patch that supports:
1) Encode max-fs and max-fr to SDP according to device preference.
2) Video stream encoding base on received max-fs in SDP.

TODO:
Apply max-fr to video stream encoding.

Comment 8

[Shian-Yow Wu [:swu]]

Attachment: Part 2: Device configuration for max-fs and max-fr

Comment 9

[Shian-Yow Wu [:swu]]

Attachment: Part 1: SDP parsing/building for max-fs and max-fr parameters

Comment 10

[Shian-Yow Wu [:swu]]

Attachment: Part 2: Device configuration for max-fs and max-fr

Rebased from bug 844177.

Comment 11

[Shian-Yow Wu [:swu]]

Attachment: Part 2: Device configuration for max-fs and max-fr

Rebased from bug 844177.

Comment 12

[Eric Rescorla (:ekr)]

shian-yow,

so I can review this can you tell me which documents it is intended to conform to?

Comment 13

[Adam Roach [:abr]]

Comment on attachment 776974 [details] [diff] [review]
Part 1: SDP parsing/building for max-fs and max-fr parameters

Review of attachment 776974 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, with two minor nits.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +1178,5 @@
>                                                 SIPSDP_ATTR_ENCNAME_VP8);
>              (void) sdp_attr_set_rtpmap_clockrate(sdp_p, level, 0, a_inst,
>                                               RTPMAP_VIDEO_CLOCKRATE);
> +
> +            /* TODO: Get max_fs & max_fr from device configuration */

As a matter of practice, I really like to see bug #s any time there is a "TODO" in the code. I know that this one is fixed by a later patch on this same bug, but just in case we land them with any kind of time gap, please add "Bug 881935" to the comment. Thanks.

::: media/webrtc/signaling/test/sdp_unittests.cpp
@@ +532,5 @@
> +  InitLocalSdp();
> +  int level = AddNewMedia(SDP_MEDIA_VIDEO);
> +  AddNewFmtpMaxFsFr(level, 300, 30);
> +  std::string body = SerializeSdp();
> +  ASSERT_NE(body.find("a=fmtp:120 max-fs=300;max-fr=30\r\n"), std::string::npos);

Please wrap this line to 80 columns.

Comment 14

[Shian-Yow Wu [:swu]]

(In reply to Eric Rescorla (:ekr) from comment #12)
> shian-yow,
> 
> so I can review this can you tell me which documents it is intended to
> conform to?

It is intended to conform to draft-ietf-payload-vp8-09, which looks more practical to our current need, though it is specific for VP8.

http://tools.ietf.org/html/draft-ietf-payload-vp8-09

Implementation of RFC 6236 will be the next.

Comment 15

[Shian-Yow Wu [:swu]]

(In reply to Adam Roach [:abr] from comment #13)
> Comment on attachment 776974 [details] [diff] [review]
> Part 1: SDP parsing/building for max-fs and max-fr parameters
> 
> Review of attachment 776974 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me, with two minor nits.
> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
> @@ +1178,5 @@
> >                                                 SIPSDP_ATTR_ENCNAME_VP8);
> >              (void) sdp_attr_set_rtpmap_clockrate(sdp_p, level, 0, a_inst,
> >                                               RTPMAP_VIDEO_CLOCKRATE);
> > +
> > +            /* TODO: Get max_fs & max_fr from device configuration */
> 
> As a matter of practice, I really like to see bug #s any time there is a
> "TODO" in the code. I know that this one is fixed by a later patch on this
> same bug, but just in case we land them with any kind of time gap, please
> add "Bug 881935" to the comment. Thanks.
> 
> ::: media/webrtc/signaling/test/sdp_unittests.cpp
> @@ +532,5 @@
> > +  InitLocalSdp();
> > +  int level = AddNewMedia(SDP_MEDIA_VIDEO);
> > +  AddNewFmtpMaxFsFr(level, 300, 30);
> > +  std::string body = SerializeSdp();
> > +  ASSERT_NE(body.find("a=fmtp:120 max-fs=300;max-fr=30\r\n"), std::string::npos);
> 
> Please wrap this line to 80 columns.

Thank you, will address the comments.

Comment 16

[Eric Rescorla (:ekr)]

Comment on attachment 777750 [details] [diff] [review]
Part 2: Device configuration for max-fs and max-fr

Review of attachment 777750 [details] [diff] [review]:
-----------------------------------------------------------------

I think this breaks the frame size adaptation, which, if true, is bad.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +553,5 @@
> +  // Don't change resolution if width and height are defined in the original
> +  // codec configuration.
> +  if (mCurSendCodecConfig &&
> +      mCurSendCodecConfig->mWidth > 0 &&
> +      mCurSendCodecConfig->mHeight > 0)

These values appear to be based solely on the other side's capabilities, but if
our capture is smaller than that why aren't we letting that control?

Unless I'm missing something, we need some more sophisticated algorithm that
uses the input frame size if it's smaller than the codec config.

@@ +557,5 @@
> +      mCurSendCodecConfig->mHeight > 0)
> +  {
> +    return true;
> +  }
> + 

Whitespace.

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +1485,5 @@
>        config_raw = new mozilla::VideoCodecConfig(
>          payloads[i].remote_rtp_pt,
> +        ccsdpCodecName(payloads[i].codec_type),
> +        payloads[i].video.width,
> +        payloads[i].video.height,

Why are we taking a position on the receive size?

@@ +2112,5 @@
>      config_raw = new mozilla::VideoCodecConfig(
>        payload->remote_rtp_pt,
> +      ccsdpCodecName(payload->codec_type),
> +      payload->video.width,
> +      payload->video.height,

These are always nonzero, right?

@@ +2865,5 @@
>    return 0;
>  }
>  
> +uint32_t vcmGetVideoMaxFs(uint16_t codec) {
> +  nsresult rv;

Is it safe to call this on the SIPCC thread?

Would it make sense to pre-acquire the preference service on vcm startup so we don't need to get it here repeatedly.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +3438,3 @@
>                      switch (codec) {
>                          case RTP_VP8:
> +                            if(!max_width || !max_height) {

Is there a way for max_height to be 0 and not max_width or vice versa?

@@ +3445,2 @@
>                          case RTP_I420:
> +                            if(!max_width || !max_height) {

When is this happening and why are we treating it differently.

@@ +3446,5 @@
> +                            if(!max_width || !max_height) {
> +                                payload_info->video.width = 176;
> +                                payload_info->video.height = 144;
> +                            }
> +                            break;

IF I am reading this core correctly, we always get out of this fxn with video.height and video.width set.

::: media/webrtc/signaling/src/sipcc/core/gsm/h/gsm_sdp.h
@@ +53,5 @@
>  
> +typedef struct {
> +    uint32 width;
> +    uint32 height;
> +} video_resolution_table_t;

Put this defn. right before the defn of the table.

::: modules/libpref/src/init/all.js
@@ +197,5 @@
>  pref("media.navigator.video.default_fps",30);
>  pref("media.navigator.video.default_minfps",10);
> +#ifdef MOZ_WIDGET_GONK
> +pref("media.navigator.video.max_fs", 99); // max-fs 99 for QCIF(176*144)
> +pref("media.navigator.video.max_fr", 30);

Let's not assume we are doing QCIF here.

Comment 17

[Shian-Yow Wu [:swu]]

Thanks for the review comments.

Comment 18

[Adam Roach [:abr]]

Formally reassigning to Shian-Yow to reflect what's actually happening. :)

Comment 19

[Shian-Yow Wu [:swu]]

(In reply to Eric Rescorla (:ekr) from comment #16)
> I think this breaks the frame size adaptation, which, if true, is bad.
> 
> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> @@ +553,5 @@
> > +  // Don't change resolution if width and height are defined in the original
> > +  // codec configuration.
> > +  if (mCurSendCodecConfig &&
> > +      mCurSendCodecConfig->mWidth > 0 &&
> > +      mCurSendCodecConfig->mHeight > 0)
> 
> These values appear to be based solely on the other side's capabilities, but
> if
> our capture is smaller than that why aren't we letting that control?
> 
> Unless I'm missing something, we need some more sophisticated algorithm that
> uses the input frame size if it's smaller than the codec config.

I misunderstood the capture resolution with the received resolution.  New patch will use smaller one from configured resolution and capture resolution.

> 
> @@ +557,5 @@
> > +      mCurSendCodecConfig->mHeight > 0)
> > +  {
> > +    return true;
> > +  }
> > + 
> 
> Whitespace.

OK.

> 
> ::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
> @@ +1485,5 @@
> >        config_raw = new mozilla::VideoCodecConfig(
> >          payloads[i].remote_rtp_pt,
> > +        ccsdpCodecName(payloads[i].codec_type),
> > +        payloads[i].video.width,
> > +        payloads[i].video.height,
> 
> Why are we taking a position on the receive size?

No need, to be removed.

> 
> @@ +2112,5 @@
> >      config_raw = new mozilla::VideoCodecConfig(
> >        payload->remote_rtp_pt,
> > +      ccsdpCodecName(payload->codec_type),
> > +      payload->video.width,
> > +      payload->video.height,
> 
> These are always nonzero, right?

In next patch, these could be zero if no max-fs from remote SDP.

> 
> @@ +2865,5 @@
> >    return 0;
> >  }
> >  
> > +uint32_t vcmGetVideoMaxFs(uint16_t codec) {
> > +  nsresult rv;
> 
> Is it safe to call this on the SIPCC thread?
> 
> Would it make sense to pre-acquire the preference service on vcm startup so
> we don't need to get it here repeatedly.

Will do it in main thread and pre-acquire service in vcm constructor.

> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
> @@ +3438,3 @@
> >                      switch (codec) {
> >                          case RTP_VP8:
> > +                            if(!max_width || !max_height) {
> 
> Is there a way for max_height to be 0 and not max_width or vice versa?
> 
> @@ +3445,2 @@
> >                          case RTP_I420:
> > +                            if(!max_width || !max_height) {
> 
> When is this happening and why are we treating it differently.
> 
> @@ +3446,5 @@
> > +                            if(!max_width || !max_height) {
> > +                                payload_info->video.width = 176;
> > +                                payload_info->video.height = 144;
> > +                            }
> > +                            break;
> 
> IF I am reading this core correctly, we always get out of this fxn with
> video.height and video.width set.

If no max-fs in remote SDP, we don't need to set video.height and video.width at all, just leave zero to them.  The capture resolution will be used for video sending if they are zero.

To be changed in next patch.

> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/h/gsm_sdp.h
> @@ +53,5 @@
> >  
> > +typedef struct {
> > +    uint32 width;
> > +    uint32 height;
> > +} video_resolution_table_t;
> 
> Put this defn. right before the defn of the table.

To be addressed in next patch.

> 
> ::: modules/libpref/src/init/all.js
> @@ +197,5 @@
> >  pref("media.navigator.video.default_fps",30);
> >  pref("media.navigator.video.default_minfps",10);
> > +#ifdef MOZ_WIDGET_GONK
> > +pref("media.navigator.video.max_fs", 99); // max-fs 99 for QCIF(176*144)
> > +pref("media.navigator.video.max_fr", 30);
> 
> Let's not assume we are doing QCIF here.

The default max_fs/max_fr is device dependant, and they should be decided and changed by OEM partner.  Do you mean not to assume default resolution are different between desktop and B2G?

For B2G, I think a better place to configure them is device property(Gonk).  For example, the Peak phone's device property are configured in PRODUCT_PROPERTY_OVERRIDES field in the file 
B2G/device/geeksphone/peak/full_peak.mk

My suggestion:
1. Having common default values in preference, used by desktop and B2G.
2. For B2G, the values can be overridden by device property if exist.

Comment 20

[Shian-Yow Wu [:swu]]

Attachment: Part 1: SDP parsing/building for max-fs and max-fr parameters (r=abr)

Addressed previous review comments.

Comment 21

[Shian-Yow Wu [:swu]]

Attachment: Part 2: Device configuration for max-fs and max-fr

Addressed previous review comments.

Comment 22

[Eric Rescorla (:ekr)]

Comment on attachment 791241 [details] [diff] [review]
Part 2: Device configuration for max-fs and max-fr

Review of attachment 791241 [details] [diff] [review]:
-----------------------------------------------------------------

I want to apologize for steering you in the wrong direction here.

The problem is that we don't want to change the video aspect ratio.
Rather, we want to use the largest image size that has:

(a) The same aspect ratio as the incoming image.
(b) <= maxfs macroblocks.

This also means you need to pass max-fs down to VideoConduit
and let it sort it out based on the incoming camera size.

Here's the tail of the IRC log discussion between Tim, ABR, and myself on this
topic:

ekr: So taking a step back, we have an image with dimensions (X, Y) and a max-fs = M
[1:54pm] dbaron joined the chat room.
[1:54pm] abr: Okay, no. If we're operating within arbitrary resolutions, then there is a pretty easy mathematical way go get from [apsect_ratio, max_fs] to [height, width]
[1:54pm] ekr: Yes.
[1:55pm] ekr: So that's the proposal? Don't try to favor even multiples or anything?
[1:55pm] abr: And I agree that is absolutely what he should be doing.
[1:55pm] derf: ekr: We should favor even multiples.
[1:55pm] ekr: Not I am absolutely ignorant of what the scaler can do
[1:55pm] mbest left the chat room. (Ping timeout)
[1:55pm] derf: Not the least because I'm pretty sure our own code falls over if we don't get them.
[1:55pm] ekr: s/Not/Note/
[1:55pm] abr: I suspect we want to favor integer number of macroblocks in each direction
[1:56pm] derf: And as I said, I think we should also limit width and height separate to 8*sqrt(max-fs) (MBs).
[1:56pm] ekr: I suspect we wish to require integer number of macroblocks
[1:56pm] derf: *separately
[1:56pm] abr: Yeah, that.
[1:56pm] abr: But that guarantees an even multiple of pixels, right?
[1:56pm] abr: Or did you mean something else?
[1:56pm] derf: Yes.
[1:56pm] abr: (like power of 2?)
[1:56pm] ekr: anyway, we don't need to solve this hear.
[1:56pm] ekr: We'll let shain-yow take care of it
[1:56pm] abr: Right.
[1:57pm] abr: Basically: replace the table with math.
[1:57pm] khuey is now known as khuey|away.
[1:57pm] ekr: well, and move it into VideoConduit
[1:57pm] derf: Limiting to an even number of MBs is okay for now (we may want to relax it later, but it's not bad to start with).
[1:58pm] derf: Though I kind of think that if someone has a stream being sent to a client with no max-fs and the same stream being sent to a client with max-fs, and then we shouldn't change the resolution in the second case if it wouldn't violate max-fs.

Comment 23

[Eric Rescorla (:ekr)]

Oh, and obviously don't scale up....

Comment 24

[Shian-Yow Wu [:swu]]

Thanks for pointing out the aspect ratio issue and the comments from Ekr/ABR/Tim, will work on the new way to calculate maximum resolution.

Looking at the comments, I'd like to confirm again my understanding on macroblock is correct.

My understanding on macroblock is: 1 macroblock = 16*16 pixels.
For video resolution is 640*480, it has 1200 macroblocks(640*480/256).
And the width and height for max-fs=1200 with aspect ratio 4:3 will be 640 and 480.

Allow me to confirm some questions on macroblock:

1. What is the number of MBs for resolution 640*480?

2. About "limit width and height separately to 8*sqrt(max-fs) (MBs)", what will width/height be for the max-fs=100 with aspect ratio 4:3?
Base on this fomula, the width and height should be 80 and 60 respectively.

3. What does it mean by limiting to an even number of MBs?

Thank you.

Comment 25

[Eric Rescorla (:ekr)]

+ Tim

Comment 26

[Timothy B. Terriberry (:derf)]

(In reply to Shian-Yow Wu [:shianyow] from comment #24)
> 1. What is the number of MBs for resolution 640*480?

It is 1200, just as you said.

> 2. About "limit width and height separately to 8*sqrt(max-fs) (MBs)", what
> will width/height be for the max-fs=100 with aspect ratio 4:3?
> Base on this fomula, the width and height should be 80 and 60 respectively.

That would leave you with 4800 MBs, which would exceed the max-fs limit. The point of the separate limit on width and height is to limit the effect of extreme aspect ratios. This comes from a similar limit in H.264 levels, which I think we should apply also. Though I mis-remembered, they actually use sqrt(8*MaxFS), limiting the width and height to 28 MBs in your example.

In another example, consider max-fs=1000. Without this limit, we could have a video that is 16000x16 pixels, which is very extreme for a max-fs setting that does not even support 640x480. With the limit, the resolution with the most extreme aspect ratio (and a whole number of MBs) would be 1424x16.

> 3. What does it mean by limiting to an even number of MBs?

I think the question was about limiting things to an even number of pixels (not MBs), to simplify the handling of 4:2:0 chroma subsampling. Limiting things to a whole number of MBs automatically limits them to an even number of pixels, so there is nothing to worry about here.

Comment 27

[Shian-Yow Wu [:swu]]

Thanks Tim, now I got the point of separate limit on width and height.

Comment 28

[Shian-Yow Wu [:swu]]

(In reply to Timothy B. Terriberry (:derf) from comment #26)
> In another example, consider max-fs=1000. Without this limit, we could have
> a video that is 16000x16 pixels, which is very extreme for a max-fs setting
> that does not even support 640x480. With the limit, the resolution with the
> most extreme aspect ratio (and a whole number of MBs) would be 1424x16.
> 

Our goal is to limit resolution up to max-fs, while keeping same aspect ratio as input image from camera.  After more thought, I think the extreme situation won't happen, unless the input image from camera already has extreme aspect ratio.

Take a real example for max_fs=300.
1) Input resolution is 640x480
The adjusted resolution will be 320x240, this is the normal case, no problem.
2) Input resolution is 3072x100
If camera provides such strange resolution, is it good to apply the sqrt(8*MaxFS) to width and height seperately here which changes the aspect ratio?  Shoudn't we address the problem from the input source?

Comment 29

[Timothy B. Terriberry (:derf)]

(In reply to Shian-Yow Wu [:shianyow] from comment #28)
> If camera provides such strange resolution, is it good to apply the
> sqrt(8*MaxFS) to width and height seperately here which changes the aspect
> ratio?  Shoudn't we address the problem from the input source?

We may not control the input source. I.e., it may be a <video> tag or some other video source (another PC).

Instead of changing the aspect ratio, I was thinking more along the lines of choosing a resolution which obeyed the maximum width/height constraint _and_ maintained the same aspect ratio (or as close as we can come). I.e., max_fs=300 => max_width=48 MB => 3072x100 would get scaled to 768x25 (really 768x24 once we make sure we have an even height).

If "as close as we can come" isn't close enough, we'd ultimately want to either drop the whole-MB constraint (in your example neither 100 nor 24 satisfy that anyway), or letterbox/pillarbox. This is something I think we can defer to a future bug, however.

Comment 30

[Shian-Yow Wu [:swu]]

(In reply to Timothy B. Terriberry (:derf) from comment #29)
> Instead of changing the aspect ratio, I was thinking more along the lines of
> choosing a resolution which obeyed the maximum width/height constraint _and_
> maintained the same aspect ratio (or as close as we can come). I.e.,
> max_fs=300 => max_width=48 MB => 3072x100 would get scaled to 768x25 (really
> 768x24 once we make sure we have an even height).

If we keep the aspect ratio, the max-fs=300 scales resolution from 3072x100 to 1536x50, which obeys the max frame size constraint.  What is the purpose of the seperate sqrt(8*MaxFS) limit to width/height, which limiting it further to 768x24?  Could you tell me more about it?

Comment 31

[Shian-Yow Wu [:swu]]

Attachment: Part 2: Device configuration for max-fs and max-fr

Limit resolution to max-fs while keeping same aspect ratio as the incoming image.

Some doubts on extreme aspect ratio questions to be clarified before asking for review.

Comment 32

[Shian-Yow Wu [:swu]]

Comment on attachment 793970 [details] [diff] [review]
Part 2: Device configuration for max-fs and max-fr

Review of attachment 793970 [details] [diff] [review]:
-----------------------------------------------------------------

Tim, could you help to review on the part of calculating width/height from input aspect ratio and max-fs?

Comment 33

[Timothy B. Terriberry (:derf)]

Comment on attachment 793970 [details] [diff] [review]
Part 2: Device configuration for max-fs and max-fr

Review of attachment 793970 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty close, but there are still cases where we aren't respecting max_fs properly. See below.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +557,5 @@
> +  {
> +    int cur_res, max_res;
> +
> +    cur_res = width * height;
> +    max_res = mCurSendCodecConfig->mMaxFrameSize * 256;

Since nothing limits the value of mMaxFrameSize (which can be provided by content via the SDP), this can overflow. Besides being undefined behavior, even if it does the "obvious" thing, we may wind up with max_res == 0. What happens then?

@@ +563,5 @@
> +    // Limit resolution to max_fs, but don't scale up
> +    if (cur_res > max_res)
> +    {
> +      width = width * sqrt((float) max_res / (float) cur_res);
> +      height = height * sqrt((float) max_res / (float) cur_res);

So, doing this in pixels means the number of macroblocks is ceil(width/16.)*ceil(height/16.), which can be larger than max_fs.

Trivial example: max_fs=1, width=8, height=32. max_res=cur_res=256, so width and height remain unchanged, but the frame is 1x2 MBs.

@@ +568,5 @@
> +
> +      // Limit width/height seperately to limit effect of extreme aspect ratios
> +      int max_pixels, w0, h0;
> +
> +      max_pixels = (int) (sqrt(8 * (float) mCurSendCodecConfig->mMaxFrameSize)) * 16;

Converting to float here instead of double might lose precision, and requires another conversion before calling sqrt(), which takes a double. I'd just convert to double directly.

@@ +573,5 @@
> +
> +      if (width > max_pixels || height > max_pixels)
> +      {
> +        w0 = width;
> +        h0 = height;

You should save off the width and height before modifying them above, to avoid compounding rounding errors.

@@ +577,5 @@
> +        h0 = height;
> +        if (w0 > max_pixels)
> +        {
> +          w0 = max_pixels;
> +          h0 = w0 * height / width;

Can this product overflow? Rounding?

@@ +582,5 @@
> +        }
> +        if (h0 > max_pixels)
> +        {
> +          h0 = max_pixels;
> +          w0 = h0 * width / height;

Can this product overflow? Rounding?

@@ +585,5 @@
> +          h0 = max_pixels;
> +          w0 = h0 * width / height;
> +        }
> +        width = w0;
> +        height = h0;

I think you should swap the roles of w0 and width, and of h0 and height. Then these two lines wouldn't be needed.

@@ +592,5 @@
> +      // Favor even multiples of pixels for width and height
> +      width = width / 2;
> +      width = width * 2;
> +      height = height / 2;
> +      height = height * 2;

In theory these can reduce width or height to 0, which would be bad.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +3400,5 @@
> +
> +                    /* Set maximum frame size */
> +                    payload_info->video.max_fs = 0;
> +                    sdp_attr_get_fmtp_max_fs(sdp_p->dest_sdp, level, 0, 1,
> +                                             &payload_info->video.max_fs);

So, sdp_attr_get_fmtp_max_fs takes a u32 * for its value, and when parsing the SDP clamps it against UINT_MAX. video.max_fs is an int. What happens when the value overflows and goes negative?

Also, presumably the compiler warns here?

@@ +3405,5 @@
> +
> +                    /* Set maximum frame rate */
> +                    payload_info->video.max_fr = 0;
> +                    sdp_attr_get_fmtp_max_fr(sdp_p->dest_sdp, level, 0, 1,
> +                                             &payload_info->video.max_fr);

Same questions apply to max_fr.

::: modules/libpref/src/init/all.js
@@ +195,5 @@
>  pref("media.navigator.video.default_width",640);
>  pref("media.navigator.video.default_height",480);
>  pref("media.navigator.video.default_fps",30);
>  pref("media.navigator.video.default_minfps",10);
> +pref("media.navigator.video.max_fs", 1200); // max-fs 1200 for VGA(640*480)

If we're going to default to something here, I think we should default to 0 (unrestricted), and then override on specific platforms that require it.

Comment 34

[Shian-Yow Wu [:swu]]

Thanks for the review comments.

Comment 35

[Timothy B. Terriberry (:derf)]

(In reply to Timothy B. Terriberry (:derf) from comment #33)
> > +      width = width * sqrt((float) max_res / (float) cur_res);
> > +      height = height * sqrt((float) max_res / (float) cur_res);

BTW, I was also concerned about the influence of rounding effects here, so I wrote a program to exhaustively check all 14-bit widths and heights for all 16-bit max_fs values, which has been chugging away.

There are indeed cases which fail (meaning width*height > max_res after rescaling), but the example with the smallest max_fs I found was
  w=h=4113, max_fs=33833.
This scales to 2943x2943, at least on x86-64 Linux w/gcc 4.5.2 and glibc 2.12.2, but
  2943*2943 == 8661249 > 8661248 == 33833*256.

I haven't found any counter-examples where either the final width or height are even, so in practice I think we're still fine on this front, but something to be aware of.

Comment 36

[Shian-Yow Wu [:swu]]

(In reply to Timothy B. Terriberry (:derf) from comment #35)
> (In reply to Timothy B. Terriberry (:derf) from comment #33)
> > > +      width = width * sqrt((float) max_res / (float) cur_res);
> > > +      height = height * sqrt((float) max_res / (float) cur_res);
> 
> BTW, I was also concerned about the influence of rounding effects here, so I
> wrote a program to exhaustively check all 14-bit widths and heights for all
> 16-bit max_fs values, which has been chugging away.
> 
> There are indeed cases which fail (meaning width*height > max_res after
> rescaling), but the example with the smallest max_fs I found was
>   w=h=4113, max_fs=33833.
> This scales to 2943x2943, at least on x86-64 Linux w/gcc 4.5.2 and glibc
> 2.12.2, but
>   2943*2943 == 8661249 > 8661248 == 33833*256.
> 

Got it.  The origin purpose of using pixels instead of MBs to do rescaling is to achieve better precision.  Now seems we should use MBs to avoid edge cases caused by rouding effect.

Comment 37

[Timothy B. Terriberry (:derf)]

(In reply to Shian-Yow Wu [:shianyow] from comment #36)
> Got it.  The origin purpose of using pixels instead of MBs to do rescaling
> is to achieve better precision.  Now seems we should use MBs to avoid edge
> cases caused by rouding effect.

I think you can do both. Scale to MBs (call them mb_width and mb_height), matching aspect ratio as closely as possible while staying under max_fs. Then use that to compute separate limits on the width and height, in pixels. Apply those limits to the original frame dimensions in pixels, in addition to the 16*sqrt(8*max_fs) limit (e.g., enforce width <= 16*min(mb_width,sqrt(8*max_fs))). As long as both width and height fall under both limits, you'll satisfy the max_fs limit and have a pixel-accurate aspect ratio.

Comment 38

[Shian-Yow Wu [:swu]]

(In reply to Timothy B. Terriberry (:derf) from comment #33)
> Comment on attachment 793970 [details] [diff] [review]
> Part 2: Device configuration for max-fs and max-fr
> 
> Review of attachment 793970 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks pretty close, but there are still cases where we aren't
> respecting max_fs properly. See below.
> 
> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> @@ +557,5 @@
> > +  {
> > +    int cur_res, max_res;
> > +
> > +    cur_res = width * height;
> > +    max_res = mCurSendCodecConfig->mMaxFrameSize * 256;
> 
> Since nothing limits the value of mMaxFrameSize (which can be provided by
> content via the SDP), this can overflow. Besides being undefined behavior,

Yes, overflow could happen because max-fs from SDP is u32 instead of u16.
Doing in MBs in pixels can avoid this.

> even if it does the "obvious" thing, we may wind up with max_res == 0. What
> happens then?

There is an if condition in line 556 that makes sure max-fs != 0, so max_res will not be 0.
  if (mCurSendCodecConfig && mCurSendCodecConfig->mMaxFrameSize)

> 
> @@ +563,5 @@
> > +    // Limit resolution to max_fs, but don't scale up
> > +    if (cur_res > max_res)
> > +    {
> > +      width = width * sqrt((float) max_res / (float) cur_res);
> > +      height = height * sqrt((float) max_res / (float) cur_res);
> 
> So, doing this in pixels means the number of macroblocks is
> ceil(width/16.)*ceil(height/16.), which can be larger than max_fs.

Do you mean round(width/16.)*round(height/16.) instead?

> 
> Trivial example: max_fs=1, width=8, height=32. max_res=cur_res=256, so width
> and height remain unchanged, but the frame is 1x2 MBs.

In this example, isn't it more accurate to leave width and height unchanged?  Because 8x32 doesn't exceed max_res.

> 
> @@ +568,5 @@
> > +
> > +      // Limit width/height seperately to limit effect of extreme aspect ratios
> > +      int max_pixels, w0, h0;
> > +
> > +      max_pixels = (int) (sqrt(8 * (float) mCurSendCodecConfig->mMaxFrameSize)) * 16;
> 
> Converting to float here instead of double might lose precision, and
> requires another conversion before calling sqrt(), which takes a double. I'd
> just convert to double directly.

Got it.

> 
> @@ +573,5 @@
> > +
> > +      if (width > max_pixels || height > max_pixels)
> > +      {
> > +        w0 = width;
> > +        h0 = height;
> 
> You should save off the width and height before modifying them above, to
> avoid compounding rounding errors.

I don't understand this part, could you elaborate it?

> 
> @@ +577,5 @@
> > +        h0 = height;
> > +        if (w0 > max_pixels)
> > +        {
> > +          w0 = max_pixels;
> > +          h0 = w0 * height / width;
> 
> Can this product overflow? Rounding?

Do you mean overflow when w0 * height exceeds int32?  The width or height are uint16, I'm not sure if this will happen.

> 
> @@ +582,5 @@
> > +        }
> > +        if (h0 > max_pixels)
> > +        {
> > +          h0 = max_pixels;
> > +          w0 = h0 * width / height;
> 
> Can this product overflow? Rounding?

Same as above.

> 
> @@ +585,5 @@
> > +          h0 = max_pixels;
> > +          w0 = h0 * width / height;
> > +        }
> > +        width = w0;
> > +        height = h0;
> 
> I think you should swap the roles of w0 and width, and of h0 and height.
> Then these two lines wouldn't be needed.

Got it.

> 
> @@ +592,5 @@
> > +      // Favor even multiples of pixels for width and height
> > +      width = width / 2;
> > +      width = width * 2;
> > +      height = height / 2;
> > +      height = height * 2;
> 
> In theory these can reduce width or height to 0, which would be bad.

Got it.

> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
> @@ +3400,5 @@
> > +
> > +                    /* Set maximum frame size */
> > +                    payload_info->video.max_fs = 0;
> > +                    sdp_attr_get_fmtp_max_fs(sdp_p->dest_sdp, level, 0, 1,
> > +                                             &payload_info->video.max_fs);
> 
> So, sdp_attr_get_fmtp_max_fs takes a u32 * for its value, and when parsing
> the SDP clamps it against UINT_MAX. video.max_fs is an int. What happens
> when the value overflows and goes negative?
> 
> Also, presumably the compiler warns here?

Got it.

> 
> @@ +3405,5 @@
> > +
> > +                    /* Set maximum frame rate */
> > +                    payload_info->video.max_fr = 0;
> > +                    sdp_attr_get_fmtp_max_fr(sdp_p->dest_sdp, level, 0, 1,
> > +                                             &payload_info->video.max_fr);
> 
> Same questions apply to max_fr.

Got it.

> 
> ::: modules/libpref/src/init/all.js
> @@ +195,5 @@
> >  pref("media.navigator.video.default_width",640);
> >  pref("media.navigator.video.default_height",480);
> >  pref("media.navigator.video.default_fps",30);
> >  pref("media.navigator.video.default_minfps",10);
> > +pref("media.navigator.video.max_fs", 1200); // max-fs 1200 for VGA(640*480)
> 
> If we're going to default to something here, I think we should default to 0
> (unrestricted), and then override on specific platforms that require it.

Agreed.

Comment 39

[Shian-Yow Wu [:swu]]

A question specific to sqrt(8*max_fs) limit: what should we do when there's no max-fs?

Example:
When input image has resolution 3072x100, new resolution becomes:
1) Don't apply sqrt(8*max_fs) limit:
  No max-fs: 3072x100
  max-fs=1200: 3072x100
  max-fs=300: 1536x50
2) Apply sqrt(8*max_fs) limit:
  No max-fs: 3072x100?  or take 1200 as max-fs which limits resolution to 1552x50?
  max-fs=1200: 1552x50
  max-fs=300: 768x24

Comment 40

[Shian-Yow Wu [:swu]]

(In reply to Shian-Yow Wu [:shianyow] from comment #38)
> (In reply to Timothy B. Terriberry (:derf) from comment #33)
> > Since nothing limits the value of mMaxFrameSize (which can be provided by
> > content via the SDP), this can overflow. Besides being undefined behavior,
> 
> Yes, overflow could happen because max-fs from SDP is u32 instead of u16.
> Doing in MBs in pixels can avoid this.

Doing in MBs instead pixels can avoid this.

Comment 41

[Timothy B. Terriberry (:derf)]

(In reply to Shian-Yow Wu [:shianyow] from comment #38)
> There is an if condition in line 556 that makes sure max-fs != 0, so max_res
> will not be 0.
>   if (mCurSendCodecConfig && mCurSendCodecConfig->mMaxFrameSize)

I was saying that in the case of overflow, you might have mMaxFrameSize > 0, but 256*mMaxFrameSize == 0 (e.g., when mMaxFrameSize == 0x1000000).

> > So, doing this in pixels means the number of macroblocks is
> > ceil(width/16.)*ceil(height/16.), which can be larger than max_fs.
> 
> Do you mean round(width/16.)*round(height/16.) instead?

No, the actual video frame must code a full MB row and/or column, even if only part of it is going to be displayed. So a width of 16*N+2 requires a frame (N+1) MBs wide.

> > Trivial example: max_fs=1, width=8, height=32. max_res=cur_res=256, so width
> > and height remain unchanged, but the frame is 1x2 MBs.
> 
> In this example, isn't it more accurate to leave width and height unchanged?
> Because 8x32 doesn't exceed max_res.

If the width is 32, the encoded video frame must be 2 MBs across, but max_fs limits you to 1 MB total.

> > You should save off the width and height before modifying them above, to
> > avoid compounding rounding errors.
> 
> I don't understand this part, could you elaborate it?

You have already scaled width and height by some value and truncated the result to an integer. Now you are using the truncated value to compute another scaling factor, compounding the rounding errors. Instead, you should use the aspect ratio defined by the original width and height in these calculations.

> > Can this product overflow? Rounding?
> 
> Do you mean overflow when w0 * height exceeds int32?  The width or height
> are uint16, I'm not sure if this will happen.

Well, 0xFFFF*0xFFFF is clearly larger than 0x7FFFFFFF. For the <video> tag we use MAX_VIDEO_WIDTH*MAX_VIDEO_HEIGHT to limit the resolution of all clips (to avoid DoS attacks caused by allocating memory for very large frames), but we don't do that for video received by a PC, that I can tell. So either we need to impose a limit like that in the GIPS decoder somewhere, or we need to be prepared to deal with arbitrary dimensions here. See also mfbt/CheckedInt.h.

(In reply to Shian-Yow Wu [:shianyow] from comment #39)
> A question specific to sqrt(8*max_fs) limit: what should we do when there's
> no max-fs?

I don't think we should have a sqrt(8*max_fs) limit when there's no max_fs.

Comment 42

[Shian-Yow Wu [:swu]]

(In reply to Timothy B. Terriberry (:derf) from comment #41)
> > > Trivial example: max_fs=1, width=8, height=32. max_res=cur_res=256, so width
> > > and height remain unchanged, but the frame is 1x2 MBs.
> > 
> > In this example, isn't it more accurate to leave width and height unchanged?
> > Because 8x32 doesn't exceed max_res.
> 
> If the width is 32, the encoded video frame must be 2 MBs across, but max_fs
> limits you to 1 MB total.

What should be the rescaled width and height in this example?  If we calculate by MBs, the only valid result within the max_fs limit is 1x1 Mbs, which is 16x16 pixels.  This deviates a lot from original 1:4 aspect ratio.

Comment 43

[Timothy B. Terriberry (:derf)]

(In reply to Shian-Yow Wu [:shianyow] from comment #42)
> (In reply to Timothy B. Terriberry (:derf) from comment #41)
> > > > Trivial example: max_fs=1, width=8, height=32. max_res=cur_res=256, so width
> > > > and height remain unchanged, but the frame is 1x2 MBs.
> 
> What should be the rescaled width and height in this example?  If we
> calculate by MBs, the only valid result within the max_fs limit is 1x1 Mbs,
> which is 16x16 pixels.  This deviates a lot from original 1:4 aspect ratio.

If you calculate by MBs, and max_fs=1, _all_ videos get scaled to 1x1 MB. If you follow the approach I suggested in comment 37, you'll turn the 1x1 scaled MB size into a limit of 16 pixels on both the width and height. Then enforcing that limit on the original height will give you a 4x16 pixel video. I think that's the best you can do for this example.

Comment 44

[Shian-Yow Wu [:swu]]

Attachment: Part 2(v3): Device configuration for max-fs and max-fr

The new patch addressed review comments and applied new approach in comment 37.

Comment 45

[Shian-Yow Wu [:swu]]

Attachment: Part 2(v3): Device configuration for max-fs and max-fr

Minor update to use std::min() and include requires files to build on Linux.

Comment 46

[Shian-Yow Wu [:swu]]

In addition to rescaling sending video to max-fs/max-fr constraints, we may also want to change camera capture resolution whenever possible, because limiting at source saves local CPU resource.

It can be done after width/height/frame_rate getUserMedia constraints supported in Bug 907352.
Note that changing camera capture resolution changes local preview resolution, if we support that, it should be a configurable option.

Comment 47

[Timothy B. Terriberry (:derf)]

Comment on attachment 796428 [details] [diff] [review]
Part 2(v3): Device configuration for max-fs and max-fr

Review of attachment 796428 [details] [diff] [review]:
-----------------------------------------------------------------

This is pretty close, but I'd like to see it one more time with the comments addressed before it gets committed.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +562,5 @@
> +
> +    aspect_ratio = (double) width / (double) height;
> +
> +    mb_width = ceil(width / 16.);
> +    mb_height = ceil(height / 16.);

Please don't actually use floating point for this.
Use, e.g.,
  mb_width = (width + 15) >> 4;

@@ +564,5 @@
> +
> +    mb_width = ceil(width / 16.);
> +    mb_height = ceil(height / 16.);
> +
> +    cur_fs = mb_width * mb_height;

The product gets promoted to int, not unsigned, and can thus overflow, which is undefined.
This is safe:
  cur_fs = mb_width * (unsigned)mb_height

Alternatively, you can just make mb_width and mb_height unsigned ints. ARM instructions don't have 16-bit operands outside of a few special-purpose instructions, so it generally requires a bunch of extra instructions to operate on 16-bit values, anyway.

@@ +576,5 @@
> +      mb_width = mb_width * scale_ratio;
> +      if (mb_width == 0)
> +      {
> +        mb_width = 1;
> +      }

mb_width = std::max((unsigned)(mb_width * scale_ratio), 1);

would be clearer, I think.

@@ +588,5 @@
> +    // Limit width/height seperately to limit effect of extreme aspect ratios.
> +    max_width = 16 * std::min(mb_width,
> +        (unsigned short) sqrt(8 * (double) mCurSendCodecConfig->mMaxFrameSize));
> +    max_height = 16 * std::min(mb_height,
> +        (unsigned short) sqrt(8 * (double) mCurSendCodecConfig->mMaxFrameSize));

Again, please factor out the sqrt() into a common variable to guarantee it's only computed once.

Also, the cast to (unsigned short) can overflow (max possible value: 0x2D413). You're going to promote things back to int anyway when you multiply by 16 (and if you didn't, that multiply could overflow, since mb_width can be as large as 0x1000), so just cast to (unsigned).

@@ +590,5 @@
> +        (unsigned short) sqrt(8 * (double) mCurSendCodecConfig->mMaxFrameSize));
> +    max_height = 16 * std::min(mb_height,
> +        (unsigned short) sqrt(8 * (double) mCurSendCodecConfig->mMaxFrameSize));
> +
> +    if (aspect_ratio > (double) max_width / (double) max_height)

The following is equivalent and doesn't need any floating point (nor worse, divisions):
  if (width*max_height > max_width*height) {

@@ +595,5 @@
> +    {
> +      if (width > max_width)
> +      {
> +        width = max_width;
> +        height = width / aspect_ratio;

That means you can do this without floating point, too:
  height = max_width*height/width;
  width = max_width;

If you do this below, as well, you don't need aspect_ratio anywhere, and get rid of another division.

@@ +613,5 @@
> +    width = width * 2;
> +    if (width == 0)
> +    {
> +      width = 2;
> +    }

width = std::max(width & ~1, 2);

@@ -779,5 @@
>  WebrtcVideoConduit::CodecConfigToWebRTCCodec(const VideoCodecConfig* codecInfo,
>                                                webrtc::VideoCodec& cinst)
>  {
>    cinst.plType  = codecInfo->mType;
> -  // leave width/height alone; they'll be overridden on the first frame

This comment was useful. Please don't delete it.

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +2879,5 @@
> +static short vcmGetVideoMaxFs_m(uint16_t codec,
> +                                int32_t *max_fs) {
> +  nsCOMPtr<nsIPrefBranch> branch = VcmSIPCCBinding::getPrefBranch();
> +  if (branch) {
> +    branch->GetIntPref("media.navigator.video.max_fs", max_fs);

This function can fail, leaving *max_fs unset.
You need to check the return value.

@@ +2901,5 @@
> +static short vcmGetVideoMaxFr_m(uint16_t codec,
> +                                int32_t *max_fr) {
> +  nsCOMPtr<nsIPrefBranch> branch = VcmSIPCCBinding::getPrefBranch();
> +  if (branch) {
> +    branch->GetIntPref("media.navigator.video.max_fr", max_fr);

Same here.

Comment 48

[Shian-Yow Wu [:swu]]

(In reply to Timothy B. Terriberry (:derf) from comment #47)
> Comment on attachment 796428 [details] [diff] [review]
> Part 2(v3): Device configuration for max-fs and max-fr
> 
> Review of attachment 796428 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is pretty close, but I'd like to see it one more time with the comments
> addressed before it gets committed.

Thanks for your review during travel.

> 
> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> @@ +562,5 @@
> > +
> > +    aspect_ratio = (double) width / (double) height;
> > +
> > +    mb_width = ceil(width / 16.);
> > +    mb_height = ceil(height / 16.);
> 
> Please don't actually use floating point for this.
> Use, e.g.,
>   mb_width = (width + 15) >> 4;

Good to know this.

> 
> @@ +564,5 @@
> > +
> > +    mb_width = ceil(width / 16.);
> > +    mb_height = ceil(height / 16.);
> > +
> > +    cur_fs = mb_width * mb_height;
> 
> The product gets promoted to int, not unsigned, and can thus overflow, which
> is undefined.

Do you mean product of (unsigned short)*(unsigned short) gets promoted to (signed int) instead of (unsigned int)?  Or I misunderstand it here?

> This is safe:
>   cur_fs = mb_width * (unsigned)mb_height
> 
> Alternatively, you can just make mb_width and mb_height unsigned ints. ARM
> instructions don't have 16-bit operands outside of a few special-purpose
> instructions, so it generally requires a bunch of extra instructions to
> operate on 16-bit values, anyway.

OK, it looks better to make them unsigned ints.

> 
> @@ +576,5 @@
> > +      mb_width = mb_width * scale_ratio;
> > +      if (mb_width == 0)
> > +      {
> > +        mb_width = 1;
> > +      }
> 
> mb_width = std::max((unsigned)(mb_width * scale_ratio), 1);
> 
> would be clearer, I think.

Got it.

> 
> @@ +588,5 @@
> > +    // Limit width/height seperately to limit effect of extreme aspect ratios.
> > +    max_width = 16 * std::min(mb_width,
> > +        (unsigned short) sqrt(8 * (double) mCurSendCodecConfig->mMaxFrameSize));
> > +    max_height = 16 * std::min(mb_height,
> > +        (unsigned short) sqrt(8 * (double) mCurSendCodecConfig->mMaxFrameSize));
> 
> Again, please factor out the sqrt() into a common variable to guarantee it's
> only computed once.
> 
> Also, the cast to (unsigned short) can overflow (max possible value:
> 0x2D413). You're going to promote things back to int anyway when you
> multiply by 16 (and if you didn't, that multiply could overflow, since
> mb_width can be as large as 0x1000), so just cast to (unsigned).

Got it.

> 
> @@ +590,5 @@
> > +        (unsigned short) sqrt(8 * (double) mCurSendCodecConfig->mMaxFrameSize));
> > +    max_height = 16 * std::min(mb_height,
> > +        (unsigned short) sqrt(8 * (double) mCurSendCodecConfig->mMaxFrameSize));
> > +
> > +    if (aspect_ratio > (double) max_width / (double) max_height)
> 
> The following is equivalent and doesn't need any floating point (nor worse,
> divisions):
>   if (width*max_height > max_width*height) {

Got it.

> 
> @@ +595,5 @@
> > +    {
> > +      if (width > max_width)
> > +      {
> > +        width = max_width;
> > +        height = width / aspect_ratio;
> 
> That means you can do this without floating point, too:
>   height = max_width*height/width;
>   width = max_width;
> 
> If you do this below, as well, you don't need aspect_ratio anywhere, and get
> rid of another division.

Got it.

> 
> @@ +613,5 @@
> > +    width = width * 2;
> > +    if (width == 0)
> > +    {
> > +      width = 2;
> > +    }
> 
> width = std::max(width & ~1, 2);

Got it, that's much simpler.

> 
> @@ -779,5 @@
> >  WebrtcVideoConduit::CodecConfigToWebRTCCodec(const VideoCodecConfig* codecInfo,
> >                                                webrtc::VideoCodec& cinst)
> >  {
> >    cinst.plType  = codecInfo->mType;
> > -  // leave width/height alone; they'll be overridden on the first frame
> 
> This comment was useful. Please don't delete it.

OK.

> 
> ::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
> @@ +2879,5 @@
> > +static short vcmGetVideoMaxFs_m(uint16_t codec,
> > +                                int32_t *max_fs) {
> > +  nsCOMPtr<nsIPrefBranch> branch = VcmSIPCCBinding::getPrefBranch();
> > +  if (branch) {
> > +    branch->GetIntPref("media.navigator.video.max_fs", max_fs);
> 
> This function can fail, leaving *max_fs unset.
> You need to check the return value.

The return value is checked in config_get_video_max_fs() of prot_configmgr.c.
When return value of vcmGetVideoMaxFs() is succesful, max_fs is returned, otherwise 0.

uint32_t
config_get_video_max_fs(const rtp_ptype codec)
{
  uint32_t max_fs;

  if(vcmGetVideoMaxFs(codec, (int32_t *) &max_fs) == 0) {
    return max_fs;
  }
  return 0;
}

Comment 49

[Shian-Yow Wu [:swu]]

We have test cases for max-fs/max-fr on SDP parsing and encoding, I think we also need test cases in VideoConduit.  May I have comments from you?

Comment 50

[Adam Roach [:abr]]

For the rtcp-fb patch, I determined that the video engine code doesn't have accessors to read this information back out once it's been set. So, to test whether the correct settings were being performed, I added some attributes to cache the values I'd configured the video engine with, and accessors to allow the unit tests to check the values.

If you look at this patch, you'll see a couple of small changes to VideoConduit:

https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=880067&attachment=797452

And then, if you look at lines 1033-1042 of signaling_unittests.cpp, you'll see how I use these accessors to check that the expected values have been set.

Comment 51

[Timothy B. Terriberry (:derf)]

(In reply to Shian-Yow Wu [:shianyow] from comment #48)
> Do you mean product of (unsigned short)*(unsigned short) gets promoted to
> (signed int) instead of (unsigned int)?  Or I misunderstand it here?

You did not misunderstand. That is exactly what I meant.

> > > +    branch->GetIntPref("media.navigator.video.max_fs", max_fs);
> > 
> > This function can fail, leaving *max_fs unset.
> > You need to check the return value.
> 
> The return value is checked in config_get_video_max_fs() of prot_configmgr.c.
> When return value of vcmGetVideoMaxFs() is succesful, max_fs is returned,
> otherwise 0.

I meant you need to check the return value of GetIntPref(). If, e.g., the preference does not exist, then it will fail and not fill in max_fs, but you will tell the caller that vcmGetVideoMaxFS_m() succeeded anyway.

(In reply to Shian-Yow Wu [:shianyow] from comment #49)
> We have test cases for max-fs/max-fr on SDP parsing and encoding, I think we
> also need test cases in VideoConduit.  May I have comments from you?

I think some tests that the values you're computing actually wind up respecting max_fs would be great. Adam's approach to writing those seems sensible to me.

Comment 52

[Shian-Yow Wu [:swu]]

Attachment: Part 3: max-fs Video Conduit tests

Thanks for comments from Adam and Tim.

Attached is the patch of max-fs Video Conduit test cases, focusing on verifying correctness of new resolution from input resolution and max-fs.

Comment 53

[Shian-Yow Wu [:swu]]

Attachment: Part 2(v4): Device configuration for max-fs and max-fr

Thanks for Tim's review, comments addressed.

Comment 54

[Shian-Yow Wu [:swu]]

Attachment: Part 3: max-fs Video Conduit tests

Fixed some trailing spaces.

Comment 55

[Timothy B. Terriberry (:derf)]

Comment on attachment 798446 [details] [diff] [review]
Part 2(v4): Device configuration for max-fs and max-fr

Review of attachment 798446 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with one nit. Looks good!

::: media/webrtc/signaling/src/media-conduit/CodecConfig.h
@@ +65,5 @@
>  
>    VideoCodecConfig(int type, std::string name): mType(type),
> +                                                mName(name),
> +                                                mMaxFrameSize(0),
> +                                                mMaxFrameRate(0)

Good catch.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +556,5 @@
> +  // incoming image
> +  if (mCurSendCodecConfig && mCurSendCodecConfig->mMaxFrameSize)
> +  {
> +    unsigned int cur_fs, max_width, max_height, mb_width, mb_height, mb_max;
> +    double scale_ratio;

You should move this inside the if() block where it's used.

Comment 56

[Shian-Yow Wu [:swu]]

Attachment: Part 4: Signaling unit test for max-fs and max-fr

Adam,

This signaling unit test for max-fs and max-fr.
May I have your feedback on it?

Comment 57

[Shian-Yow Wu [:swu]]

Attachment: Part 2: Device configuration for max-fs and max-fr (r=derf)

Review comments addressed, thanks Tim!

Comment 58

[Adam Roach [:abr]]

Comment on attachment 798836 [details] [diff] [review]
Part 4: Signaling unit test for max-fs and max-fr

Review of attachment 798836 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not approving this right now because I think it's incomplete. I beleive you need a lot more test coverage. There are several ways the code could misbehave that would not be caught by this one test. Basically, you need to check that the prefs have the proper impact on the SDP (once for offer, once for answer); and separately that the SDP has the proper impact on the codec configuration. I would suggest you have at least the following several tests:

1) Set the max_fs and max_fr prefs; call a1_.createOffer(); and verify that the SDP contains the correct a= lines. Remove the prefs.

2) Without the prefs set, call a1_.createOffer() and manually add max-fs and max-fr prefs attributes to the offer SDP. Make sure a2's pipeline honors the restrictions.

3) Without the prefs set, run through a call to the point of a2_.createAnswer(). Manually add max-fs and max-fr prefs attributes to the answer SDP. Make sure a2's pipeline honors the restrictions.

4) Without the prefs set, run through a call to the point of a2_.setRemoteDescription(). Verify that the offer does not contain max-fs and max-fr attributes. Then, set the max_fs and max_fr prefs; call a2_.createAnswer(); and verify that the SDP contains the correct a= lines. Remove the prefs.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +2468,5 @@
> +  prefs->SetIntPref("media.navigator.video.max_fs", max_fs);
> +  prefs->SetIntPref("media.navigator.video.max_fr", max_fr);
> +
> +  a1_.CreateOffer(constraints, OFFER_AV,
> +                  DONT_CHECK_AUDIO | DONT_CHECK_VIDEO | DONT_CHECK_DATA);

Why are you setting all of the "DONT_CHECK_" flags? Either remove these or add a comment explaining why they are needed.

@@ +2471,5 @@
> +  a1_.CreateOffer(constraints, OFFER_AV,
> +                  DONT_CHECK_AUDIO | DONT_CHECK_VIDEO | DONT_CHECK_DATA);
> +  a1_.SetLocal(TestObserver::OFFER, a1_.offer());
> +
> +  ParsedSDP sdpWrapper(a1_.offer());

The ParsedSDP class is really only needed if you're using it to extract or modify the contents of the SDP. I would suggest removing it, and simply using a1_.offer() below.

@@ +2501,5 @@
> +
> +  std::cerr << "====== SendingMaxFrameSize=" << video_conduit->SendingMaxFs() << std::endl;
> +  std::cerr << "====== SendingMaxFrameRate=" << video_conduit->SendingMaxFr() << std::endl;
> +  ASSERT_EQ(video_conduit->SendingMaxFs(), max_fs);
> +  ASSERT_EQ(video_conduit->SendingMaxFr(), max_fr);

My understanding is that the prefs are global, and won't be reset between tests automatically, which may cause variation in any subsequent tests depending on whether this one is run.

Please ensure that any added/changed prefs are reset before the test case exits.

Comment 59

[Adam Roach [:abr]]

Comment on attachment 798836 [details] [diff] [review]
Part 4: Signaling unit test for max-fs and max-fr

Ah, sorry for the confusion -- I misread the f? as an r?. Fixing the flags.

Comment 60

[Shian-Yow Wu [:swu]]

(In reply to Adam Roach [:abr] from comment #58)
> Comment on attachment 798836 [details] [diff] [review]
> Part 4: Signaling unit test for max-fs and max-fr
> 
> Review of attachment 798836 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not approving this right now because I think it's incomplete. I beleive
> you need a lot more test coverage. There are several ways the code could
> misbehave that would not be caught by this one test. Basically, you need to
> check that the prefs have the proper impact on the SDP (once for offer, once
> for answer); and separately that the SDP has the proper impact on the codec
> configuration. I would suggest you have at least the following several tests:
> 
> 1) Set the max_fs and max_fr prefs; call a1_.createOffer(); and verify that
> the SDP contains the correct a= lines. Remove the prefs.
> 
> 2) Without the prefs set, call a1_.createOffer() and manually add max-fs and
> max-fr prefs attributes to the offer SDP. Make sure a2's pipeline honors the
> restrictions.
> 
> 3) Without the prefs set, run through a call to the point of
> a2_.createAnswer(). Manually add max-fs and max-fr prefs attributes to the
> answer SDP. Make sure a2's pipeline honors the restrictions.
> 
> 4) Without the prefs set, run through a call to the point of
> a2_.setRemoteDescription(). Verify that the offer does not contain max-fs
> and max-fr attributes. Then, set the max_fs and max_fr prefs; call
> a2_.createAnswer(); and verify that the SDP contains the correct a= lines.
> Remove the prefs.

These comments are helpful, thank you.

> 
> ::: media/webrtc/signaling/test/signaling_unittests.cpp
> @@ +2468,5 @@
> > +  prefs->SetIntPref("media.navigator.video.max_fs", max_fs);
> > +  prefs->SetIntPref("media.navigator.video.max_fr", max_fr);
> > +
> > +  a1_.CreateOffer(constraints, OFFER_AV,
> > +                  DONT_CHECK_AUDIO | DONT_CHECK_VIDEO | DONT_CHECK_DATA);
> 
> Why are you setting all of the "DONT_CHECK_" flags? Either remove these or
> add a comment explaining why they are needed.

I was intended to use "SHOULD_SENDRECV_AV".
But when we have a=fmtp for video like below example,
    a=rtpmap:120 VP8/90000
    a=fmtp:120 max-fs=300;max-fr=15
    a=sendrecv
it would fail in the checking at:
https://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/test/signaling_unittests.cpp#1088

1088       case SHOULD_SENDRECV_VIDEO:
1089             ASSERT_NE(sdp.find("a=rtpmap:120 VP8/90000\r\na=sendrecv"),
1090                   std::string::npos);

I don't have a good way to solve the problem for now.
I think we can at least add a new flag "SHOULD_VIDEO" to check RTP payload type only, and that's the case 0 in the video checking.

1077       case 0:
1078             ASSERT_EQ(sdp.find("a=rtpmap:120 VP8/90000"), std::string::npos);
1079         break;

> 
> @@ +2501,5 @@
> > +
> > +  std::cerr << "====== SendingMaxFrameSize=" << video_conduit->SendingMaxFs() << std::endl;
> > +  std::cerr << "====== SendingMaxFrameRate=" << video_conduit->SendingMaxFr() << std::endl;
> > +  ASSERT_EQ(video_conduit->SendingMaxFs(), max_fs);
> > +  ASSERT_EQ(video_conduit->SendingMaxFr(), max_fr);
> 
> My understanding is that the prefs are global, and won't be reset between
> tests automatically, which may cause variation in any subsequent tests
> depending on whether this one is run.
> 
> Please ensure that any added/changed prefs are reset before the test case
> exits.

That's a good point.

Comment 61

[Shian-Yow Wu [:swu]]

(In reply to Shian-Yow Wu [:shianyow] from comment #60)
> I think we can at least add a new flag "SHOULD_VIDEO" to check RTP payload
> type only, and that's the case 0 in the video checking.
> 
> 1077       case 0:
> 1078             ASSERT_EQ(sdp.find("a=rtpmap:120 VP8/90000"), std::string::npos);
> 1079         break;

To correct, "case 0" is inverted checking, so we can't use that.
We should have something like below, and same thing for AUDIO too.

      case SHOULD_CHECK_VIDEO:
            ASSERT_NE(sdp.find("a=rtpmap:120 VP8/90000"), std::string::npos);
        break;

Comment 62

[Shian-Yow Wu [:swu]]

Attachment: (WIP) Part 4: Signaling unit test for max-fs and max-fr

This WIP patch support 4 test cases for max-fs and max-fr.
  Case 1: Test max_fs and max_fr prefs have proper impact on SDP offer
  Case 2: Test max_fs and max_fr prefs have proper impact on SDP answer
  Case 3: Test SDP offer has proper impact on callee's codec configuration
  Case 4: Test SDP answer has proper impact on caller's codec configuration

Currently it assumes SDP has one set of max-fs and max-fr in CheckMaxFsFrSdp().
I think some mechanisms in bug 880067 can be used to check the correct set of max-fs and max-fr.  So I would like to wait for it to land first.

One thing to note:
Instead of closing send/receive streams for a1 and a2,
  a1_.CloseSendStreams();
  a1_.CloseReceiveStreams();
  a2_.CloseSendStreams();
  a2_.CloseReceiveStreams();
I close only:
  a1_.CloseSendStreams();
  a2_.CloseReceiveStreams();

I am not sure if we really need to close these streams.
When closing all 4 streams, it could crash when test case 4(MaxFsFrCallerCodec) finished.

Comment 63

[Shian-Yow Wu [:swu]]

Attachment: Crash log when closing send/recv streams for caller and callee

GDB back trace log of the crash problem mentioned in previous comment.

Comment 64

[Adam Roach [:abr]]

(In reply to Shian-Yow Wu [:shianyow] from comment #60)

> I was intended to use "SHOULD_SENDRECV_AV".
> But when we have a=fmtp for video like below example,
>     a=rtpmap:120 VP8/90000
>     a=fmtp:120 max-fs=300;max-fr=15
>     a=sendrecv
> it would fail in the checking at:
> https://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/test/
> signaling_unittests.cpp#1088
> 
> 1088       case SHOULD_SENDRECV_VIDEO:
> 1089             ASSERT_NE(sdp.find("a=rtpmap:120 VP8/90000\r\na=sendrecv"),
> 1090                   std::string::npos);
> 
> I don't have a good way to solve the problem for now.

Ah, yes. I've been intending to re-do these with the parsed SDP class for some time now, since they are very fragile (as you've just discovered). For now, set the flags to whatever is necessary to work, but include a comment indicating why you've done so (as a reminder to fix the flags once the SDP checks are fixed).

Comment 65

[Adam Roach [:abr]]

Comment on attachment 799449 [details] [diff] [review]
(WIP) Part 4: Signaling unit test for max-fs and max-fr

Review of attachment 799449 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, this generally looks good. The only thing that I might improve is handling pref resetting if the ASSERT checks fail (since they will return under such circumstances). I think the easiest way to do this would be to create an instance of a guard class at the beginning of the function that resets the prefs in its destructor. Something like:

> class FsFrPrefClearer {
>   public:
>     FsFrPrefClearer(nsCOMPtr<nsIPrefBranch> prefs): mPrefs(prefs) { }
>     ~FsFrPrefClearer() {
>       mPrefs->ClearUserPref("media.navigator.video.max_fs");
>       mPrefs->ClearUserPref("media.navigator.video.max_fr");
>     }
>   private:
>     nsCOMPtr<nsIPrefBranch> mPrefs;
> };
> 
> 
> TEST_F(SignalingTest, MaxFsFrInOffer)
> {
>   nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
>   ASSERT_TRUE(prefs);
>   FsFrPrefClearer prefClearer(prefs);
>   ...
> }

Comment 66

[Shian-Yow Wu [:swu]]

(In reply to Shian-Yow Wu [:shianyow] from comment #62)
> When closing all 4 streams, it could crash when test case
> 4(MaxFsFrCallerCodec) finished.

For this test, we don't need to close those streams, because no need to get packet count.
However it's potential bug to be tracked.

Filed Bug 912841 for tracking.

Comment 67

[Shian-Yow Wu [:swu]]

Attachment: Part 3: Media conduit unit test for max-fs

Tim,

Could you help to review it?

Comment 68

[Shian-Yow Wu [:swu]]

Attachment: Part 4: Signaling unit test for max-fs and max-fr

Adam,

Rebased on bug 880067, could you help to review it?

Comment 69

[Timothy B. Terriberry (:derf)]

Comment on attachment 808433 [details] [diff] [review]
Part 3: Media conduit unit test for max-fs

Review of attachment 808433 [details] [diff] [review]:
-----------------------------------------------------------------

Just a few nits, and one suggestion, but I won't require you to implement that to land this.

::: media/webrtc/signaling/test/mediaconduit_unittests.cpp
@@ +710,5 @@
>      EXPECT_TRUE(err != mozilla::kMediaConduitNoError);
>  
>    }
>  
> +  // Calculate new resolution for sending video by applying max-fs contraint

s/contraint/constraint/

@@ +728,5 @@
> +    err = mVideoSession->ConfigureSendMediaCodec(&cinst1);
> +    ASSERT_EQ(mozilla::kMediaConduitNoError, err);
> +
> +    // Send one frame
> +    int len = ((orig_width * orig_height) * 3 / 2);

Can you please MOZ_ASSERT that orig_width and orig_height are even when you do this?

@@ +740,5 @@
> +                                  mozilla::kVideoI420,
> +                                  0);
> +    PR_Free(frame);
> +
> +    // Get the new resolution been changed by max-fs constraint

"// Get the new resolution as adjusted by the max-fs constraint."

@@ +777,5 @@
> +
> +    // Small max-fs
> +    GetVideoResolutionWithMaxFs(8, 32, 1, &width, &height);
> +    ASSERT_EQ(width, 4);
> +    ASSERT_EQ(height, 16);

It would be nice to test a few random values on each run and verify that the results meet our hard constraints: ceil(width/16.)*ceil(height/16.) <= max_fs, and both width and height are even (I don't see an easy way to do much about the aspect ratio). Picking random values will give us much better coverage over time, even if we don't know the exact "right" answers.

Generally for randomized tests, we'd want to log the random seed so that failures are reproducible, though in this case simply printing the failing values (as you already do in GetVideoResolutionWtihMaxFs()) would probably be enough, since in theory successive tests shouldn't depend on each other.

Comment 70

[Shian-Yow Wu [:swu]]

Attachment: (WIP) Part 3: Media conduit unit test for max-fs with random values

Tim,

This patch added random values test, I found two problems during the test.

1. When 1 <= max_fs <= 5, it is possible that ceil(width/16.)*ceil(height/16.) > max_fs.

For example, below are potential failure values:
1)
Applying max_fs=3 to input resolution 872x136
New resolution: 64x8

2)
Applying max_fs=1 to input resolution 14x102
New resolution: 4x32

2. Potential crash after testing finished when we encode a video frame with
width or height is 2, such as 4x2.  This looks like VP8 encoder problem, will file a new bug for tracking.

Comment 71

[Shian-Yow Wu [:swu]]

Bug 919979 filed for tracking problem 2 in comment 70.

Comment 72

[Timothy B. Terriberry (:derf)]

(In reply to Shian-Yow Wu [:shianyow] from comment #70)
> This patch added random values test, I found two problems during the test.

Sounds like an excellent test, then!

> 1. When 1 <= max_fs <= 5, it is possible that
> ceil(width/16.)*ceil(height/16.) > max_fs.

This violates my understanding of how the current enforcement mechanism was designed to work, so we should figure this out and fix it. Otherwise, I'm not sure why these failures would be restricted to very small max_fs values.

Comment 73

[Shian-Yow Wu [:swu]]

(In reply to Timothy B. Terriberry (:derf) from comment #72)
> (In reply to Shian-Yow Wu [:shianyow] from comment #70)
> > This patch added random values test, I found two problems during the test.
> 
> Sounds like an excellent test, then!
> 
> > 1. When 1 <= max_fs <= 5, it is possible that
> > ceil(width/16.)*ceil(height/16.) > max_fs.
> 
> This violates my understanding of how the current enforcement mechanism was
> designed to work, so we should figure this out and fix it. Otherwise, I'm
> not sure why these failures would be restricted to very small max_fs values.

Root cause found.

If mb_width or mb_height was truncated to 0, they were to forced to 1 here:

  mb_width = std::max((unsigned) (mb_width * scale_ratio), (unsigned) 1);
  mb_height = std::max((unsigned) (mb_height * scale_ratio), (unsigned) 1);

Take an example on input resolution 160x8 with max_fs=5.  The mb_width and mb_height will be 7x1(MBs) in the mechanism.

Comment 74

[Shian-Yow Wu [:swu]]

I plan to make two improvements on the mechanism.

1. Adjust mb_width and mb_height if they were truncated to zero

If mb_width was truncated to 0, add it to 1 and decrease mb_height until the constraint mb_width*mb_height <= max_fs is met, and vice versa.

2. Improve accuracy of aspect ratio

Example:
  Resolution 872x136 with max_fs=3

The mb_width and mb_height will be 3 and 1 after applied max_fs.  So max_width and max_height will be 48 and 16.  To keep the same aspect ratio, the new width remains 48, and new height is truncated to 7(from 7.48) and later changed to 6 to favor even multiples.  However, 8 is a better value for closer aspect ratio.

We can force to add 1 to the new height before favoring even multiples.  If original value is even, adding 1 will be changed back after favoring even multiples.

Comment 75

[Shian-Yow Wu [:swu]]

Attachment: Part 2: Device configuration for max-fs and max-fr

Tim,

The new patch has following modifications, could you help to review it again?

1. Rebased.
2. Fix failure on small max-fs values.
3. Improve accuracy of aspect ratio.

Comment 76

[Shian-Yow Wu [:swu]]

Attachment: Part 3: Media conduit unit test for max-fs

Tim,

The patch contains random values test.

With new mechanism, the expected result of input resolution 3072x100 with max_fs=300 is changed from 768x24 to 768x26.

Comment 77

[Adam Roach [:abr]]

Comment on attachment 808435 [details] [diff] [review]
Part 4: Signaling unit test for max-fs and max-fr

Review of attachment 808435 [details] [diff] [review]:
-----------------------------------------------------------------

My earlier versions of the patch for bug 880067 had been making use of the WebrtcVideoConduit class, which was apparently in error. I apologize for the ensuing confusion. EKR pointed out that the unit tests should only use the abstract classes (VideoSessionConduit in this case) and not the WebRTC-specific ones. I'll call out the needed changes inline below.

I'm declining the review for the moment, but I think it's ready to go as soon as the tests are moved over to use VideoSessionConduit.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
@@ +172,5 @@
> +      return mCurSendCodecConfig->mMaxFrameRate;
> +    }
> +    return 0;
> +  }
> +

You need to be able to use these methods from VideoSessionConduit. Probably the easiest thing to do here is to add them to VideoSessionConduit as virtual methods, and leave the implementation down here.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +23,5 @@
>  #include "FakeMediaStreamsImpl.h"
>  #include "PeerConnectionImpl.h"
>  #include "PeerConnectionCtx.h"
> +#include "AudioConduit.h"
> +#include "VideoConduit.h"

Remove these two includes; they should not be necessary if you use VideoSessionConduit rather than WebrtcVideoConduit.

@@ +3132,5 @@
> +  mozilla::MediaSessionConduit *conduit = pipeline->Conduit();
> +  ASSERT_TRUE(conduit);
> +  ASSERT_EQ(conduit->type(), mozilla::MediaSessionConduit::VIDEO);
> +  mozilla::WebrtcVideoConduit *video_conduit =
> +    static_cast<mozilla::WebrtcVideoConduit*>(conduit);

This needs to use mozilla::VideoSessionConduit rather than mozilla::WebrtcVideoConduit

@@ +3183,5 @@
> +  mozilla::MediaSessionConduit *conduit = pipeline->Conduit();
> +  ASSERT_TRUE(conduit);
> +  ASSERT_EQ(conduit->type(), mozilla::MediaSessionConduit::VIDEO);
> +  mozilla::WebrtcVideoConduit *video_conduit =
> +    static_cast<mozilla::WebrtcVideoConduit*>(conduit);

This should also use VideoSessionConduit.

Comment 78

[Shian-Yow Wu [:swu]]

Comment on attachment 809766 [details] [diff] [review]
Part 3: Media conduit unit test for max-fs

Need to change to use VideoSessionConduit, remove review request for now.

Comment 79

[Shian-Yow Wu [:swu]]

Attachment: Part 3: Media conduit unit test for max-fs

Change to use VideoSessionConduit to check values are as expected.

Comment 80

[Shian-Yow Wu [:swu]]

Attachment: Part 4: Signaling unit test for max-fs and max-fr

Change to use VideoSessionConduit to check values are as expected.

Comment 81

[Shian-Yow Wu [:swu]]

Attachment: Part 3: Media conduit unit test for max-fs

Minor fix on previous patch.

Comment 82

[Shian-Yow Wu [:swu]]

Attachment: Part 3: Media conduit unit test for max-fs

Comment 83

[Adam Roach [:abr]]

Comment on attachment 810409 [details] [diff] [review]
Part 4: Signaling unit test for max-fs and max-fr

Review of attachment 810409 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the slow review on this. It looks ready to land.

Comment 84

[Shian-Yow Wu [:swu]]

Attachment: Part 1: SDP parsing/building for max-fs and max-fr parameters (r=abr)

Rebased.

Comment 85

[Shian-Yow Wu [:swu]]

Attachment: Part 2: Device configuration for max-fs and max-fr

Tim,

As the review havn't been started yet, allow me to rebase again.
The major change from previous review can be refered in comment 75.

Thank you.

Comment 86

[Shian-Yow Wu [:swu]]

Attachment: Part 3: Media conduit unit test for max-fs

Change from previous review can be referred from comment 76, plus rebased.

Comment 87

[Shian-Yow Wu [:swu]]

Attachment: Part 4: Signaling unit test for max-fs and max-fr (r=abr)

Rebased.

Comment 88

[Shian-Yow Wu [:swu]]

(In reply to Adam Roach [:abr] from comment #83)
> Sorry for the slow review on this. It looks ready to land.

Thank you.

Comment 89

[Timothy B. Terriberry (:derf)]

Comment on attachment 814782 [details] [diff] [review]
Part 2: Device configuration for max-fs and max-fr

Review of attachment 814782 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with a few nits.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +645,5 @@
> +      if (mb_width == 0) {
> +        mb_width++;
> +        while (mb_width * mb_height > mCurSendCodecConfig->mMaxFrameSize) {
> +          mb_height--;
> +        }

You now know mb_width == 1 (which you can make clearer by setting it instead of incrementing it). That means this whole loop can be replaced by
  mb_height = std::min(mb_height, mCurSendCodecConfig->mMaxFrameSize);

@@ +666,5 @@
> +    {
> +      if (width > max_width)
> +      {
> +        // Adding 1 to improve accuracy from truncated value.
> +        height = max_width * height / width + 1;

If you want to increase accuracy, wouldn't
  height = (max_width * height + (width >> 1)) / width;
be more accurate?

It may also be worth commenting that this is safe because width >= height and the new width (max_width) is strictly smaller.

Comment 90

[Timothy B. Terriberry (:derf)]

Comment on attachment 814785 [details] [diff] [review]
Part 3: Media conduit unit test for max-fs

Review of attachment 814785 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with a couple of minor fixes.

::: media/webrtc/signaling/test/mediaconduit_unittests.cpp
@@ +830,5 @@
> +    ASSERT_EQ(height, 4);
> +
> +    // Random values.
> +    unsigned int seed;
> +    seed = rand() % 32768;

rand() returns a signed integer, for which % does not have the same semantics as & (in particular, negative values go to negative values). You should probably use
   seed = rand() & 32767;
instead, but this may not even be necessary (see below).

@@ +834,5 @@
> +    seed = rand() % 32768;
> +    cerr << "Testing random values with seed=" << seed << endl;
> +    for (int i = 0; i < 30; i++) {
> +      max_fs = rand_r(&seed) % 1000;
> +      orig_width = ((rand_r(&seed) % 2000) & ~1) + 2;

rand_r() isn't really portable, and I don't think there's much benefit to it here (since you're printing out the input parameters to the test). Suggest just using rand() here.

Comment 91

[Shian-Yow Wu [:swu]]

(In reply to Timothy B. Terriberry (:derf) from comment #89)
> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> @@ +645,5 @@
> > +      if (mb_width == 0) {
> > +        mb_width++;
> > +        while (mb_width * mb_height > mCurSendCodecConfig->mMaxFrameSize) {
> > +          mb_height--;
> > +        }
> 
> You now know mb_width == 1 (which you can make clearer by setting it instead
> of incrementing it). That means this whole loop can be replaced by
>   mb_height = std::min(mb_height, mCurSendCodecConfig->mMaxFrameSize);

Nice simplification.

> 
> @@ +666,5 @@
> > +    {
> > +      if (width > max_width)
> > +      {
> > +        // Adding 1 to improve accuracy from truncated value.
> > +        height = max_width * height / width + 1;
> 
> If you want to increase accuracy, wouldn't
>   height = (max_width * height + (width >> 1)) / width;
> be more accurate?

Taking the example: 
  Resolution 872x136 with max_fs=3

The mb_width and mb_height will be 3 and 1 after applied max_fs.  Then max_width and max_height will be 48 and 16.
The expected new width and height which best close to aspect ratio(6.41) are 48 and 8.
When adding 0.5, the new height is 7(truncated from 7.98) instead of the expected 8.

Wouldn't adding 1 getting it closer to aspect ratio?

As this might need more discussion, I can land the code without this accuracy improvement.

Comment 92

[Shian-Yow Wu [:swu]]

Attachment: Part 4: Signaling unit test for max-fs and max-fr (r=abr)

Avoid using std::to_string() as only supported by C++11 compiler, and would cause build fail on some platforms of try server.

Changed to use std:stringstream instead.

Comment 93

[Timothy B. Terriberry (:derf)]

(In reply to Shian-Yow Wu [:shianyow] from comment #91)
> The mb_width and mb_height will be 3 and 1 after applied max_fs.  Then
> max_width and max_height will be 48 and 16.
> The expected new width and height which best close to aspect ratio(6.41) are
> 48 and 8.
> When adding 0.5, the new height is 7(truncated from 7.98) instead of the
> expected 8.
> 
> Wouldn't adding 1 getting it closer to aspect ratio?

You are quite correct that
  48/7 > 872/136 > 48/8
= 6.85 >   6.41  > 6.00

and |6.85 - 6.41| = 0.44 > |6.41 - 6.00| = 0.41, suggesting we should pick 8.

However,

  8/48 > 136/872 > 7/48
= 0.1667 > 0.1560 > 0.1458

and |0.1667-0.1560| = 0.0107 > |0.1560 - 0.1458| = 0.0102, suggesting we should pick 7.

Since we write aspect ratio as w/h instead of h/w by convention only (and the code in the first branch optimizes for the former, while the code in the second branch optimizes for the latter), I don't think it makes sense to subtract two aspect ratios like this. Simply looking at things the other way gives the opposite answer. Attempting to minimize the distortion in height directly makes more sense.

However, since we're rounding to even values anyway, we aren't really choosing between 7 and 8. We're choosing between 6 and 8. Using my approach of minimizing the distortion in height directly, that would be
  height = 2*((max_width * height + width) / (2*width));

But this is equivalent to the code you wrote (once we force height to be even below). So I think it is okay to land your version. Sorry for not thinking things all the way through before. A comment explaining this wouldn't be amiss.

Comment 94

[Shian-Yow Wu [:swu]]

(In reply to Timothy B. Terriberry (:derf) from comment #93)
> However, since we're rounding to even values anyway, we aren't really
> choosing between 7 and 8. We're choosing between 6 and 8. Using my approach
> of minimizing the distortion in height directly, that would be
>   height = 2*((max_width * height + width) / (2*width));
> 
> But this is equivalent to the code you wrote (once we force height to be
> even below). So I think it is okay to land your version. Sorry for not
> thinking things all the way through before. A comment explaining this
> wouldn't be amiss.

Yes, forcing to even is the point that we can add 1.
It was mentioned in comment 74, but I misleaded you to comment 75 for the change.
I will explain it more clearly in the comment of the code, to avoid confusion. 
Thank you.

Comment 95

[Shian-Yow Wu [:swu]]

Attachment: Part 2: Device configuration for max-fs and max-fr (r=derf)

Addressed review comments:
1. Simplify mb_width and mb_height calculation in zero case.
2. More clearly comment to explain accuracy improvement.

Comment 96

[Shian-Yow Wu [:swu]]

Attachment: Part 3: Media conduit unit test for max-fs (r=derf)

Addressed review comments.
1. Avoid using rand_r() and removed using seed.

Comment 97

[Shian-Yow Wu [:swu]]

https://tbpl.mozilla.org/?tree=Try&rev=650d54244a27

Comment 98

[Shian-Yow Wu [:swu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8127c14d0bd5
https://hg.mozilla.org/integration/mozilla-inbound/rev/46d26336b01a
https://hg.mozilla.org/integration/mozilla-inbound/rev/3df544a5e7ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7ee3e6fba30

Comment 99

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8127c14d0bd5
https://hg.mozilla.org/mozilla-central/rev/46d26336b01a
https://hg.mozilla.org/mozilla-central/rev/3df544a5e7ce
https://hg.mozilla.org/mozilla-central/rev/b7ee3e6fba30