Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new encoder factory for UWP MF encoder #37

Open
wants to merge 4 commits into
base: releases/m71
Choose a base branch
from

Conversation

Holo-Krzysztof
Copy link
Contributor

The rationale behind this is the ability to use the UWP H264 encoder when building with rtc_use_builtin_sw_codecs=false because that flag disables some CreatePeerconnectionFactory overloads that take cricket::WebRtcVideoEncoderFactory (which is deprecated anyway) instead of webrtc::VideoEncoderFactory.

The naming isn't perfect but I thought as long as cricket::WebRtcVideoEncoderFactory is still used somewhere in M71 I should not delete it.

Copy link
Contributor

@morosev morosev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we able to move to one approach only? We are fine to move from deprecated solution and to use webrtc::VideoEncoderFactory as a base class.
On the other hand, I've found this comment in WebRTC sources:

  struct CodecInfo {
    // |is_hardware_accelerated| is true if the encoders created by this factory
    // of the given codec will use hardware support.
    bool is_hardware_accelerated;
    // |has_internal_source| is true if encoders created by this factory of the
    // given codec will use internal camera sources, meaning that they don't
    // require/expect frames to be delivered via webrtc::VideoEncoder::Encode.
    // This flag is used as the internal_source parameter to
    // webrtc::ViEExternalCodec::RegisterExternalSendCodec.
    bool has_internal_source;
  };

I searched the code and is_hardware_accelerated and has_internal_source are not used for codec selection. Not sure whether the flags are needed at all. MF based encoder uses HW resources if available on the platform.
I would suggest to delete old approach and to make a PR for webrtc::VideoEncoderFactory base class. No need for two factories, anyway.

@Holo-Krzysztof
Copy link
Contributor Author

I've found out it's used in video_stream_encoder.cc, line 593. Seems to be for CPU overuse detection/estimation, i.e. the thresholds are set differently when is_hardware_accelerated is true.

@robin-raymond
Copy link
Contributor

Would be nice to have a single factor with feature options.

@Holo-Krzysztof
Copy link
Contributor Author

Alright, then should I set is_hardware_accelerated to true? I'd default to yes because I'd assume most hardware from the last 5+ years has dedicated H264 encoding/decoding support. According to Wikipedia, Intel has had it since Sandy Bridge, which is from 2011 so it should be safe on desktops. Windows Mobile is dead, so there's little value in supporting that.
Not sure about HoloLens, though. I know that decoding works and the hardware would theoretically even support HEVC, but do you know if it supports hardware-accelerated H264 encoding?

@morosev
Copy link
Contributor

morosev commented Feb 19, 2019

Yes, I agree that HW support should be always on for MF based encoder engine. But anyway we don't need two factories. I'm fine to move to the newer factory API and to keep the HW acceleration flag set to true value. You can apply your changes to the original factory code.
HoloLens supports HW H.264 acceleration.

@Holo-Krzysztof
Copy link
Contributor Author

I've removed the old encoder factory and renamed the new one so it has the same name.
Decoder will likely follow in a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants