844177 - Codec resolution for sending should match input video resolution

Status: VERIFIED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

Really, it should be the minimum of the input res and the preferred max sending res selected by current output max bandwidth, but mirroring the input is a start.

We might want to put an upper cap on it in case someone tries to feed HD video through a 200K channel.  Or not.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: set codec resolution based on input mediastream resolution

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: set codec resolution based on input mediastream resolution

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 717253 [details] [diff] [review]
set codec resolution based on input mediastream resolution

Spreading the review-pain... ;-)

GCP/etc report this works

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 717253 [details] [diff] [review]
set codec resolution based on input mediastream resolution

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

This seems to duplicate a lot of state that is already in the VideoConduit.

In particular, VideoConduit->mCurSendCodecconfig already has
mHeight and mWidth parameters.

ISTM that the correct behavior here is simply to reset the
config and then do ConfigureSendVideoCodec().

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Modifying mCurSendCodecConfig would break how it's used for CheckCodecsForMatch() (which is the only thing mCurSendCodecConfig is used for).

Comment 6

[Eric Rescorla (:ekr)]

Then the right thing to do is to fix that.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

I think the codec config selected should be separate from the current-as-of-this-instant resolution, which can change frame-by-frame.  If we modify width/height, it will cause problems, and especially so if there are multiple configs for the same codec with different widths/heights, which appears to be envisioned by the code (as it compares those fields).  If you want to rearchitect how codec configs are handled or if they're doing something they shouldn't, that's a different bug.

I don't see "the last frame sent's width/height" (two ushorts) as "a lot of state".  It doesn't store the codec info separately, it asks the engine for the current config and then modifies it.  We need to store that amount of state somewhere to implement this.

Comment 8

[Timothy B. Terriberry (:derf)]

Comment on attachment 717253 [details] [diff] [review]
set codec resolution based on input mediastream resolution

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

Since Randell asked me to weigh in here...

(In reply to Randell Jesup [:jesup] from comment #7)
> I think the codec config selected should be separate from the
> current-as-of-this-instant resolution, which can change frame-by-frame.  If
> we modify width/height, it will cause problems, and especially so if there
> are multiple configs for the same codec with different widths/heights, which
> appears to be envisioned by the code (as it compares those fields).  If you
> want to rearchitect how codec configs are handled or if they're doing
> something they shouldn't, that's a different bug.

I don't think it is. The size of the "state" I think is mostly a red herring (I don't care about two ushorts one way or the other). The real issue is that ConfigureSendMediaCodec() does a bunch of work to change codec configurations, and you're skipping most of that work. Now, either that's necessary or not, but we should be changing that configuration in one place. If there's stuff we need to special-case (like always attempting to set the codec if the old width was 0 as this patch does), then that should be done the same way in both cases. If there's certain steps that we can skip because only the resolution is changing, then we should be checking for that and skipping them in ConfigureSendMediaCodec() (the same way that if all the codec parameters match the current codec, ConfigureSendMediaCodec() just returns early with a return value indicating that fact).

Which, by the way, is currently a little braindead (ValidateCodecConfig() already calls CheckCodecsForMatch(). When ConfigureSendMediaCodec() calls it again it is guaranteed to succeed, so the check is useless.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +533,5 @@
>    }
>  
> +  // enforce even width/height (paranoia)
> +  width &= ~1;
> +  height &= ~1;

1) This comes after the argument checks (so when a width or height of 1 gets converted to 0, we don't bail out like we're supposed to).
2) You're potentially changing the row width, which will screw up all kinds of things because the GIPS APIs don't provide stride parameters. So your video will still be completely screwed up, you'll just have masked the cause.

If you want to assert, that's fine (that's what the caller does... somewhat badly). but if you want to actually fix this safely, you either need to do it in the caller or you need to make some assumptions about what the buffer layout is with odd sizes and re-align the buffer here (IMHO a bad choice).

@@ +560,5 @@
> +    } else {
> +      CSFLogError(logTag, "%s: GetSendCodec failed, err %d", __FUNCTION__, err);
> +    }
> +    // If we tried and failed to set it, this will keep us from continuously
> +    // trying again (until it changes again)

Hopefully this also keeps us from continually trying again if it succeeds?

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Timothy B. Terriberry (:derf) from comment #8)

> Since Randell asked me to weigh in here...

Thanks

