Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
647613007:
RtcVideoEncoder: determine video codec profile at InitEncode time. (Closed)
|
Created:
6 years, 2 months ago by hshi1 Modified:
6 years, 1 month ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionRtcVideoEncoder: determine video codec profile at InitEncode time.
The profile cannot be determined by video codec type alone. Instead
the RtcVideoEncoder should wait until InitEncode time to determine
video codec profile based on the codec settings.
BUG=424828
TEST=trybot
Committed: https://crrev.com/872c6c6113f21dbca5d9c06ac21106e83be87bfd
Cr-Commit-Position: refs/heads/master@{#301414}
Patch Set 1 #
Total comments: 15
Patch Set 2 : nits #Patch Set 3 : Determine profile at InitEncode. #
Total comments: 8
Patch Set 4 : Second pass of comments. #
Messages
Total messages: 22 (5 generated)
hshi@chromium.org changed reviewers: + posciak@chromium.org
PTAL. It is necessary to override the default codec profile (which is determined by RtcVideoEncoderFactory based on video codec type only), to the codec profile specified in webrtc codec settings. Currently this is only required for H264 codec.
On 2014/10/18 02:28:43, hshi1 wrote: > PTAL. It is necessary to override the default codec profile (which is determined > by RtcVideoEncoderFactory based on video codec type only), to the codec profile > specified in webrtc codec settings. > > Currently this is only required for H264 codec. Would be nice to have this explanation in the CL message please.
https://chromiumcodereview.appspot.com/647613007/diff/1/content/renderer/medi... File content/renderer/media/rtc_video_encoder.cc (right): https://chromiumcodereview.appspot.com/647613007/diff/1/content/renderer/medi... content/renderer/media/rtc_video_encoder.cc:65: // Maps webrtc::VideoCodecProfile to media::VideoCodecProfile for H264 codec. Could we instead have a method that takes both profile and type from webrtc and converts to media::VideoCodecProfile and unify with the existing method in RtcVEFactory? Or actually, since we should move type/codec to InitEncode entirely, probably unify it here and remove from factory. https://chromiumcodereview.appspot.com/647613007/diff/1/content/renderer/medi... content/renderer/media/rtc_video_encoder.cc:70: return media::H264PROFILE_BASELINE; Is this actually baseline or constrained baseline? https://chromiumcodereview.appspot.com/647613007/diff/1/content/renderer/medi... content/renderer/media/rtc_video_encoder.cc:76: return media::H264PROFILE_BASELINE; notreached. https://chromiumcodereview.appspot.com/647613007/diff/1/content/renderer/medi... content/renderer/media/rtc_video_encoder.cc:591: if (codec_settings->codecType == webrtc::kVideoCodecH264) { If we can't determine profile on construction, can we remove it from the constructor?
posciak@chromium.org changed reviewers: + wuchengli@chromium.org
+Wu-Cheng
https://codereview.chromium.org/647613007/diff/1/content/renderer/media/rtc_v... File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/647613007/diff/1/content/renderer/media/rtc_v... content/renderer/media/rtc_video_encoder.cc:65: // Maps webrtc::VideoCodecProfile to media::VideoCodecProfile for H264 codec. On 2014/10/20 06:28:43, Pawel Osciak wrote: > Could we instead have a method that takes both profile and type from webrtc and > converts to media::VideoCodecProfile and unify with the existing method in > RtcVEFactory? > > Or actually, since we should move type/codec to InitEncode entirely, probably > unify it here and remove from factory. The |profile_| is of media::VideoCodecProfile type; it is internal to chrome media code and not recognized by WebRTC. The reason for RtcVE to retain a member |profile_| at its constructor is because that is the only place where RtcVEFactory interacts with the RtcVE instance. If we don't save |profile| there it would be lost, as webRTC code has no idea about its existence. https://codereview.chromium.org/647613007/diff/1/content/renderer/media/rtc_v... content/renderer/media/rtc_video_encoder.cc:70: return media::H264PROFILE_BASELINE; On 2014/10/20 06:28:43, Pawel Osciak wrote: > Is this actually baseline or constrained baseline? This is the actual baseline, however as I commented in the bug, the HW encoder is free to implement a proper subset of the requested profile (e.g. ConstrainedBaseline) and still be compliant. https://codereview.chromium.org/647613007/diff/1/content/renderer/media/rtc_v... content/renderer/media/rtc_video_encoder.cc:76: return media::H264PROFILE_BASELINE; On 2014/10/20 06:28:43, Pawel Osciak wrote: > notreached. Acknowledged. https://codereview.chromium.org/647613007/diff/1/content/renderer/media/rtc_v... content/renderer/media/rtc_video_encoder.cc:591: if (codec_settings->codecType == webrtc::kVideoCodecH264) { On 2014/10/20 06:28:43, Pawel Osciak wrote: > If we can't determine profile on construction, can we remove it from the > constructor? No we can't because of the way webRTC defines the VideoEncoder interface. webRTC does not explicitly define video codec profiles (the H264 profile enum is only added by holmer@ very recently, and it is part of the codecSpecific struct, not a general concept).
Patchset #2 (id:20001) has been deleted
https://chromiumcodereview.appspot.com/647613007/diff/1/content/renderer/medi... File content/renderer/media/rtc_video_encoder.cc (right): https://chromiumcodereview.appspot.com/647613007/diff/1/content/renderer/medi... content/renderer/media/rtc_video_encoder.cc:65: // Maps webrtc::VideoCodecProfile to media::VideoCodecProfile for H264 codec. On 2014/10/20 16:55:50, hshi1 wrote: > On 2014/10/20 06:28:43, Pawel Osciak wrote: > > Could we instead have a method that takes both profile and type from webrtc > and > > converts to media::VideoCodecProfile and unify with the existing method in > > RtcVEFactory? > > > > Or actually, since we should move type/codec to InitEncode entirely, probably > > unify it here and remove from factory. > > The |profile_| is of media::VideoCodecProfile type; it is internal to chrome > media code and not recognized by WebRTC. Yes. My suggestion though was that it would be nice to try to delay translation from WebRTC's [codec,profile] pair into media::VCP until InitEncode, when we have both, instead of translating from codec and then doing a fixup in InitEncode and do it in RTCVE::InitEncode. Especially since RTCVE constructor doesn't really do anything with it. > > The reason for RtcVE to retain a member |profile_| at its constructor is because > that is the only place where RtcVEFactory interacts with the RtcVE instance. If > we don't save |profile| there it would be lost, as webRTC code has no idea about > its existence. Yes, although could we maybe change the factory to do what I'm suggesting in comment below, unless I'm missing something? https://chromiumcodereview.appspot.com/647613007/diff/1/content/renderer/medi... content/renderer/media/rtc_video_encoder.cc:70: return media::H264PROFILE_BASELINE; On 2014/10/20 16:55:50, hshi1 wrote: > On 2014/10/20 06:28:43, Pawel Osciak wrote: > > Is this actually baseline or constrained baseline? > > This is the actual baseline, however as I commented in the bug, the HW encoder > is free to implement a proper subset of the requested profile (e.g. > ConstrainedBaseline) and still be compliant. The issue is, what if an encoder does non-constrained and receiver can't decode it... There is no way to pass this information/negotiate through media::VCP, or WebRTC, or between devices, as far as I can tell? No matter whether it's CBP or BP, we always say BP... It's true that it's a superset so technically ok, but we may lose opportunity to decode if it's CBP and we treat it as BP, or may fail unexpectedly if we always treat as CBP. https://chromiumcodereview.appspot.com/647613007/diff/1/content/renderer/medi... content/renderer/media/rtc_video_encoder.cc:591: if (codec_settings->codecType == webrtc::kVideoCodecH264) { On 2014/10/20 16:55:50, hshi1 wrote: > On 2014/10/20 06:28:43, Pawel Osciak wrote: > > If we can't determine profile on construction, can we remove it from the > > constructor? > > No we can't because of the way webRTC defines the VideoEncoder interface. webRTC > does not explicitly define video codec profiles (the H264 profile enum is only > added by holmer@ very recently, and it is part of the codecSpecific struct, not > a general concept). We could perhaps store webrtc codec in RTCVE constructor and translate only once we get webrtc profile here from both (or just codec for VP8), and not in the factory?
https://codereview.chromium.org/647613007/diff/1/content/renderer/media/rtc_v... File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/647613007/diff/1/content/renderer/media/rtc_v... content/renderer/media/rtc_video_encoder.cc:65: // Maps webrtc::VideoCodecProfile to media::VideoCodecProfile for H264 codec. On 2014/10/21 13:41:46, Pawel Osciak wrote: > On 2014/10/20 16:55:50, hshi1 wrote: > > On 2014/10/20 06:28:43, Pawel Osciak wrote: > > > Could we instead have a method that takes both profile and type from webrtc > > and > > > converts to media::VideoCodecProfile and unify with the existing method in > > > RtcVEFactory? > > > > > > Or actually, since we should move type/codec to InitEncode entirely, > probably > > > unify it here and remove from factory. > > > > The |profile_| is of media::VideoCodecProfile type; it is internal to chrome > > media code and not recognized by WebRTC. > > Yes. My suggestion though was that it would be nice to try to delay translation > from WebRTC's [codec,profile] pair into media::VCP until InitEncode, when we > have both, instead of translating from codec and then doing a fixup in > InitEncode and do it in RTCVE::InitEncode. Especially since RTCVE constructor > doesn't really do anything with it. > > > > > The reason for RtcVE to retain a member |profile_| at its constructor is > because > > that is the only place where RtcVEFactory interacts with the RtcVE instance. > If > > we don't save |profile| there it would be lost, as webRTC code has no idea > about > > its existence. > > Yes, although could we maybe change the factory to do what I'm suggesting in > comment below, unless I'm missing something? Done. https://codereview.chromium.org/647613007/diff/1/content/renderer/media/rtc_v... content/renderer/media/rtc_video_encoder.cc:70: return media::H264PROFILE_BASELINE; On 2014/10/21 13:41:46, Pawel Osciak wrote: > On 2014/10/20 16:55:50, hshi1 wrote: > > On 2014/10/20 06:28:43, Pawel Osciak wrote: > > > Is this actually baseline or constrained baseline? > > > > This is the actual baseline, however as I commented in the bug, the HW encoder > > is free to implement a proper subset of the requested profile (e.g. > > ConstrainedBaseline) and still be compliant. > > The issue is, what if an encoder does non-constrained and receiver can't decode > it... There is no way to pass this information/negotiate through media::VCP, or > WebRTC, or between devices, as far as I can tell? No matter whether it's CBP or > BP, we always say BP... It's true that it's a superset so technically ok, but we > may lose opportunity to decode if it's CBP and we treat it as BP, or may fail > unexpectedly if we always treat as CBP. But there's no constrained baseline profile definition in media code, so even if we want to pass CBP down to GpuVEA there's no enums defined at the moment (see media/base/video_decoder_config.h) https://codereview.chromium.org/647613007/diff/1/content/renderer/media/rtc_v... content/renderer/media/rtc_video_encoder.cc:591: if (codec_settings->codecType == webrtc::kVideoCodecH264) { On 2014/10/21 13:41:46, Pawel Osciak wrote: > On 2014/10/20 16:55:50, hshi1 wrote: > > On 2014/10/20 06:28:43, Pawel Osciak wrote: > > > If we can't determine profile on construction, can we remove it from the > > > constructor? > > > > No we can't because of the way webRTC defines the VideoEncoder interface. > webRTC > > does not explicitly define video codec profiles (the H264 profile enum is only > > added by holmer@ very recently, and it is part of the codecSpecific struct, > not > > a general concept). > > We could perhaps store webrtc codec in RTCVE constructor and translate only once > we get webrtc profile here from both (or just codec for VP8), and not in the > factory? Done.
Ping -- PTAL?
https://chromiumcodereview.appspot.com/647613007/diff/60001/content/renderer/... File content/renderer/media/rtc_video_encoder.cc (right): https://chromiumcodereview.appspot.com/647613007/diff/60001/content/renderer/... content/renderer/media/rtc_video_encoder.cc:556: DVLOG(1) << "RTCVideoEncoder(): type=" << type; s/type/codec type/ https://chromiumcodereview.appspot.com/647613007/diff/60001/content/renderer/... content/renderer/media/rtc_video_encoder.cc:576: video_codec_profile_ = RTCVideoEncoderFactory::GetCodecProfile( video_codec_profile_ could just be a local variable here (if we start passing it to RecordInitEncodeUMA). https://chromiumcodereview.appspot.com/647613007/diff/60001/content/renderer/... File content/renderer/media/rtc_video_encoder_factory.cc (right): https://chromiumcodereview.appspot.com/647613007/diff/60001/content/renderer/... content/renderer/media/rtc_video_encoder_factory.cc:75: default: There are only two cases and we are handling both. Perhaps removing the default case would be better, letting the compiler warn/error out if there is ever a new enum value added in the future, so that whoever adds it will have to update this too? https://chromiumcodereview.appspot.com/647613007/diff/60001/content/renderer/... File content/renderer/media/rtc_video_encoder_factory.h (right): https://chromiumcodereview.appspot.com/647613007/diff/60001/content/renderer/... content/renderer/media/rtc_video_encoder_factory.h:33: static media::VideoCodecProfile GetCodecProfile( Since only RTCVE is using it, perhaps we could move it into RTCVE itself please? Otherwise we are creating a circular dependency between RTCVE and RTCVEF. I'd suggest calling it WebRTCProfileToVideoCodecProfile() or something similar also.
https://codereview.chromium.org/647613007/diff/1/content/renderer/media/rtc_v... File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/647613007/diff/1/content/renderer/media/rtc_v... content/renderer/media/rtc_video_encoder.cc:70: return media::H264PROFILE_BASELINE; On 2014/10/22 19:01:16, hshi1 wrote: > On 2014/10/21 13:41:46, Pawel Osciak wrote: > > On 2014/10/20 16:55:50, hshi1 wrote: > > > On 2014/10/20 06:28:43, Pawel Osciak wrote: > > > > Is this actually baseline or constrained baseline? > > > > > > This is the actual baseline, however as I commented in the bug, the HW > encoder > > > is free to implement a proper subset of the requested profile (e.g. > > > ConstrainedBaseline) and still be compliant. > > > > The issue is, what if an encoder does non-constrained and receiver can't > decode > > it... There is no way to pass this information/negotiate through media::VCP, > or > > WebRTC, or between devices, as far as I can tell? No matter whether it's CBP > or > > BP, we always say BP... It's true that it's a superset so technically ok, but > we > > may lose opportunity to decode if it's CBP and we treat it as BP, or may fail > > unexpectedly if we always treat as CBP. > > But there's no constrained baseline profile definition in media code, so even if > we want to pass CBP down to GpuVEA there's no enums defined at the moment (see > media/base/video_decoder_config.h) Right, but that's a bug (crbug.com/345569) so perhaps it might be a good time to consider looking into it (separately from this CL). I'm just explaining consequences here and in crbug.com/424828 and hoping whomever is more knowledgeable of what we should be expecting from decoders/encoders we use for this to weigh the priority of looking into it.
https://codereview.chromium.org/647613007/diff/60001/content/renderer/media/r... File content/renderer/media/rtc_video_encoder.cc (right): https://codereview.chromium.org/647613007/diff/60001/content/renderer/media/r... content/renderer/media/rtc_video_encoder.cc:556: DVLOG(1) << "RTCVideoEncoder(): type=" << type; On 2014/10/25 01:23:04, Pawel Osciak wrote: > s/type/codec type/ Done. https://codereview.chromium.org/647613007/diff/60001/content/renderer/media/r... content/renderer/media/rtc_video_encoder.cc:576: video_codec_profile_ = RTCVideoEncoderFactory::GetCodecProfile( On 2014/10/25 01:23:04, Pawel Osciak wrote: > video_codec_profile_ could just be a local variable here (if we start passing it > to RecordInitEncodeUMA). Done. https://codereview.chromium.org/647613007/diff/60001/content/renderer/media/r... File content/renderer/media/rtc_video_encoder_factory.cc (right): https://codereview.chromium.org/647613007/diff/60001/content/renderer/media/r... content/renderer/media/rtc_video_encoder_factory.cc:75: default: On 2014/10/25 01:23:04, Pawel Osciak wrote: > There are only two cases and we are handling both. Perhaps removing the default > case would be better, letting the compiler warn/error out if there is ever a new > enum value added in the future, so that whoever adds it will have to update this > too? Done. https://codereview.chromium.org/647613007/diff/60001/content/renderer/media/r... File content/renderer/media/rtc_video_encoder_factory.h (right): https://codereview.chromium.org/647613007/diff/60001/content/renderer/media/r... content/renderer/media/rtc_video_encoder_factory.h:33: static media::VideoCodecProfile GetCodecProfile( On 2014/10/25 01:23:04, Pawel Osciak wrote: > Since only RTCVE is using it, perhaps we could move it into RTCVE itself please? > Otherwise we are creating a circular dependency between RTCVE and RTCVEF. > > I'd suggest calling it WebRTCProfileToVideoCodecProfile() or something similar > also. Done.
lgtm
hshi@chromium.org changed reviewers: + dalecurtis@chromium.org
+dalecurtis (OWNERS content/renderer/media) PTAL thanks!
rs lgtm
The CQ bit was checked by hshi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/647613007/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/872c6c6113f21dbca5d9c06ac21106e83be87bfd Cr-Commit-Position: refs/heads/master@{#301414} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
