1706917 - Verify codec configuration in conduits and codec implementations

Status: NEW

(Core :: WebRTC, task, P2)

Description

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Structurally find all supported codec (mainly video) parameters supported upstream, and:

• Make sure they are configured correctly by conduits, and
• Make sure they are supported by our own codec implementations

Comment 1

[Byron Campen [:bwc]]

So I'm assuming that the focus is on the stuff here:

https://searchfox.org/mozilla-central/rev/d21e359bd26dd0a7ba216472184d6fed8f0afd48/third_party/libwebrtc/api/video_codecs/video_codec.h

and the stuff here:

https://searchfox.org/mozilla-central/rev/d21e359bd26dd0a7ba216472184d6fed8f0afd48/third_party/libwebrtc/api/video_codecs/video_encoder_config.h

Comment 2

[Byron Campen [:bwc]]

VideoCodecVP8::complexity -> Set to 0 (kComplexityNormal) here, which we then use here. Then, this is mapped to MediaDataEncoder::VPXSpecific::Complexity and passed to MediaDataEncoder from here.

VideoCodecVP8::numberOfTemporalLayers -> Set to 1 here, which we then use here. Passed to MediaDataEncoder here.

VideoCodecVP8::denoisingOn -> Set to true here, which we then use here. However, we override this, setting it to false iff we're doing screencasting. We ignore the value returned by WebrtcVideoConduit::Denoising here; if |denoising| is true, |codec_default_denoising| is false, causing us to use true here, whereas if |denoising| false, |codec_default_denoising| is true, also causing us to use true in the same place. I do not see anything else using WebrtcVideoConduit::Denoising in the tree. There is either a bug here, or we can eliminate WebrtcVideoConduit::Denoising. Passed to MediaDataEncoder here.

VideoCodecVP8::automaticResizeOn -> Set to false here, which we then use here, however we override the value based on this code, setting it to true for non-screencast unicast, and false otherwise. Passed to MediaDataEncoder here.

VideoCodecVP8::frameDroppingOn -> Set to true here, which we then use here, but we override, setting it to true iff we aren't screencasting. Passed to MediaDataEncoder here.

VideoCodecVP8::keyFrameInterval -> Set to 3000 here, which we then use here. Passed to MediaDataEncoder here.

Comment 3

[Byron Campen [:bwc]]

^ Is this the kind of analysis we're looking for here?

Comment 4

[Byron Campen [:bwc]]

VideoCodecVP9::complexity -> Set to 0 (kComplexityNormal) here, which we then use here. Then, this is mapped to MediaDataEncoder::VPXSpecific::Complexity and passed to MediaDataEncoder from here.

VideoCodecVP9::numberOfTemporalLayers -> Set to 1 here, which we then use here. Passed to MediaDataEncoder here.

VideoCodecVP9::denoisingOn -> Set to true here, which we then use here. However, we override this if WebrtcVideoConduit::Denoising returns false or if screencasting. Passed to MediaDataEncoder here.

VideoCodecVP9::frameDroppingOn -> Set to true here, and also by us here, even when doing screencast. Passed to MediaDataEncoder here.

VideoCodecVP9::keyFrameInterval -> Set to 3000 here, which we then use here. Passed to MediaDataEncoder here.

VideoCodecVP9::adaptiveQpMode -> Set to true here, which we then use here. Passed to MediaDataEncoder here.

VideoCodecVP9::automaticResizeOn -> Set to true here, which we then use here. We do not set this to false in the screencapture case like we do for VP8. Passed to MediaDataEncoder here.

VideoCodecVP9::numberOfSpatialLayers -> Set to 2 in the screencast case, or the return of WebrtcVideoConduit::SpatialLayers otherwise. The return of WebrtcVideoConduit::SpatialLayers is either 1, or equal to the value of the 'media.peerconnection.video.svc.spatial' pref, although it is unclear whether values other than 1 will work. Passed to MediaDataEncoder here.

VideoCodecVP9::flexibleMode -> Set to false here, which we then use here. Passed to MediaDataEncoder at encoder creation time here, and then again at encode init time here? Weird that we're setting this in two different places. Someone who knows more about this should give this a look.

VideoCodecVP9::interLayerPred -> Set to On here, which we then use here. Not passed on to MediaDataEncoder at any point. Query: Are we doing the right thing here?

Comment 5

[Byron Campen [:bwc]]

VideoCodecH264::frameDroppingOn -> We set this to true iff we are not screencasting. I don't see any code actually using this field, however. Query: Are we doing the right thing here?

VideoCodecH264::keyFrameInterval -> Set to 3000 in GetDefaultH264Settings, which we call in ConfigureVideoEncoderSettings. The only read I see is in MultiplexEncoderAdapter::InitEncode, and that only has an effect in MultiplexEncoderAdapter::Encode. I do not know if this is in the path for any of our H264 encoding. Further verification will be needed.

VideoCodecH264::scaleDownBy -> Set to 0 with a memset, and never read/written again. This is a double, so memset 0 is a little sketchy maybe, but it isn't being used anyway.

VideoCodecH264::numberOfTemporalLayers -> Set to 1 in VideoEncoder::GetDefaultH264Settings, which we call in ConfigureVideoEncoderSettings. Not read in any mozilla specific code, and only appears to be used in libwebrtc code to disable the frame dropper in some cases. Seems to have no real effect in our build.

VideoCodecH264::packetizationMode -> Set in ConfigureVideoEncoderSettings based on fmtp negotiation. Used to set CodecSpecificInfoH264::packetization_mode inside WebrtcGmpVideoEncoder::InitEncode and InitCodecSpecficInfo.

Comment 6

[Nico Grunbaum [:ng, @chew:mozilla.org]]

(In reply to Byron Campen [:bwc] from comment #3)

^ Is this the kind of analysis we're looking for here?

Yes, this is exactly what we are looking for.

Comment 7

[Byron Campen [:bwc]]

VideoCodec::codecType -> Surely this is being set.

VideoCodec::width and VideoCodec::height -> These aren't configured, but are set based on frames.

VideoCodec::startBitrate -> This is set:

• To 300 in CreateDecoderVideoCodec (this function is also defined in video_receive_stream2.cc, and both of these files are being compiled, presumably on different platforms)

• Based on VideoStreamEncoder::encoder_target_bitrate_bps_ in VideoStreamEncoder::ReconfigureEncoder. encoder_target_bitrate_bps_ is set in:

  • VideoStreamEncoder::SetStartBitrate, which is called from VideoSendStreamImpl::VideoSendStreamImpl, based on what the bitrate allocator says. This is highly dependent on network capacity, but early on will fall back to BitrateAllocator::last_non_zero_bitrate_bps_, which is set in BitrateAllocator::UpdateStartRate, which is called by Call::OnStartRateUpdate, which is called by RtpTransportControllerSend::RegisterTargetTransferRateObserver, which passes the TargetRateConstraints::starting_rate of RtpTransportControllerSend::initial_config_, which is set in RtpTransportControllerSend::RtpTransportControllerSend, which is called by Call::Create, which we call without any bitrate settings in WebrtcCallWrapper::Create.
  • VideoStreamEncoder::OnBitrateUpdated, which is in turn called by VideoSendStreamImpl::OnBitrateUpdated, which passes a param that is derived from RtpVideoSenderInterface::GetPayloadBitrateBps, which is based on RtpVideoSender::encoder_target_rate_bps_, which is set in RtpVideoSender::OnBitrateUpdated, which is in turn called by VideoSendStreamImpl::OnBitrateUpdated, which is in turn called by BitrateAllocator::OnNetworkEstimateChanged (or BitrateAllocator::AddObserver based on the latest call to BitrateAllocator::OnNetworkEstimateChanged), which in turn is called from Call::OnTargetTransferRate, which is in turn called from RtpTransportControllerSend::UpdateControlState, which is called periodically and passes the output of CongestionControlHandler::GetUpdate, which returns changes to CongestionControlHandler::last_incoming_, which is updated in CongestionControlHandler::SetTargetRate, which is called from RtpTransportControllerSend::PostUpdates when called with a NetworkControlUpdate with a target_rate, which is manufactured either by:
    • GoogCcNetworkController::GetNetworkState, which is based on GoogCcNetworkController::last_pushback_target_rate_, which is set in GoogCcNetworkController::GoogCcNetworkController, which is called by GoogCcNetworkControllerFactory::Create, which is called by RtpTransportControllerSend::MaybeCreateControllers, based on the value of RtpTransportControllerSend::initial_config_, which then ultimately ends up in WebrtcCallWrapper::Create again, without any bitrate setting, or...
    • GoogCcNetworkController::MaybeTriggerOnNetworkChanged, which uses the value of SendSideBandwidthEstimation::target_rate, which is based on SendSideBandwidthEstimation::current_target_, which is set in SendSideBandwidthEstimation::UpdateTargetBitrate, which is called by:
      • SendSideBandwidthEstimation::SetSendBitrate, which is called from:
        • GoogCcNetworkController::OnTransportPacketsFeedback, which is based on bandwidth estimation logic that is complex, and I am running out of patience for this, or
        • SendSideBandwidthEstimation::SetBitrates, which is called from GoogCcNetworkController::ResetConstraints, which is called from:
          • GoogCcNetworkController::OnNetworkRouteChange, which is called by RtpTransportControllerSend::OnNetworkRouteChanged based on the value of RtpBitrateConfigurator::bitrate_config_, which is set by either:
            • RtpBitrateConfigurator::RtpBitrateConfigurator, which is called by
              RtpTransportControllerSend::RtpTransportControllerSend, which ultimately traces back to WebrtcCallWrapper::Create again, or
            • RtpBitrateConfigurator::UpdateConstraints, which is called by RtpBitrateConfigurator::UpdateWithClientPreferences, which is called by RtpTransportControllerSend::SetClientBitratePreferences, which is called by Call::SetClientBitratePreferences, which is finally called in a meaningful way by WebrtcVideoConduit::OnControlConfigChange, based on WebrtcVideoConduit::mMinBitrateEstimate, which ultimately comes from a pref in TransceiverImpl::InitVideo (but not, as it turns out, mStartBitrate! That is actually ignored!)
          • GoogCcNetworkController::OnProcessInterval, or
          • GoogCcNetworkController::OnTargetRateConstraints
      • SendSideBandwidthEstimation::UpdateEstimate, which adjusts the previous value of SendSideBandwidthEstimation::current_target_ based on network conditions.

• In SimulcastEncoderAdapter::PopulateStreamCodec based on the |start_bitrate_kbps| parameter, which is determined in SimulcastEncoderAdapter::InitEncode based on what the rate allocator says to use, based on the value of the starting |VideoCodec::startBitrate| (so, ultimately this branch traces back to one of the two others above).

Our conduit code uses a totally different entry point for starting bitrate that does not seem to have anything to do with VideoCodec::startBitrate, and there are a lot of hops:

• Pref-based config in TransceiverImpl::InitVideo which ends up in
• WebrtcVideoConduit::mStartBitrate, which is used to create the
• VideoStreamFactory, which uses the value in
• VideoStreamFactory::CreateEncoderStreams in the call it makes to SelectBitrates, which uses it to set
• VideoStream::target_bitrate_bps, which is then used to set
• SpatialLayer::targetBitrate, which is used in calculations (in SvcRateAllocator::GetAllocationScreenSharing and SimulcastRateAllocator::DistributeAllocationToSimulcastLayers) to make calls to
• VideoBitrateAllocation::SetBitrate to update VideoBitrateAllocation::sum_, which is then used to set
• VideoEncoder::RateControlParameters::bandwidth_allocation in a few places (VideoEncoder::RateControlParameters c'tor, SimulcastEncoderAdapter::SetRates, VideoStreamEncoder::UpdateBitrateAllocationAndNotifyObserver), as well as vpx_codec_enc_cfg::rc_target_bitrate in VP9EncoderImpl::SetSvcRates, which are then used in libwebrtc's encoder code.

There really do seem to be at least two distinct implementations of starting bitrate in here, and I am not sure how (or if) these things are reconciled. All I have now is a headache.

Comment 8

[Byron Campen [:bwc]]

As for how VideoCodec::startBitrate is used by our code, we pass it to:

• WebrtcGmpVideoEncoder::InitEncode, but I do not see anyplace where it is actually used, and
• WebrtcMediaDataEncoder::SetupConfig, which is passed to BitrateAdjuster::SetTargetBitrateBps to update BitrateAdjuster::target_bitrate_bps_ (and maybe BitrateAdjuster::adjusted_bitrate_bps_ and BitrateAdjuster::last_adjusted_target_bitrate_bps_), which is then used in WebrtcMediaDataEncoder::CreateEncoder, which then ends up in either:
  • MediaDataEncoder::H264Config, which is used to configure either an AndroidDataEncoder, an AppleVTEncoder, or a WMFMediaDataEncoder
  • MediaDataEncoder::VP8Config, which is used to configure an AndroidDataEncoder or a WMFMediaDataEncoder
  • MediaDataEncoder::VP9Config, which is used to configure an AndroidDataEncoder or a WMFMediaDataEncoder

Comment 9

[Byron Campen [:bwc]]

VideoCodec::maxBitrate -> This is set:

• In SimulcastEncoderAdapter::PopulateStreamCodec, based on SpatialLayer::maxBitrate, which is set in:

  • For VP9, ConfigureSvcNormalVideo, based on the resolution in SpatialLayer (but not SpatialLayer::maxFramerate? Weird.)
  • For VP9, ConfigureSvcScreenSharing, based on some static config
  • VideoCodecInitializer::VideoEncoderConfigToVideoCodec, based on VideoStream::max_bitrate_bps, which is set by:
    • SelectBitrates inside our conduit code, based on our config.
    • VideoStreamEncoder::ReconfigureEncoder based on:
      • In the VP9 case, SvcRateAllocator::GetMaxBitrate, which is in turn based either on a SpatialLayer::maxBitrate or a VideoCodec::maxBitrate (lots of loopy dependencies here...)
      • VideoStreamEncoder::encoder_bitrate_limits_, which is set by VideoStreamEncoder::ReconfigureEncoder based on the return of VideoEncoder::EncoderInfo::GetEncoderBitrateLimitsForResolution, which is based on VideoEncoder::EncoderInfo::resolution_bitrate_limits, which is set by LibvpxVp8Encoder::GetEncoderInfo, which is based on LibvpxVp8Encoder::resolution_bitrate_limits_, which is set by LibvpxVp8Encoder::LibvpxVp8Encoder, based on VP8Encoder::resolution_bitrate_limits, which does not appear to be set anywhere

• In VideoCodecInitializer::VideoEncoderConfigToVideoCodec, based on either resolution or VideoStream::max_bitrate_bps (same as above)

• To 300 in CreateDecoderVideoCodec (also in video_receive_stream2.cc)

• To one bit per pixel (and at least startBitrate) in VideoStreamEncoder::ReconfigureEncoder

Comment 10

[Byron Campen [:bwc]]

The way that we are trying to set max bitrate is:

• VideoStreamFactory::CreateEncoderStreams, which uses our config in SelectBitrates to set VideoStream::max_bitrate_bps, which is described in the above comment.
• WebrtcVideoConduit::OnControlConfigChange to set WebrtcVideoConduit::mEncoderConfig, which is then used in:
  • WebrtcVideoConduit::OnControlConfigChange, which calls VideoSendStream::ReconfigureVideoEncoder, which then calls VideoStreamEncoder::ConfigureEncoder, which sets VideoStreamEncoder::encoder_config_, which is used by:
    • VideoStreamEncoder::ReconfigureEncoder, which passes it to VideoStreamFactory::CreateEncoderStreams, which ignores the max bitrate setting entirely.
    • VideoStreamEncoder::ReconfigureEncoder, which passes it to:
      • VideoCodecInitializer::SetupCodec, which then calls VideoCodecInitializer::VideoEncoderConfigToVideoCodec, which also ignores the max bitrate setting entirely.
      • SendStatisticsProxy::OnEncoderReconfigured, which also ignores the max bitrate setting entirely.
    • VideoStreamEncoder::OnEncoderSettingsChanged, which then puts a copy inside EncoderSettings::encoder_config_, which nothing ever inspects (besides the codec_type and content_type).
  • WebrtcVideoConduit::CreateSendStream, which then passes it to Call::CreateVideoSendStream, which then passes it to VideoSendStream::VideoSendStream, which then passes the bitrate field to VideoSendStreamImpl::VideoSendStreamImpl, which is used to init VideoSendStreamImpl::encoder_max_bitrate_bps_, which is then used by VideoSendStreamImpl::OnBitrateUpdated to set the target bitrate, which is maybe actually what we want (does libwebrtc attempt to reach, but not exceed, this target)?

Comment 11

[Byron Campen [:bwc]]

VideoCodec::minBitrate -> We do not attempt to configure this in our conduit code, since that is not something that we negotiate, or configure from JS. We do use it in WebrtcGmpVideoEncoder::InitEncode and WebrtcMediaDataEncoder::SetupConfig

VideoCodec::maxFramerate -> We do not set this in our conduit code, although we do have a field (EncodingConstraints::maxFps) that we set based on SDP negotiation. This ends up being used in SelectSendFrameRate, which is called by WebrtcVideoConduit::OnControlConfigChange and WebrtcVideoConduit::SelectSendResolution to set WebrtcVideoConduit::mSendingFramerate, which is used to set VideoStreamFactory::mSendingFramerate, which is used by VideoStreamFactory::CreateEncoderStreams to set the VideoStream::max_framerate in the streams it creates. VideoStream::max_framerate is in turn used:

• By VideoCodecInitializer::VideoEncoderConfigToVideoCodec to init both SpatialLayer::maxFramerate and VideoCodec::maxFramerate
• By VideoStreamEncoder::ReconfigureEncoder to call VideoSourceSinkController::SetFrameRateUpperLimit

maxFramerate is used in WebrtcGmpVideoEncoder::InitEncode, WebrtcMediaDataEncoder::SetupConfig, and WebrtcMediaDataEncoder::CreateEncoder

Comment 12

[Byron Campen [:bwc]]

VideoCodec::active -> Seems to do what the comment says; the bitrate allocator looks at this and allocates 0 if false, and nothing else pays any attention to it. Set either based on:

• SpatialLayer::active by SimulcastEncoderAdapter::PopulateStreamCodec or
• VideoStream::active by VideoCodecInitializer::VideoEncoderConfigToVideoCodec

Comment 13

[Byron Campen [:bwc]]

VideoCodec::qpMax

Substantial reads:

• LibvpxVp8Encoder::InitEncode into LibvpxVp8Encoder::qp_max_, leading to
  LibvpxVp8Encoder::SetRates or LibvpxVp8Encoder::InitEncode into vpx_codec_enc_cfg::rc_max_quantizer, which I doubt makes it out of libvpx into our platform code...

Writes:

• SimulcastEncoderAdapter::InitEncode to kDefaultMaxQp
• SimulcastEncoderAdapter::PopulateStreamCodec based on SpatialLayer::qpMax, which is in turn set by VideoCodecInitializer::VideoEncoderConfigToVideoCodec based on VideoStream::max_qp, which is in turn set by (our code) VideoStreamFactory::CreateEncoderStreams to kQpMax.
• SimulcastEncoderAdapter::PopulateStreamCodec to SimulcastEncoderAdapter::experimental_boosted_screenshare_qp_ in some cases, which I'm guessing is not being used? Query: Should we be using this, or doing something similar?
• SimulcastEncoderAdapter::PopulateStreamCodec to kLowestResMaxQp for the lowest resolution simulcast stream, if a field trial thing is enabled (which it probably isn't). Query: Should we be doing this?
• VideoCodecInitializer::VideoEncoderConfigToVideoCodec to the largest VideoStream::max_qp among the simulcast streams.

I do not see this VideoCodec::qpMax/SpatialLayer::qpMax/VideoStream::max_qp triad making it to our platform encoders in any way... Query: Do our platform encoders support this at all?

Comment 14

[Byron Campen [:bwc]]

VideoCodec::numberOfSimulcastStreams

VideoCodecInitializer::VideoEncoderConfigToVideoCodec sets this to the size of an array of VideoStreams, passed by VideoCodecInitializer::SetupCodec, passed by VideoStreamEncoder::ReconfigureEncoder, which creates the array with (our code) VideoStreamFactory::CreateEncoderStreams, based on the passed VideoEncoderConfig

Comment 15

[Byron Campen [:bwc]]

VideoCodec::mode

Set in VideoCodecInitializer::VideoEncoderConfigToVideoCodec (based on VideoEncoderConfig::content_type), which is called by VideoCodecInitializer::SetupCodec, which is called by VideoStreamEncoder::ReconfigureEncoder with VideoStreamEncoder::encoder_config_, which is updated by VideoStreamEncoder::ConfigureEncoder, which is called by VideoSendStream::ReconfigureVideoEncoder, which is called by WebrtcVideoConduit::OnControlConfigChange, with a copy of WebrtcVideoConduit::mEncoderConfig, whose content_type is set in WebrtcVideoConduit::OnControlConfigChange

Used in WebrtcGmpVideoEncoder::InitEncode

Comment 16

[Byron Campen [:bwc]]

VideoCodec::expect_encode_from_texture

I only see this set in one place, and never read by anything. The Chromium Code Search turns up only one use, in an EXPECT_EQ in a unit-test:

https://source.chromium.org/search?q=expect_encode_from_texture&sq=&ss=chromium

Comment 17

[Byron Campen [:bwc]]

VideoCodec::buffer_pool_size

We don't seem to be using this.

Comment 18

[Byron Campen [:bwc]]

VideoCodec::timing_frame_thresholds

Struct initted by VideoCodecInitializer::VideoEncoderConfigToVideoCodec to kDefaultTimingFramesDelayMs,kDefaultOutlierFrameSizePercent.

Used by FrameEncodeMetadataWriter::FillTimingInfo and nothing else.

Comment 19

[Byron Campen [:bwc]]

VideoCodec::legacy_conference_mode

We aren't using this.

Comment 20

[Byron Campen [:bwc]]

VideoStream::width and VideoStream::height

Set by VideoStreamFactory::CreateEncoderStreams, based on VideoStreamFactory::mCodecConfig, which holds information on the number of simulcast streams and the scale-down value for each, and which is set when the VideoStreamFactory is constructed by WebrtcVideoConduit::OnControlConfigChange. (Note: This code uses the passed parameter |config| to determine the stream count which is used to index into mCodecConfig.mEncodings, and not the size of mCodecConfig.mEncodings. This strikes me as fragile.) This scale-down information is passed to VideoAdapter::OnScaleResolutionBy, and then the width/height is adjusted with a call to VideoAdapter::AdaptFrameResolution.

Comment 21

[Byron Campen [:bwc]]

VideoStream::max_framerate

Set by VideoStreamFactory::CreateEncoderStreams, based on VideoStreamFactory::mSendingFramerate (updated by VideoStreamFactory::SetSendingFramerate).

Comment 22

[Byron Campen [:bwc]]

VideoStream::min_bitrate_bps, VideoStream::target_bitrate_bps, and VideoStream::max_bitrate_bps

Set in SelectBitrates which is called by VideoStreamFactory::CreateEncoderStreams, based on:

• frame size, which is fed into VideoStreamFactory::GetLimitsFor (which uses this table)
• VideoStreamFactory::mMinBitrate (ultimately based on a combination of the "media.peerconnection.video.min_bitrate" pref and kViEMinCodecBitrate_bps with a current default of 0)
• VideoStreamFactory::mStartBitrate (which is ultimately based on the "media.peerconnection.video.start_bitrate" pref, subject to min/max constraints, also currently with a default of 0)
• The EncodingConstraints::maxBr of the stream in question (which is based on constraints from both JS and SDP negotiation)
• VideoStreamFactory::mPrefMaxBitrate (which is ultimately set based on the "media.peerconnection.video.max_bitrate" pref which currently defaults to 0. Query: SelectBitrates ignores this parameter. Is this intended?)
• VideoStreamFactory::mNegotiatedMaxBitrate (which ultimately comes from b=TIAS in SDP if present, which is almost never the case I think)

Comment 23

[Byron Campen [:bwc]]

VideoStream::scale_resolution_down_by

I don't see anything setting this besides things like bulk adjustment factors, ceilings, and floors. The comment seems to indicate that setting this does not cause the dimensions to be further downscaled, but it is possible that failing to set this field will confuse things further down the line, possibly in subtle and hard to diagnose ways. Chromium does seem to set this.

Query: What bugs might this be causing?

If this is causing a bug, it would be related to AlignmentAdjuster::GetAlignmentAndMaybeAdjustScaleFactors, the only user of this field. This is called from VideoStreamEncoder::ReconfigureEncoder right before the call to VideoStreamFactory::CreateEncoderStreams. So, we might be getting our alignment calculations for simulcast wrong. I am not 100% sure what the consequences are of this, but this comment hints that we could end up with bad cropping or aspect ratio.

Comment 24

[Byron Campen [:bwc]]

VideoStream::max_qp

VideoStreamFactory::CreateEncoderStreams sets this to kQpMax. Pretty straightforward.

Comment 25

[Byron Campen [:bwc]]

VideoStream::num_temporal_layers

VideoStreamFactory::CreateEncoderStreams sets this to 1 for simulcast screenshare, and 2 for regular simulcast, and not at all in non-simulcast.

Comment 26

[Byron Campen [:bwc]]

VideoStream::bitrate_priority

VideoStreamFactory::CreateEncoderStreams sets this based on VideoEncoderConfig::bitrate_priority within VideoStreamEncoder::encoder_config_ which is set as described in comment 15. VideoEncoderConfig::bitrate_priority is only ever set in WebrtcVideoConduit::OnControlConfigChange, to 1, although it looks like there's a TODO there to connect it to the webrtc-priority w3c work.

Comment 27

[Byron Campen [:bwc]]

VideoStream::active

We are not using this, but will probably start doing so as part of bug 1676855.

Comment 28

[Byron Campen [:bwc]]

VideoEncoderConfig::codec_type

WebrtcVideoConduit::OnControlConfigChange sets this on WebrtcVideoConduit::mEncoderConfig based on the VideoCodecConfig::mName field of WebrtcVideoConduit::mSendCodec. WebrtcVideoConduit::mEncoderConfig is then subsequently fed into VideoSendStream::ReconfigureVideoEncoder.

Comment 29

[Byron Campen [:bwc]]

VideoEncoderConfig::video_format

Exactly the same story as codec_type; seems to be redundant information, and there is even a TODO for eliminating one of these.

Comment 30

[Byron Campen [:bwc]]

VideoEncoderConfig::video_stream_factory

WebrtcVideoConduit::OnControlConfigChange sets this to a dedicated VideoStreamFactory with additional config bits that are not captured in WebrtcVideoConduit::mEncoderConfig.

Comment 31

[Byron Campen [:bwc]]

VideoEncoderConfig::spatial_layers

Nothing ever sets this in our codebase, and it seems that the same is true in Chromium? Query: Is this just dead code?

Comment 32

[Byron Campen [:bwc]]

VideoEncoderConfig::content_type

WebrtcVideoConduit::OnControlConfigChange sets this on WebrtcVideoConduit::mEncoderConfig based on WebrtcVideoConduit::mControl::mCodecMode. WebrtcVideoConduit::mEncoderConfig is then subsequently fed into VideoSendStream::ReconfigureVideoEncoder.

Comment 33

[Byron Campen [:bwc]]

VideoEncoderConfig::encoder_specific_settings

WebrtcVideoConduit::OnControlConfigChange sets this, based on the return of ConfigureVideoEncoderSettings, which ends up being a wrapper around either a VideoCodecH264 (covered in comment 5), a VideoCodecVP8 (covered in comment 2), or a VideoCodecVP9 (covered in comment 4).

Comment 34

[Byron Campen [:bwc]]

VideoEncoderConfig::min_transmit_bitrate_bps

WebrtcVideoConduit::OnControlConfigChange sets this to WebrtcVideoConduit::mMinBitrate, which is set in the constructor, ultimately based on a combination of the "media.peerconnection.video.min_bitrate" pref and kViEMinCodecBitrate_bps with a current default of 0. We are configuring this same thing on VideoStream::min_bitrate_bps (as described in comment 22), via VideoStreamFactory. Query: Couldn't we simplify this? VideoStreamFactory::CreateEncoderStreams is passed the config object, which has a copy of the same minimum bitrate in it, making VideoStreamFactory::mMinBitrate unnecessary...

Comment 35

[Byron Campen [:bwc]]

VideoEncoderConfig::max_bitrate_bps

WebrtcVideoConduit::OnControlConfigChange sets this to the minimum of:

• 10Mbps
• WebrtcVideoConduit::mPrefMaxBitrate (if set; this comes from the "media.peerconnection.video.max_bitrate" pref which currently defaults to 0)
• WebrtcVideoConduit::mNegotiatedMaxBitrate (if set; this comes from b=TIAS in SDP which is uncommon)
• EncodingConstraints::maxBr for the codec config as a whole (if set; right now this only happens with an h264 max_br fmtp or the "media.navigator.video.h264.max_br" pref)
• EncodingConstraints::maxBr for the first stream if no simulcast is being used (if set; this could happen with JS constraints or rid).

It should be noted that this calculation is similar to the one carried out by SelectBitrates, as described in comment 22, but not quite the same. Query: Should we reconcile these? I don't think we want there to be two different implementations of this basic logic. Given that the config object has a max bitrate field, and the factory has access to that config object, we ought to be performing this calculation in the conduit code, and have the factory copy the field from the config object when creating VideoStreams.

Comment 36

[Byron Campen [:bwc]]

VideoEncoderConfig::bitrate_priority

Set to 1 as described in comment 26.

Comment 37

[Byron Campen [:bwc]]

VideoEncoderConfig::simulcast_layers

I don't see anything in our codebase that puts anything in here... it seems that these are intended to be filled by something before the video_stream_factory gets a crack at the config (which makes it less of a factory, and more of a converter or mutator). AlignmentAdjuster::GetAlignmentAndMaybeAdjustScaleFactors (see the potential alignment problem noted in comment 23) only operates on elements in this array, so to fix that problem, we would need to start using VideoEncoderConfig::simulcast_layers as well as VideoStream::scale_resolution_down_by. Query: What other problems are we going to trip over by using a factory as a factory?

Comment 38

[Byron Campen [:bwc]]

VideoEncoderConfig::number_of_streams

WebrtcVideoConduit::OnControlConfigChange sets this to the number of negotiated encodings.

Comment 39

[Andreas Pehrson [:pehrsons]]

There's also the question if we set codec params properly from signaling, like we do here for opus. If any params for opus were added upstream in the update, we might need to add them. And then check that they are propagated properly (like you've been doing).

There are less sdp params for video I believe, but we should verify this. Especially that those that exist are handled by our own codecs.

Comment 40

[Byron Campen [:bwc]]

SdpAudioFormat::name, SdpAudioFormat::clockrate_hz, SdpAudioFormat::num_channels

WebrtcAudioConduit::CodecConfigToLibwebrtcFormat sets these, based on AudioCodecConfig::mName, AudioCodecConfig::mFreq, and AudioCodecConfig::mChannels respectively. This is in turn called by WebrtcAudioConduit::OnControlConfigChange (as well as WebrtcAudioConduit::CodecConfigToWebRTCCodec, which appears to be unused Query: Do we get rid of this, or did we miss something here?). JsepCodecDescToAudioCodecConfig performs the conversion from JsepCodecDescription to AudioCodecConfig, which carries over JsepCodecDescription::mName, JsepCodecDescription::mClock, and JsepCodecDescription::mChannels (subject to a JsepAudioCodecDescription::mForceMono check). The name/frequency/channels are hard-coded in JsepSessionImpl::SetupDefaultCodecs, while mForceMono is negotiated in JsepAudioCodecDescription::Negotiate for Opus send streams, based on whether a stereo fmtp is present.

Comment 41

[Byron Campen [:bwc]]

SdpAudioFormat::Parameters

This is used in many places in libwebrtc:

• AudioEncoderOpusImpl::SdpToConfig ("useinbandfec", "usedtx", "cbr", "maxaveragebitrate", "minptime", "maxptime")
• AudioDecoderOpus::SdpToConfig ("stereo")
• GetMaxPlaybackRate (in audio_encoder_opus.cc) ("maxplaybackrate")
• GetFrameSizeMs (in audio_encoder_opus.cc) ("ptime")
• GetChannelCount ("stereo")
• AudioDecoderMultiChannelOpusImpl::SdpToConfig ("num_streams", "coupled_streams", "channel_mapping") (Query: We do not support this multi-channel stuff at all, but should we?)
• AudioEncoderMultiChannelOpusImpl::SdpToConfig ("useinbandfec", "usedtx", "cbr", "maxaveragebitrate", "num_streams", "coupled_streams", "channel_mapping") (Query: More multi-channel Opus stuff.)
• GetMaxPlaybackRate (in audio_encoder_multi_channel_opus_impl.cc) ("maxplaybackrate")
• GetFrameSizeMs (in audio_encoder_multi_channel_opus_impl.cc) ("ptime")

"useinbandfec"/kCodecParamUseInbandFec: WebrtcAudioConduit::CodecConfigToLibwebrtcFormat sets this to the value of AudioCodecConfig::mFECEnabled, which JsepCodecDescToAudioCodecConfig sets to JsepAudioCodecDescription::mFECEnabled. For send streams, this is negotiated, and for receive streams, this is set based on the "media.navigator.audio.use_fec" pref (which is true by default). (In other words, the receiver decides whether to use FEC, and right now both Firefox and Chrome do this) Both our sipcc and rust SDP parsers look like they support this fmtp.

"usedtx"/kCodecParamUseDtx: WebrtcAudioConduit::CodecConfigToLibwebrtcFormat sets this to the value of AudioCodecConfig::mDTXEnabled, which JsepCodecDescToAudioCodecConfig sets to JsepAudioCodecDescription::mDTXEnabled, which JsepAudioCodecDescription::Negotiate sets based on fmtp for opus (but only for send). Like FEC, the receiver decides whether DTX will be used on a given stream, but neither Chrome nor Firefox use the usedtx fmtp right now, since the default for opus is to use variable bitrate (DTX allows a dramatic slowdown in packet transmission when there is silence, to save bandwidth, but variable bitrate also saves bandwidth when there is silence). Both our sipcc and rust SDP parsers appear to support this fmtp.

"cbr"/kCodecParamCbr: WebrtcAudioConduit::CodecConfigToLibwebrtcFormat sets this to AudioCodecConfig::mCbrEnabled, which JsepCodecDescToAudioCodecConfig sets to JsepAudioCodecDescription::mCbrEnabled, which JsepAudioCodecDescription::Negotiate sets based on the cbr fmtp for opus (again, only for send). Neither Firefox or Chrome use the cbr fmtp. Both our sipcc and rust SDP parsers appear to support this fmtp.

"maxaveragebitrate"/kCodecParamMaxAverageBitrate: WebrtcAudioConduit::CodecConfigToLibwebrtcFormat sets this to AudioCodecConfig::mMaxAverageBitrate, which JsepCodecDescToAudioCodecConfig sets to JsepAudioCodecDescription::mMaxAverageBitrate, which JsepAudioCodecDescription::Negotiate sets based on the maxaveragebitrate fmtp for opus (again, only for send). Neither Firefox nor Chrome use this fmtp. Both our sipcc and rust SDP parsers appear to support this fmtp.

"minptime"/kCodecParamMinPTime: WebrtcAudioConduit::CodecConfigToLibwebrtcFormat sets this to AudioCodecConfig::mMinFrameSizeMs, which JsepCodecDescToAudioCodecConfig sets to JsepAudioCodecDescription::mMinFrameSizeMs, which JsepAudioCodecDescription::Negotiate sets based on the minptime opus fmtp (again, only for send). Firefox does not use this fmtp, but Chrome does by default (with a value of 10). Our sipcc-based SDP parser does not support it. Our rust-based SDP parser does appear to support minptime. Query: This almost certainly explains some of the fmtp differences we're seeing in the field. Nico?

"maxptime"/kCodecParamMaxPTime: WebrtcAudioConduit::CodecConfigToLibwebrtcFormat sets this to AudioCodecConfig::mMaxFrameSizeMs, which JsepCodecDescToAudioCodecConfig sets to JsepAudioCodecDescription::mMaxFrameSizeMs, which JsepAudioCodecDescription::Negotiate sets based on the maxptime opus fmtp (again, only for send). Neither Firefox nor Chrome use this fmtp. Our sipcc-based SDP parser does not support this fmtp, but our rust SDP parser does. Query: Maybe this might explain some of the fmtp differences we're seeing in the field? Nico?

"stereo"/kCodecParamStereo: WebrtcAudioConduit::CodecConfigToLibwebrtcFormat sets this to true iff the number of channels is 2 (this is duplicative, but whatever), which is set in the manner described in comment 40. Both our sipcc and rust SDP parsers support this fmtp.

"maxplaybackrate"/kCodecParamMaxPlaybackRate: WebrtcAudioConduit::CodecConfigToLibwebrtcFormat sets this to AudioCodecConfig::mMaxPlaybackRate, which JsepCodecDescToAudioCodecConfig sets to JsepAudioCodecDescription, which JsepAudioCodecDescription::Negotiate sets based on the maxplaybackrate opus fmtp (again, only for send). Firefox always sets this to 48000, while Chrome does not emit it. about:webrtc displays the maxplaybackrate=0 for Chrome's SDP, which is a bit concerning. Query: Is there some parse code that is not assigning the correct default of 48000 when the fmtp is not present?

"ptime"/kCodecParamPTime: WebrtcAudioConduit::CodecConfigToLibwebrtcFormat sets this to AudioCodecConfig::mFrameSizeMs, which JsepCodecDescToAudioCodecConfig sets to JsepAudioCodecDescription::mFrameSizeMs, which JsepAudioCodecDescription::Negotiate sets based on either the a=maxptime SDP attribute, or if that is not set, the a=ptime attribute, or if that is not set, the ptime opus fmtp param (but only for the send direction, and only for opus). Neither Firefox nor Chrome use put any of these things in their SDP. Our sipcc and rust SDP parsers seem to support a=ptime and a=maxptime, but only our rust parser supports the ptime fmtp. Additionally, the ptime-related logic in JsepAudioCodecDescription::Negotiate seems pretty strange to me. Query: Should this be setting mMaxFrameSizeMs instead of mFrameSizeMs?

Comment 42

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Byron, with regards to kCodecParamMaxPlaybackRate, there may indeed be an issue.

The C++ OpusParameters wrapper type has the correct default value [1]. The rust parser also has the correct value [2]. SIPCC does not have a default value [3][4].
[1] https://searchfox.org/mozilla-central/rev/e9cd2997be1071b9bb76fc14df0f01a2bd721c30/dom/media/webrtc/sdp/SdpAttribute.h#1369
[2] https://searchfox.org/mozilla-central/rev/e9cd2997be1071b9bb76fc14df0f01a2bd721c30/third_party/rust/webrtc-sdp/src/attribute_type.rs#2050
[3] https://searchfox.org/mozilla-central/rev/e9cd2997be1071b9bb76fc14df0f01a2bd721c30/third_party/sipcc/sdp_attr.c#1289
[4] https://searchfox.org/mozilla-central/rev/e9cd2997be1071b9bb76fc14df0f01a2bd721c30/dom/media/webrtc/sdp/SipccSdpAttributeList.cpp#759

Comment 43

[Byron Campen [:bwc]]

Is there anything else we need to examine here?

Comment 44

[Andreas Pehrson [:pehrsons]]

Yes, SdpVideoFormat, and most importantly its parameters. We have our own platform codec impls for video and they need to handle these parameters properly too. Decoders can generally inspect the bitstream but encoders would have to rely on parameters. If we know what the parameters are per-codec we can always divide the work of verifying our codec impls later.

Great work on the others so far. Could you summarize any action points when you're done? I'm low on cycles and this is a lot to go through.

Comment 45

[Nico Grunbaum [:ng, @chew:mozilla.org]]

This will be very useful as we look at deleting SipCC and refactoring WebRTC-SDP.

Comment 46

[Byron Campen [:bwc]]

(In reply to Andreas Pehrson [:pehrsons] from comment #44)

Yes, SdpVideoFormat, and most importantly its parameters. We have our own platform codec impls for video and they need to handle these parameters properly too. Decoders can generally inspect the bitstream but encoders would have to rely on parameters. If we know what the parameters are per-codec we can always divide the work of verifying our codec impls later.

Great work on the others so far. Could you summarize any action points when you're done? I'm low on cycles and this is a lot to go through.

I don't see any of our code writing SdpVideoFormat::parameters, is that something we expect to see?

Comment 47

[Byron Campen [:bwc]]

Yeah, the stuff we're negotiating in SDP is definitely never ending up in SdpVideoFormat::parameters, and therefore not making it down into the code in WebrtcMediaDataEncoder::CreateEncoder. We just end up using the default in ParseSdpProfileLevelId. Now, this just happens to be the same thing that we negotiate by default, but we end up doing the wrong thing if SDP negotiation settles on something else.

Comment 48

[Nico Grunbaum [:ng, @chew:mozilla.org]]

(In reply to Byron Campen [:bwc] from comment #47)

Yeah, the stuff we're negotiating in SDP is definitely never ending up in SdpVideoFormat::parameters, and therefore not making it down into the code in WebrtcMediaDataEncoder::CreateEncoder. We just end up using the default in ParseSdpProfileLevelId. Now, this just happens to be the same thing that we negotiate by default, but we end up doing the wrong thing if SDP negotiation settles on something else.

Yeah, we need a bug here.

Comment 49

[Andreas Pehrson [:pehrsons]]

(In reply to Byron Campen [:bwc] from comment #46)

I don't see any of our code writing SdpVideoFormat::parameters, is that something we expect to see?

Great, so we shouldn't expect a regression from this. But we should look at what is supported by the upstream codecs, and what Chromium is writing. Because I'd guess there's something we should pass on from negotiation, although not as much as for audio.

Comment 50

[Byron Campen [:bwc]]

(In reply to Nico Grunbaum [:ng, @chew:mozilla.org] from comment #48)

(In reply to Byron Campen [:bwc] from comment #47)

Yeah, the stuff we're negotiating in SDP is definitely never ending up in SdpVideoFormat::parameters, and therefore not making it down into the code in WebrtcMediaDataEncoder::CreateEncoder. We just end up using the default in ParseSdpProfileLevelId. Now, this just happens to be the same thing that we negotiate by default, but we end up doing the wrong thing if SDP negotiation settles on something else.

Yeah, we need a bug here.

Filed bug 1746205.