WebRTC Code Reviews
Help | Chromium Project | Sign in
(10885)

Issue 54549004: Move decoder attributes to webrtc::VideoDecoder. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 5 months ago by pbos
Modified:
3 years, 9 months ago
Reviewers:
Stefan, mflodman
CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie.mao, ajm, tterriberry, qiang.lu, Niklas, andresp-webrtc, perkj, mflodman
Base URL:
https://chromium.googlesource.com/external/webrtc.git@master
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

Move decoder attributes to webrtc::VideoDecoder. Adds a packetization_type() to the VideoDecoder interface to specify packetization type instead of basing it on VideoCodec::CodecType or VideoCodec::plName. Also moves decoder-is-renderer attributes to the decoder with is_renderer() and expected_delay_ms(). BUG= R=mflodman@webrtc.org,stefan@webrtc.org

Patch Set 1 #

Patch Set 2 : initialize payload_name #

Patch Set 3 : add packetization_type to android #

Patch Set 4 : bah #

Patch Set 5 : bah again! :D #

Patch Set 6 : more bah #

Total comments: 22

Patch Set 7 : feedback #

Patch Set 8 : rebase #

Patch Set 9 : render-info struct #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+235 lines, -190 lines) Patch
M talk/app/webrtc/java/jni/androidmediadecoder_jni.cc View 1 2 3 4 5 6 7 3 chunks +17 lines, -0 lines 0 comments Download
M talk/media/webrtc/fakewebrtcvideoengine.h View 1 2 3 4 5 6 7 1 chunk +12 lines, -11 lines 0 comments Download
M talk/media/webrtc/webrtcvideoengine2.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -5 lines 0 comments Download
M talk/media/webrtc/webrtcvideoengine2_unittest.cc View 1 2 3 4 5 6 7 1 chunk +8 lines, -2 lines 0 comments Download
M webrtc/common_types.h View 1 2 3 4 5 6 1 chunk +11 lines, -0 lines 0 comments Download
M webrtc/common_types.cc View 1 2 3 4 5 6 1 chunk +16 lines, -0 lines 0 comments Download
M webrtc/modules/interface/module_common_types.h View 1 2 3 4 5 6 7 2 chunks +1 line, -7 lines 0 comments Download
M webrtc/modules/rtp_rtcp/interface/rtp_rtcp_defines.h View 1 chunk +1 line, -1 line 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_format.h View 2 chunks +2 lines, -2 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_format.cc View 3 chunks +6 lines, -2 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc View 1 chunk +1 line, -1 line 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_sender.h View 1 2 3 4 5 6 7 2 chunks +2 lines, -3 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_sender.cc View 1 2 3 4 5 6 7 5 chunks +2 lines, -14 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_sender_video.h View 1 2 3 4 5 6 4 chunks +1 line, -6 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc View 1 2 3 4 5 6 3 chunks +2 lines, -11 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/video_codec_information.h View 1 chunk +1 line, -1 line 0 comments Download
M webrtc/modules/video_coding/codecs/i420/main/interface/i420.h View 1 2 3 4 5 6 7 1 chunk +4 lines, -0 lines 0 comments Download
M webrtc/modules/video_coding/codecs/interface/mock/mock_video_codec_interface.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/modules/video_coding/codecs/vp8/vp8_impl.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc View 1 2 3 4 5 6 7 1 chunk +4 lines, -0 lines 0 comments Download
M webrtc/modules/video_coding/codecs/vp9/vp9_impl.h View 1 2 3 4 5 6 7 2 chunks +2 lines, -2 lines 0 comments Download
M webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc View 1 2 3 4 5 6 7 1 chunk +4 lines, -0 lines 0 comments Download
M webrtc/modules/video_coding/main/source/packet.cc View 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/modules/video_coding/main/test/test_util.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M webrtc/test/call_test.cc View 1 chunk +3 lines, -3 lines 0 comments Download
M webrtc/test/encoder_settings.h View 1 chunk +1 line, -1 line 0 comments Download
M webrtc/test/encoder_settings.cc View 1 chunk +8 lines, -12 lines 0 comments Download
M webrtc/test/fake_decoder.h View 1 2 3 4 5 6 7 2 chunks +4 lines, -0 lines 0 comments Download
M webrtc/test/fake_decoder.cc View 2 chunks +8 lines, -0 lines 0 comments Download
M webrtc/video/bitrate_estimator_tests.cc View 1 chunk +3 lines, -7 lines 0 comments Download
M webrtc/video/end_to_end_tests.cc View 1 2 3 4 5 6 7 4 chunks +11 lines, -20 lines 0 comments Download
M webrtc/video/loopback.cc View 2 chunks +2 lines, -3 lines 0 comments Download
M webrtc/video/replay.cc View 1 2 3 4 5 6 7 2 chunks +5 lines, -6 lines 0 comments Download
M webrtc/video/video_decoder.cc View 1 2 3 4 5 6 7 8 2 chunks +11 lines, -0 lines 3 comments Download
M webrtc/video/video_decoder_unittest.cc View 1 2 3 4 5 6 7 1 chunk +5 lines, -0 lines 0 comments Download
M webrtc/video/video_receive_stream.cc View 1 2 3 4 5 6 7 8 4 chunks +51 lines, -37 lines 0 comments Download
M webrtc/video_decoder.h View 1 2 3 4 5 6 7 8 3 chunks +20 lines, -0 lines 0 comments Download
M webrtc/video_receive_stream.h View 1 2 3 4 5 6 7 2 chunks +1 line, -32 lines 0 comments Download
Trybot results:
Project "webrtc" does not have a commit queue.