> (In reply to Randell Jesup [:jesup] from comment #7)
> > I think the codec config selected should be separate from the
> > current-as-of-this-instant resolution, which can change frame-by-frame.  If
> > we modify width/height, it will cause problems, and especially so if there
> > are multiple configs for the same codec with different widths/heights, which
> > appears to be envisioned by the code (as it compares those fields).  If you
> > want to rearchitect how codec configs are handled or if they're doing
> > something they shouldn't, that's a different bug.
> 
> I don't think it is. The size of the "state" I think is mostly a red herring
> (I don't care about two ushorts one way or the other). The real issue is
> that ConfigureSendMediaCodec() does a bunch of work to change codec
> configurations, and you're skipping most of that work. Now, either that's
> necessary or not, but we should be changing that configuration in one place.
> If there's stuff we need to special-case (like always attempting to set the
> codec if the old width was 0 as this patch does), then that should be done
> the same way in both cases. If there's certain steps that we can skip
> because only the resolution is changing, then we should be checking for that
> and skipping them in ConfigureSendMediaCodec() (the same way that if all the
> codec parameters match the current codec, ConfigureSendMediaCodec() just
> returns early with a return value indicating that fact).
> 
> Which, by the way, is currently a little braindead (ValidateCodecConfig()
> already calls CheckCodecsForMatch(). When ConfigureSendMediaCodec() calls it
> again it is guaranteed to succeed, so the check is useless.

If you're saying that the current setup has issues and should be rethought, I agree, totally. I do want to avoid anything possibly heavyweight where it's not needed (and the current ConfigureSendMediaCodec() really isn't designed for what we're talking about, as you mention).  What we really need to do is to rearchitect it to interact with the bandwidth controls to dynamically adjust the encoding resolution, to deal with source resolution changes, and to feed back "upstream" to gUM that we're encoding at X by Y, since in many cases the camera will scale for us (or at least scale to something close, we the amount of data copying we do is greatly reduced).

*That's* the redesign we need to do.  I do not want to spend much time redoing the current stuff short of the full redesign, and that is in my mind a very different bug than this one.  Right now gUM's effectively fixed resolution, with the resolution controllable via about:config prefs (and perhaps soon input constraints).  But that doesn't help people trying to use this if the PeerConnection rescales everything back (up) to 640x480 to encode.

I think the approach in this patch is a reasonably clean way to allow devs to work with other resolutions in the short term, without opening (much) the barrel of monkeys that the CodecConfig stuff is.  We *must* do the larger redesign, but that's probably at least in 23, and I'd rather have this than nothing.  However, I'll look to see if there's an equivalently low-impact patch via modifying ConfigureSendMediaCodec. 

> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> @@ +533,5 @@
> >    }
> >  
> > +  // enforce even width/height (paranoia)
> > +  width &= ~1;
> > +  height &= ~1;
> 
> 1) This comes after the argument checks (so when a width or height of 1 gets
> converted to 0, we don't bail out like we're supposed to).
> 2) You're potentially changing the row width, which will screw up all kinds
> of things because the GIPS APIs don't provide stride parameters. So your
> video will still be completely screwed up, you'll just have masked the cause.
> 
> If you want to assert, that's fine (that's what the caller does... somewhat
> badly). but if you want to actually fix this safely, you either need to do
> it in the caller or you need to make some assumptions about what the buffer
> layout is with odd sizes and re-align the buffer here (IMHO a bad choice).

I was putting these in there in response to your comment that they MUST be even.  :-)

An assert is good, but it would need to be backed by *something - error exit, etc unless you can tell me the GIPS code or VP8 code can cleanly handle it (and because of lack of a stride input, it really can't).  As you mentioned, I don't really believe we'll get odd values in here - but it's theoretically possible with streams sourced at video elements or especially if streams can be sourced from canvases.  Given the lack of stride, a kickout is probably best (an assert might even be the wrong thing if we think a stream can come from a user canvas or future MediaStream Processing-like api, and that nothing will enforce event sizes).

We should consider whether to suggest to Google/GIPS add an API with a stride to allow subsetting/cropping inputs without data copies.
 
> 
> @@ +560,5 @@
> > +    } else {
> > +      CSFLogError(logTag, "%s: GetSendCodec failed, err %d", __FUNCTION__, err);
> > +    }
> > +    // If we tried and failed to set it, this will keep us from continuously
> > +    // trying again (until it changes again)
> 
> Hopefully this also keeps us from continually trying again if it succeeds?

It does; I can revise the comment.

Comment 10

[Gian-Carlo Pascutto [:gcp]]

This hasn't moved in a month. Can we get this landed so Android can work?

Comment 11

[Maire Reavy [:mreavy]]

When we did our bug review last week, we forgot to mark this blocking Android.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Attachment: set codec resolution based on input mediastream resolution

Minor changes from tim's comments

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 742487 [details] [diff] [review]
set codec resolution based on input mediastream resolution

Forgot to remark this for ekr when I updated last week

Comment 15

[Eric Rescorla (:ekr)]

As far as I can tell, from an architectural perspective this patch is effectively unchanged from the patch that I r-ed.

I've read c7 and c8, but I'm unconvinced. IMO the right thing here is to keep this information in the codec structure, and manipulate it with ConfigureSendMediaCodec(), not to keep a separate set of state that you manipulate.

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

Attachment: set codec resolution based on input mediastream resolution

revised to remove resolultions from the config structures, as they shouldn't be used statically anyways.  Filed bug 877954 to actually implement bandwidth-based resolution picking

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 756437 [details] [diff] [review]
set codec resolution based on input mediastream resolution

You can look at the follow-on bug to get an idea of how the framework set up here will be used.

Comment 18

[Eric Rescorla (:ekr)]

Comment on attachment 756437 [details] [diff] [review]
set codec resolution based on input mediastream resolution

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

I agree we need some bandwidth adaptation here, but I don't
think this is quite right. If I am reading the temperature
of the WG, we are going to be doing some sort of 6236-style
resolution negotiation, we are going to need to pass indicators
of acceptable resolutions down into VideoConduit.