Messages

Total messages: 20 (0 generated)
pbos
PTAL, I'd like to add ::packetization_type() to webrtc::VideoEncoder as well. Check this out and see ...
4 years, 5 months ago (2015-05-25 10:53:08 UTC) #1
pbos
initialize payload_name
4 years, 5 months ago (2015-05-25 11:01:00 UTC) #2
pbos
add packetization_type to android
4 years, 5 months ago (2015-05-25 11:18:24 UTC) #3
pbos
bah
4 years, 5 months ago (2015-05-25 11:25:35 UTC) #4
pbos
bah again! :D
4 years, 5 months ago (2015-05-25 12:22:27 UTC) #5
pbos
more bah
4 years, 5 months ago (2015-05-25 12:59:09 UTC) #6
Stefan
good change I think! https://review.webrtc.org/54549004/diff/100001/webrtc/common_types.h File webrtc/common_types.h (right): https://review.webrtc.org/54549004/diff/100001/webrtc/common_types.h#newcode338 webrtc/common_types.h:338: // matters. Solve before submit ...
4 years, 5 months ago (2015-05-28 12:36:41 UTC) #7
pbos
feedback
4 years, 5 months ago (2015-06-03 10:01:48 UTC) #8
pbos
rebase
4 years, 5 months ago (2015-06-03 10:04:11 UTC) #9
pbos
PTAL https://review.webrtc.org/54549004/diff/100001/webrtc/common_types.h File webrtc/common_types.h (right): https://review.webrtc.org/54549004/diff/100001/webrtc/common_types.h#newcode338 webrtc/common_types.h:338: // matters. Solve before submit so that kRtpVideoVp9 ...
4 years, 5 months ago (2015-06-03 10:04:28 UTC) #10
pbos
https://review.webrtc.org/54549004/diff/100001/webrtc/video/video_receive_stream.cc File webrtc/video/video_receive_stream.cc (right): https://review.webrtc.org/54549004/diff/100001/webrtc/video/video_receive_stream.cc#newcode33 webrtc/video/video_receive_stream.cc:33: // string function? On 2015/05/28 12:36:40, Stefan wrote: > ...
4 years, 5 months ago (2015-06-03 12:52:14 UTC) #11
Stefan
LG, but please explain the last comment. https://review.webrtc.org/54549004/diff/100001/webrtc/common_types.h File webrtc/common_types.h (right): https://review.webrtc.org/54549004/diff/100001/webrtc/common_types.h#newcode338 webrtc/common_types.h:338: // matters. ...
4 years, 5 months ago (2015-06-04 11:32:02 UTC) #12
pbos
https://review.webrtc.org/54549004/diff/100001/webrtc/video_decoder.h File webrtc/video_decoder.h (right): https://review.webrtc.org/54549004/diff/100001/webrtc/video_decoder.h#newcode75 webrtc/video_decoder.h:75: virtual int expected_delay_ms() const { return 0; } On ...
4 years, 5 months ago (2015-06-04 11:43:28 UTC) #13
Stefan
https://review.webrtc.org/54549004/diff/100001/webrtc/video_decoder.h File webrtc/video_decoder.h (right): https://review.webrtc.org/54549004/diff/100001/webrtc/video_decoder.h#newcode75 webrtc/video_decoder.h:75: virtual int expected_delay_ms() const { return 0; } On ...
4 years, 5 months ago (2015-06-04 11:55:28 UTC) #14
pbos
On 2015/06/04 11:55:28, Stefan wrote: > https://review.webrtc.org/54549004/diff/100001/webrtc/video_decoder.h > File webrtc/video_decoder.h (right): > > https://review.webrtc.org/54549004/diff/100001/webrtc/video_decoder.h#newcode75 > ...
4 years, 5 months ago (2015-06-04 12:10:46 UTC) #15
Stefan
On 2015/06/04 12:10:46, pbos wrote: > On 2015/06/04 11:55:28, Stefan wrote: > > https://review.webrtc.org/54549004/diff/100001/webrtc/video_decoder.h > ...
4 years, 5 months ago (2015-06-04 12:31:46 UTC) #16
pbos
render-info struct
4 years, 5 months ago (2015-06-04 14:31:11 UTC) #17
pbos
https://review.webrtc.org/54549004/diff/160001/webrtc/video/video_decoder.cc File webrtc/video/video_decoder.cc (right): https://review.webrtc.org/54549004/diff/160001/webrtc/video/video_decoder.cc#newcode136 webrtc/video/video_decoder.cc:136: return decoder_->internal_renderer_info(); I don't like this part. If we ...
4 years, 5 months ago (2015-06-04 14:33:49 UTC) #18
Stefan
https://review.webrtc.org/54549004/diff/160001/webrtc/video/video_decoder.cc File webrtc/video/video_decoder.cc (right): https://review.webrtc.org/54549004/diff/160001/webrtc/video/video_decoder.cc#newcode136 webrtc/video/video_decoder.cc:136: return decoder_->internal_renderer_info(); On 2015/06/04 14:33:49, pbos wrote: > I ...
4 years, 5 months ago (2015-06-10 14:19:06 UTC) #19
pbos
4 years, 5 months ago (2015-06-12 14:35:26 UTC) #20
https://review.webrtc.org/54549004/diff/160001/webrtc/video/video_decoder.cc
File webrtc/video/video_decoder.cc (right):

https://review.webrtc.org/54549004/diff/160001/webrtc/video/video_decoder.cc#...
webrtc/video/video_decoder.cc:136: return decoder_->internal_renderer_info();
On 2015/06/10 14:19:06, Stefan wrote:
> On 2015/06/04 14:33:49, pbos wrote:
> > I don't like this part. If we have to fall back to software rendering the
> > rendering will break. Do we have any case where we actually have to support
> that
> > the decoder is a renderer? For textures we pass a handle into the future,
and
> > anyone can choose to implement a no-op renderer.
> 
> I'm not sure if it's being used anywhere. We may want to check that.

Talked to mflodman@, looks like we need it. I think handling software-decoder
fallback for this case is harder though and not necessarily part of this CL (nor
does it constitute a regression).
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 245c2c2-tainted