Arguably, the existing code is totally busted, since it only
supports one reoslution rather than ranges, but I'm not sure
it's improved by totally ignoring everything that happens
in signaling.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +547,5 @@
> +  // XXX This will do bandwidth-resolution adaptation as well - bug 877954
> +
> +  // Adapt to getUserMedia resolution changes
> +  // check if we need to reconfigure the sending resolution
> +  if (mSendingWidth == 0 ||

What if height is zero?

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Eric Rescorla (:ekr) from comment #18)
> I agree we need some bandwidth adaptation here, but I don't
> think this is quite right. If I am reading the temperature
> of the WG, we are going to be doing some sort of 6236-style
> resolution negotiation, we are going to need to pass indicators
> of acceptable resolutions down into VideoConduit.

Yes, we will, and that will need to be added when we get that (and mods made deeper in the GIPS code to support that as well).  We won't be getting that decision (and code for 6236) for FF24.

> Arguably, the existing code is totally busted, since it only
> supports one reoslution rather than ranges, but I'm not sure
> it's improved by totally ignoring everything that happens
> in signaling.

When we change what signaling is doing, we'll absolutely need to revise this to pass that in - but we should never allow the resolution specified at this level to be different (certainly not higher) than the input video level.  The processing of the resolution choice for *actual* encoding needs to take into account 6236 as it reacts to modified bandwidth and CPU use, but that's (actually or effectively) below this level.

> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> @@ +547,5 @@
> > +  // XXX This will do bandwidth-resolution adaptation as well - bug 877954
> > +
> > +  // Adapt to getUserMedia resolution changes
> > +  // check if we need to reconfigure the sending resolution
> > +  if (mSendingWidth == 0 ||
> 
> What if height is zero?

mSendingWidth=0 is doing double-duty as a flag that it hasn't been set.

Comment 20

[Jason Smith [:jsmith]]

This isn't a blocker to turn the prefs on, but we need this done before we ship FxAndroid's WebRTC support. Without this, the resolution you'll see for videos rendering from your Android phone will have incorrect video resolutions (e.g. objects you render via your camera will be rendered with the incorrect resolution).

Comment 21

[Eric Rescorla (:ekr)]

Comment on attachment 756437 [details] [diff] [review]
set codec resolution based on input mediastream resolution

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

::: media/webrtc/signaling/src/media-conduit/CodecConfig.h
@@ -56,5 @@
>     * corresponding webrtc::VideoCodec data-types.
>     */
>    int mType;
>    std::string mName;
> -  int mWidth;

Please add a comment here that this is where resolution goes.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +437,5 @@
>  }
>  
>  MediaConduitErrorCode
>  WebrtcVideoConduit::ConfigureRecvMediaCodecs(
> +  const std::vector<VideoCodecConfig* >& codecConfigList)

Align with open paren, or four spaces if you are having to wrap.

@@ +547,5 @@
> +  // XXX This will do bandwidth-resolution adaptation as well - bug 877954
> +
> +  // Adapt to getUserMedia resolution changes
> +  // check if we need to reconfigure the sending resolution
> +  if (mSendingWidth == 0 ||

Please don't double use 0 like this. Add an explicit state field.

@@ +622,5 @@
> +  // enforce even width/height (paranoia)
> +  MOZ_ASSERT(!(width & 1));
> +  MOZ_ASSERT(!(height & 1));
> +
> +  SelectSendResolution(width, height, capture_time);

Do we really want to be sending video if we can't set the resolution?

@@ +772,5 @@
>  {
>    cinst.plType  = codecInfo->mType;
> +  // width/height are just defaults that will be overridden on the first frame
> +  cinst.width   = 640;
> +  cinst.height  = 480;

I think this introduces a bug.

Consider the following sequence of events:

1. SetSendCodec(). This imposes 640x480.
2. Send some packets with, say QCIF. At this point mSendingWidth becomes 176, etc.
3. SetSendCodec() again. This will reimpose 640x480.
4. However, since mSendingWidth is unchanged, we don't reset the video size and we are now stuck in 640x480.

I think if you are going to remove height and width here you really need to remove it.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
@@ +172,5 @@
>                        mChannel(-1),
>                        mCapId(-1),
> +                      mCurSendCodecConfig(nullptr),
> +                      mSendingWidth(0),
> +                      mSendingHeight(0)

Please add some boolean to indicate whether we have set these.

@@ +231,5 @@
>    int mCapId;   // Capturer for this conduit
>    RecvCodecList    mRecvCodecList;
>    VideoCodecConfig* mCurSendCodecConfig;
> +  unsigned short mSendingWidth;
> +  unsigned short mSendingHeight;

If you're not going to use int here, please use a concrete type.

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

Attachment: interdiff

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

Attachment: set codec resolution based on input mediastream resolution

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Eric Rescorla (:ekr) from comment #21)
> ::: media/webrtc/signaling/src/media-conduit/CodecConfig.h
> @@ -56,5 @@
> >     * corresponding webrtc::VideoCodec data-types.
> >     */
> >    int mType;
> >    std::string mName;
> > -  int mWidth;
> 
> Please add a comment here that this is where resolution goes.

Done

> 
> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> @@ +437,5 @@
> >  }
> >  
> >  MediaConduitErrorCode
> >  WebrtcVideoConduit::ConfigureRecvMediaCodecs(
> > +  const std::vector<VideoCodecConfig* >& codecConfigList)
> 
> Align with open paren, or four spaces if you are having to wrap.

Done

> @@ +547,5 @@
> > +  // XXX This will do bandwidth-resolution adaptation as well - bug 877954
> > +
> > +  // Adapt to getUserMedia resolution changes
> > +  // check if we need to reconfigure the sending resolution
> > +  if (mSendingWidth == 0 ||
> 
> Please don't double use 0 like this. Add an explicit state field.

Test removed (see below).

> @@ +622,5 @@
> > +  // enforce even width/height (paranoia)
> > +  MOZ_ASSERT(!(width & 1));
> > +  MOZ_ASSERT(!(height & 1));
> > +
> > +  SelectSendResolution(width, height, capture_time);
> 
> Do we really want to be sending video if we can't set the resolution?

Changed to bool, kick out on error.

> 
> @@ +772,5 @@
> >  {
> >    cinst.plType  = codecInfo->mType;
> > +  // width/height are just defaults that will be overridden on the first frame
> > +  cinst.width   = 640;
> > +  cinst.height  = 480;
> 
> I think this introduces a bug.
> 
> Consider the following sequence of events:
> 
> 1. SetSendCodec(). This imposes 640x480.
> 2. Send some packets with, say QCIF. At this point mSendingWidth becomes
> 176, etc.
> 3. SetSendCodec() again. This will reimpose 640x480.

The only way this can happen is ConfigureSendCodec(); made that force mSendingWidth and Height to 0.

> 4. However, since mSendingWidth is unchanged, we don't reset the video size
> and we are now stuck in 640x480.
> 
> I think if you are going to remove height and width here you really need to
> remove it.

cinst is a structure in the GIPS code, so we can't remove it from there.  We can avoid setting it in ConfigureSendCodec(), and only set it later in SelectSendResolution().  Not sure that gains or hurts us.

> 
> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
> @@ +172,5 @@
> >                        mChannel(-1),
> >                        mCapId(-1),
> > +                      mCurSendCodecConfig(nullptr),
> > +                      mSendingWidth(0),
> > +                      mSendingHeight(0)
> 
> Please add some boolean to indicate whether we have set these.

See above.

> 
> @@ +231,5 @@
> >    int mCapId;   // Capturer for this conduit
> >    RecvCodecList    mRecvCodecList;
> >    VideoCodecConfig* mCurSendCodecConfig;
> > +  unsigned short mSendingWidth;
> > +  unsigned short mSendingHeight;
> 
> If you're not going to use int here, please use a concrete type.

They're unsigned short because those are the values that the GIPS structure takes.

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Randell Jesup [:jesup] from comment #24)

> > @@ +231,5 @@
> > >    int mCapId;   // Capturer for this conduit
> > >    RecvCodecList    mRecvCodecList;
> > >    VideoCodecConfig* mCurSendCodecConfig;
> > > +  unsigned short mSendingWidth;
> > > +  unsigned short mSendingHeight;
> > 
> > If you're not going to use int here, please use a concrete type.
> 
> They're unsigned short because those are the values that the GIPS structure
> takes.

That said, I'm open to converting them if there's an argument it's beneficial or will reduce the chance of our having maintenaince problems (likely to uint32_t - it would isolate us a little from future hypothetical API changes, though I really doubt they'd do so in a way that would break us here).

Comment 26

[Eric Rescorla (:ekr)]

Comment on attachment 768226 [details] [diff] [review]
set codec resolution based on input mediastream resolution

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

::: media/webrtc/signaling/src/media-conduit/CodecConfig.h
@@ +27,5 @@
>    int mChannels;
>    int mRate;
>  
> +  /* When we have resolution negotiation information (RFC 6236)
> +   * it will be stored here.

Shouldn't this comment be associated with VideoCodecConfig?

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +543,5 @@
> +// resolution to the getUserMedia source
> +bool
> +WebrtcVideoConduit::SelectSendResolution(unsigned short width,
> +                                         unsigned short height,
> +                                         uint64_t capture_time)

This argument is unused

@@ +567,5 @@
> +        {
> +          CSFLogDebug(logTag, "%s: Encoder resolution changed to %ux%u",
> +                      __FUNCTION__, width, height);
> +        } else {
> +          CSFLogError(logTag, "%s: SetSendCodec(%ux%u) failed, err %d",

Please have the early return part of the code first and then you will not need nested else, both here and below.

i.e.,

GetSendCodec()
if (failed)  {
  log;
  return;
} 

SetSendCodec();
if (failed)  {
  log;
  return;
} 



If you also revert to the previous semantics of setting the members unconditionally, then you can do that by having the mSendingWidth and mSendingHeight set above.

@@ +576,5 @@
> +    } else {
> +      CSFLogError(logTag, "%s: GetSendCodec failed, err %d", __FUNCTION__, err);
> +      return false;
> +    }
> +    mSendingWidth = width;

Didn't the previous version of this code always set these so you didn't go back through the loop if these fxns failed? Why the change? It's not like this disables the conduit if it fails.

@@ +577,5 @@
> +      CSFLogError(logTag, "%s: GetSendCodec failed, err %d", __FUNCTION__, err);
> +      return false;
> +    }
> +    mSendingWidth = width;
> +    mSendingWidth = height;

Cut-and-paste error here. You probably mean mSendingWidth.

@@ +780,5 @@
>  {
>    cinst.plType  = codecInfo->mType;
> +  // width/height are just defaults that will be overridden on the first frame
> +  cinst.width   = 640;
> +  cinst.height  = 480;

Why are you setting these at all? They already have values, since they came from GIPS in the first place. You can see that from the fact that we're not changing the *name*. Please just leave the defaults, since you're not setting them to anything useful here.

Comment 27

[Eric Rescorla (:ekr)]

Or, rather, mSendingHeight.(In reply to Eric Rescorla (:ekr) from comment #26)

> > +    mSendingWidth = width;
> > +    mSendingWidth = height;
> 
> Cut-and-paste error here. You probably mean mSendingWidth.

mSendingHeight, rather.

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Eric Rescorla (:ekr) from comment #26)
> Comment on attachment 768226 [details] [diff] [review]
> set codec resolution based on input mediastream resolution
> 
> Review of attachment 768226 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/media-conduit/CodecConfig.h
> @@ +27,5 @@
> >    int mChannels;
> >    int mRate;
> >  
> > +  /* When we have resolution negotiation information (RFC 6236)
> > +   * it will be stored here.
> 
> Shouldn't this comment be associated with VideoCodecConfig?

Yup, fixed.

> 
> ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> @@ +543,5 @@
> > +// resolution to the getUserMedia source
> > +bool
> > +WebrtcVideoConduit::SelectSendResolution(unsigned short width,
> > +                                         unsigned short height,
> > +                                         uint64_t capture_time)
> 
> This argument is unused

See the comment; that will likely be used when doing bandwidth-resolution adaptation.
We can remove it until then, however.

> @@ +567,5 @@
> > +        {
> > +          CSFLogDebug(logTag, "%s: Encoder resolution changed to %ux%u",
> > +                      __FUNCTION__, width, height);
> > +        } else {
> > +          CSFLogError(logTag, "%s: SetSendCodec(%ux%u) failed, err %d",
> 
> Please have the early return part of the code first and then you will not
> need nested else, both here and below.

done.

> @@ +576,5 @@
> > +    } else {
> > +      CSFLogError(logTag, "%s: GetSendCodec failed, err %d", __FUNCTION__, err);
> > +      return false;
> > +    }
> > +    mSendingWidth = width;
> 
> Didn't the previous version of this code always set these so you didn't go
> back through the loop if these fxns failed? Why the change? It's not like
> this disables the conduit if it fails.

Restored that, with comment.

> @@ +577,5 @@
> > +      CSFLogError(logTag, "%s: GetSendCodec failed, err %d", __FUNCTION__, err);
> > +      return false;
> > +    }
> > +    mSendingWidth = width;
> > +    mSendingWidth = height;
> 
> Cut-and-paste error here. You probably mean mSendingWidth.

Good catch!

> @@ +780,5 @@
> >  {
> >    cinst.plType  = codecInfo->mType;
> > +  // width/height are just defaults that will be overridden on the first frame
> > +  cinst.width   = 640;
> > +  cinst.height  = 480;
> 
> Why are you setting these at all? They already have values, since they came
> from GIPS in the first place. You can see that from the fact that we're not
> changing the *name*. Please just leave the defaults, since you're not
> setting them to anything useful here.

Ok.

Comment 29

[Eric Rescorla (:ekr)]

(In reply to Randell Jesup [:jesup] from comment #28)
> (In reply to Eric Rescorla (:ekr) from comment #26)
> > Comment on attachment 768226 [details] [diff] [review]
> > set codec resolution based on input mediastream resolution
> > 
> > Review of attachment 768226 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: media/webrtc/signaling/src/media-conduit/CodecConfig.h
> > @@ +27,5 @@
> > >    int mChannels;
> > >    int mRate;
> > >  
> > > +  /* When we have resolution negotiation information (RFC 6236)
> > > +   * it will be stored here.
> > 
> > Shouldn't this comment be associated with VideoCodecConfig?
> 
> Yup, fixed.
> 
> > 
> > ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
> > @@ +543,5 @@
> > > +// resolution to the getUserMedia source
> > > +bool
> > > +WebrtcVideoConduit::SelectSendResolution(unsigned short width,
> > > +                                         unsigned short height,
> > > +                                         uint64_t capture_time)
> > 
> > This argument is unused
> 
> See the comment; that will likely be used when doing bandwidth-resolution
> adaptation.
> We can remove it until then, however.

I'm indifferent between remove and marking it unused, but I'm trying to avoid compiler
warnings.


> > @@ +567,5 @@
> > > +        {
> > > +          CSFLogDebug(logTag, "%s: Encoder resolution changed to %ux%u",
> > > +                      __FUNCTION__, width, height);
> > > +        } else {
> > > +          CSFLogError(logTag, "%s: SetSendCodec(%ux%u) failed, err %d",
> > 
> > Please have the early return part of the code first and then you will not
> > need nested else, both here and below.
> 
> done.
> 
> > @@ +576,5 @@
> > > +    } else {
> > > +      CSFLogError(logTag, "%s: GetSendCodec failed, err %d", __FUNCTION__, err);
> > > +      return false;
> > > +    }
> > > +    mSendingWidth = width;
> > 
> > Didn't the previous version of this code always set these so you didn't go
> > back through the loop if these fxns failed? Why the change? It's not like
> > this disables the conduit if it fails.
> 
> Restored that, with comment.
> 
> > @@ +577,5 @@
> > > +      CSFLogError(logTag, "%s: GetSendCodec failed, err %d", __FUNCTION__, err);
> > > +      return false;
> > > +    }
> > > +    mSendingWidth = width;
> > > +    mSendingWidth = height;
> > 
> > Cut-and-paste error here. You probably mean mSendingWidth.
> 
> Good catch!
> 
> > @@ +780,5 @@
> > >  {
> > >    cinst.plType  = codecInfo->mType;
> > > +  // width/height are just defaults that will be overridden on the first frame
> > > +  cinst.width   = 640;
> > > +  cinst.height  = 480;
> > 
> > Why are you setting these at all? They already have values, since they came
> > from GIPS in the first place. You can see that from the fact that we're not
> > changing the *name*. Please just leave the defaults, since you're not
> > setting them to anything useful here.
> 
> Ok.

Comment 30

[Randell Jesup [:jesup] (needinfo me)]

Attachment: set codec resolution based on input mediastream resolution

Comment 31

[Eric Rescorla (:ekr)]

Comment on attachment 768320 [details] [diff] [review]
set codec resolution based on input mediastream resolution

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

lgtm with comments below. The second one seems optional

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +550,5 @@
> +  // Adapt to getUserMedia resolution changes
> +  // check if we need to reconfigure the sending resolution
> +  if (mSendingWidth != width || mSendingHeight != height)
> +  {
> +    // Get current vie codec.

This should live closer to use, no?

@@ +626,5 @@
>    }
>  
> +  // enforce even width/height (paranoia)
> +  MOZ_ASSERT(!(width & 1));
> +  MOZ_ASSERT(!(height & 1));

should this have a check in release mode?

Comment 32

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/64d9bac71cc9

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/64d9bac71cc9

Comment 34

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 768320 [details] [diff] [review]
set codec resolution based on input mediastream resolution

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A

User impact if declined: Major video quality issues on non-high-end phones

Testing completed (on m-c, etc.): On m-c for a while.  No problems.

Risk to taking this patch (and alternatives if risky): Very low risk

String or IDL/UUID changes made by this patch: none

Comment 35

[bhavana bajaj [:bajaj]]

Comment on attachment 768320 [details] [diff] [review]
set codec resolution based on input mediastream resolution

low risk patch helps improve video quality on low end devices.

Comment 36

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ea5bde96c04c

Comment 37

[Mihai Morar, (:MihaiMorar)]

Randell, Jason: does this issue affects Firefox Desktop too or it is related only for Android?

Comment 38

[Randell Jesup [:jesup] (needinfo me)]

It affects all of them; to see it currently on Desktop you have to modify the width/height prefs:
media.navigator.video.default_width/height

Comment 39

[Mihai Morar, (:MihaiMorar)]

Attachment: PrintScreen - Blurry display.jpg

I've done following verification using FF 25b8:

I used https://apprtc.webrtc.org for 1:1 calls (using different machines from same and different connections) on Ubuntu 13.04, Mac OSX, Windows 7x64 and Windows XP.

Windows 7 x64 - Windows XP
Windows 7 x64 - Ubuntu 13.04
Windows 7 x64 - Mac OSX

For all the above connections I got same results:

1. Works as expected using:
media.navigator.video.height 480(default)
media.navigator.video.width 640 (default)

2. Works as expected using:
media.navigator.video.height 600
media.navigator.video.width 800

3. The display is blurry (a lot) but the audio works fine without any delay or interruptions:
media.navigator.video.height 180
media.navigator.video.width 260

Please see the attachment for Case 3 mentioned above.

Comment 40

[u279076]

Randell, is comment 39 sufficient to call this verified fixed?

Comment 41

[Randell Jesup [:jesup] (needinfo me)]

yes, thanks