Expose codec implementation names in stats.

Expose codec implementation names in stats.

Used to distinguish between software/hardware encoders/decoders and
other implementation differences. Useful for tracking quality
regressions related to specific implementations.

BUG=webrtc:4897
R=hta@webrtc.org, mflodman@webrtc.org, stefan@webrtc.org

Committed: https://crrev.com/b7d9a97ce41022e984348efb5f28bf6dd6c6b779
Cr-Commit-Position: refs/heads/master@{#11084}

[pbos-webrtc, 2015-10-15 14:48:34]

PTAL, hta@, pay extra attention to the constant names.

[stefan-webrtc, 2015-10-16 07:47:03]

https://codereview.webrtc.org/1406903002/diff/1/webrtc/modules/video_coding/c...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc
(right):

https://codereview.webrtc.org/1406903002/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:270:
return "VideoToolbox";
Is this the proper name to use? Would it be good to at least include iOS
somewhere in the name?

https://codereview.webrtc.org/1406903002/diff/1/webrtc/modules/video_coding/m...
File webrtc/modules/video_coding/main/source/generic_decoder.cc (right):

https://codereview.webrtc.org/1406903002/diff/1/webrtc/modules/video_coding/m...
webrtc/modules/video_coding/main/source/generic_decoder.cc:164:
_callback->OnDecoderImplementationName(_decoder.ImplementationName());
Can't we call this only when the implementation actually changes?

https://codereview.webrtc.org/1406903002/diff/1/webrtc/modules/video_coding/m...
File webrtc/modules/video_coding/main/source/generic_encoder.cc (right):

https://codereview.webrtc.org/1406903002/diff/1/webrtc/modules/video_coding/m...
webrtc/modules/video_coding/main/source/generic_encoder.cc:349:
_sendCallback->OnEncoderImplementationName(implementation_name);
feel free to fix _sendCallback -> send_callback_

https://codereview.webrtc.org/1406903002/diff/1/webrtc/test/fake_decoder.h
File webrtc/test/fake_decoder.h (right):

https://codereview.webrtc.org/1406903002/diff/1/webrtc/test/fake_decoder.h#ne...
webrtc/test/fake_decoder.h:45: private:
newline above

https://codereview.webrtc.org/1406903002/diff/1/webrtc/test/fake_encoder.h
File webrtc/test/fake_encoder.h (right):

https://codereview.webrtc.org/1406903002/diff/1/webrtc/test/fake_encoder.h#ne...
webrtc/test/fake_encoder.h:45: protected:
newline above

https://codereview.webrtc.org/1406903002/diff/1/webrtc/video/send_statistics_...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1406903002/diff/1/webrtc/video/send_statistics_...
webrtc/video/send_statistics_proxy.cc:80: void
SendStatisticsProxy::OnOutgoingRate(uint32_t framerate, uint32_t bitrate) {
newline above

https://codereview.webrtc.org/1406903002/diff/1/webrtc/video/video_encoder.cc
File webrtc/video/video_encoder.cc (right):

https://codereview.webrtc.org/1406903002/diff/1/webrtc/video/video_encoder.cc...
webrtc/video/video_encoder.cc:139: return
fallback_encoder_->ImplementationName();
Should we add "fallback-" to the string here?

[pbos-webrtc, 2015-10-16 13:10:40]

feedback

[pbos-webrtc, 2015-10-16 13:12:05]

PTAL

https://codereview.webrtc.org/1406903002/diff/1/webrtc/modules/video_coding/c...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc
(right):

https://codereview.webrtc.org/1406903002/diff/1/webrtc/modules/video_coding/c...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:270:
return "VideoToolbox";
On 2015/10/16 07:47:03, stefan-webrtc (holmer) wrote:
> Is this the proper name to use? Would it be good to at least include iOS
> somewhere in the name?

Afaik this is a general Mac interface so this could run on non-ios as well.

https://codereview.webrtc.org/1406903002/diff/1/webrtc/modules/video_coding/m...
File webrtc/modules/video_coding/main/source/generic_decoder.cc (right):

https://codereview.webrtc.org/1406903002/diff/1/webrtc/modules/video_coding/m...
webrtc/modules/video_coding/main/source/generic_decoder.cc:164:
_callback->OnDecoderImplementationName(_decoder.ImplementationName());
On 2015/10/16 07:47:03, stefan-webrtc (holmer) wrote:
> Can't we call this only when the implementation actually changes?

Problem is that the interface doesn't allow us to know when we change
implementation. This might be a fallback and we don't know when that is being
switched to. If you have a good interface idea for that let me know.

https://codereview.webrtc.org/1406903002/diff/1/webrtc/modules/video_coding/m...
File webrtc/modules/video_coding/main/source/generic_encoder.cc (right):

https://codereview.webrtc.org/1406903002/diff/1/webrtc/modules/video_coding/m...
webrtc/modules/video_coding/main/source/generic_encoder.cc:349:
_sendCallback->OnEncoderImplementationName(implementation_name);
On 2015/10/16 07:47:03, stefan-webrtc (holmer) wrote:
> feel free to fix _sendCallback -> send_callback_

Done.

https://codereview.webrtc.org/1406903002/diff/1/webrtc/test/fake_decoder.h
File webrtc/test/fake_decoder.h (right):

https://codereview.webrtc.org/1406903002/diff/1/webrtc/test/fake_decoder.h#ne...
webrtc/test/fake_decoder.h:45: private:
On 2015/10/16 07:47:03, stefan-webrtc (holmer) wrote:
> newline above

Done.

https://codereview.webrtc.org/1406903002/diff/1/webrtc/test/fake_encoder.h
File webrtc/test/fake_encoder.h (right):

https://codereview.webrtc.org/1406903002/diff/1/webrtc/test/fake_encoder.h#ne...
webrtc/test/fake_encoder.h:45: protected:
On 2015/10/16 07:47:03, stefan-webrtc (holmer) wrote:
> newline above

Done.

https://codereview.webrtc.org/1406903002/diff/1/webrtc/video/send_statistics_...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1406903002/diff/1/webrtc/video/send_statistics_...
webrtc/video/send_statistics_proxy.cc:80: void
SendStatisticsProxy::OnOutgoingRate(uint32_t framerate, uint32_t bitrate) {
On 2015/10/16 07:47:03, stefan-webrtc (holmer) wrote:
> newline above

Done.

https://codereview.webrtc.org/1406903002/diff/1/webrtc/video/video_encoder.cc
File webrtc/video/video_encoder.cc (right):

https://codereview.webrtc.org/1406903002/diff/1/webrtc/video/video_encoder.cc...
webrtc/video/video_encoder.cc:139: return
fallback_encoder_->ImplementationName();
On 2015/10/16 07:47:03, stefan-webrtc (holmer) wrote:
> Should we add "fallback-" to the string here?

Not unless we can know when an implementation is changed. I don't want to do any
string manipulation per frame. I added "software-fallback" since I think that's
more informative than "libvpx" when we've tried to do VP8 but failed.

[stefan-webrtc, 2015-10-22 09:33:34]

https://codereview.webrtc.org/1406903002/diff/1/webrtc/modules/video_coding/m...
File webrtc/modules/video_coding/main/source/generic_decoder.cc (right):

https://codereview.webrtc.org/1406903002/diff/1/webrtc/modules/video_coding/m...
webrtc/modules/video_coding/main/source/generic_decoder.cc:164:
_callback->OnDecoderImplementationName(_decoder.ImplementationName());
On 2015/10/16 13:12:05, pbos-webrtc wrote:
> On 2015/10/16 07:47:03, stefan-webrtc (holmer) wrote:
> > Can't we call this only when the implementation actually changes?
> 
> Problem is that the interface doesn't allow us to know when we change
> implementation. This might be a fallback and we don't know when that is being
> switched to. If you have a good interface idea for that let me know.

Acknowledged.

https://codereview.webrtc.org/1406903002/diff/1/webrtc/video/video_encoder.cc
File webrtc/video/video_encoder.cc (right):

https://codereview.webrtc.org/1406903002/diff/1/webrtc/video/video_encoder.cc...
webrtc/video/video_encoder.cc:139: return
fallback_encoder_->ImplementationName();
On 2015/10/16 13:12:05, pbos-webrtc wrote:
> On 2015/10/16 07:47:03, stefan-webrtc (holmer) wrote:
> > Should we add "fallback-" to the string here?
> 
> Not unless we can know when an implementation is changed. I don't want to do
any
> string manipulation per frame. I added "software-fallback" since I think
that's
> more informative than "libvpx" when we've tried to do VP8 but failed.

You don't have to manipulate the string for every frame. If you know you are
using the fallback encoder you should use the fallback encoder string, right? Or
do you mean that the fallback encoder implementation may change? In that case
maybe we should have a method to get the type of encoder so that we can check if
it has changed?

[pbos-webrtc, 2015-10-22 14:37:16]

rebase

[pbos-webrtc, 2015-10-22 15:07:12]

software-fallback string + VP8 ImplementationName()s

[pbos-webrtc, 2015-10-22 15:11:21]

PTAL, I also added VP8's implementation name ("libvpx").

https://codereview.webrtc.org/1406903002/diff/1/webrtc/video/video_encoder.cc
File webrtc/video/video_encoder.cc (right):

https://codereview.webrtc.org/1406903002/diff/1/webrtc/video/video_encoder.cc...
webrtc/video/video_encoder.cc:139: return
fallback_encoder_->ImplementationName();
On 2015/10/22 09:33:34, stefan-webrtc (holmer) wrote:
> On 2015/10/16 13:12:05, pbos-webrtc wrote:
> > On 2015/10/16 07:47:03, stefan-webrtc (holmer) wrote:
> > > Should we add "fallback-" to the string here?
> > 
> > Not unless we can know when an implementation is changed. I don't want to do
> any
> > string manipulation per frame. I added "software-fallback" since I think
> that's
> > more informative than "libvpx" when we've tried to do VP8 but failed.
> 
> You don't have to manipulate the string for every frame. If you know you are
> using the fallback encoder you should use the fallback encoder string, right?
Or
> do you mean that the fallback encoder implementation may change? In that case
> maybe we should have a method to get the type of encoder so that we can check
if
> it has changed?

Done, constructing the string once we start using the fallback encoder/decoder.

[pbos-webrtc, 2015-10-22 15:19:35]

report what we fell back from

[stefan-webrtc, 2015-10-27 08:31:53]

An option would be to pass the implementation name with the Decoded() and
Encoded() callbacks, that way we would know exactly what frames were encoded and
decoded with what implementation. It would also reduce the number of threads
that access the Encode/Decode callback objects in case of async
encoders/decoders. The down-side is that if the encoding/decoding starts failing
upon a implementation switch, we wouldn't get the implementation reported.

What do you think?

https://codereview.webrtc.org/1406903002/diff/80001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/1406903002/diff/80001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2_unittest.cc:2371:
stats.encoder_implementation_name = "decoder_implementation_name";
s/decoder/encoder

https://codereview.webrtc.org/1406903002/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc
(right):

https://codereview.webrtc.org/1406903002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:270:
return "VideoToolbox";
Maybe it should at least be named H264VideoToolbox? If they end up supporting
other codecs?

https://codereview.webrtc.org/1406903002/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/main/source/generic_encoder.cc (right):

https://codereview.webrtc.org/1406903002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/main/source/generic_encoder.cc:340: void
VCMEncodedFrameCallback::LastEncoderImplementationUsed(
SetLast...?

https://codereview.webrtc.org/1406903002/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/main/source/video_receiver.cc (right):

https://codereview.webrtc.org/1406903002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/main/source/video_receiver.cc:324: return ret;
Did you mean to change this code? Is it only formatting?

[pbos-webrtc, 2015-10-27 12:15:06]

feedback

[pbos-webrtc, 2015-10-27 12:16:59]

I don't want to put it in the Encoded() callback since that requires changing
all existing implementations (no "unknown" default).

I think most implementations will be able to be const on construction or const
after InitEncode, so we shouldn't have to hop onto the codec thread for async
encoders even.

https://codereview.webrtc.org/1406903002/diff/80001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right):

https://codereview.webrtc.org/1406903002/diff/80001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvideoengine2_unittest.cc:2371:
stats.encoder_implementation_name = "decoder_implementation_name";
On 2015/10/27 08:31:53, stefan-webrtc (holmer) wrote:
> s/decoder/encoder

Done.

https://codereview.webrtc.org/1406903002/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc
(right):

https://codereview.webrtc.org/1406903002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc:270:
return "VideoToolbox";
On 2015/10/27 08:31:53, stefan-webrtc (holmer) wrote:
> Maybe it should at least be named H264VideoToolbox? If they end up supporting
> other codecs?

Covered by codec name, we call both VP8 and VP9 "libvpx".

https://codereview.webrtc.org/1406903002/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/main/source/generic_encoder.cc (right):

https://codereview.webrtc.org/1406903002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/main/source/generic_encoder.cc:340: void
VCMEncodedFrameCallback::LastEncoderImplementationUsed(
On 2015/10/27 08:31:53, stefan-webrtc (holmer) wrote:
> SetLast...?

SignalLast.. done.

https://codereview.webrtc.org/1406903002/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/main/source/video_receiver.cc (right):

https://codereview.webrtc.org/1406903002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/main/source/video_receiver.cc:324: return ret;
On 2015/10/27 08:31:53, stefan-webrtc (holmer) wrote:
> Did you mean to change this code? Is it only formatting?

Yes, early-return if (frame == NULL) instead of having an else. Formatting only
really.

[stefan-webrtc, 2015-10-29 07:17:16]

lgtm

[hta-webrtc, 2015-10-29 11:35:03]

Two important things:

- I don't think we should expose encoderImplementationName on outgoing tracks
and decoderImplementationName on incoming tracks. We should just expose
implementationName. 

- Please verify whether this adds another cross-thread hop either per frame or
per stats call, and convince Tommi@ that this is OK.

Otherwise this looks straightforward.

https://codereview.webrtc.org/1406903002/diff/100001/talk/app/webrtc/statstyp...
File talk/app/webrtc/statstypes.cc (left):

https://codereview.webrtc.org/1406903002/diff/100001/talk/app/webrtc/statstyp...
talk/app/webrtc/statstypes.cc:597: break;
Unrelated change, will cause compiler error if it would have been hit, so
probably OK.

https://codereview.webrtc.org/1406903002/diff/100001/talk/app/webrtc/statstyp...
File talk/app/webrtc/statstypes.cc (right):

https://codereview.webrtc.org/1406903002/diff/100001/talk/app/webrtc/statstyp...
talk/app/webrtc/statstypes.cc:412: // Codec 'implementationName' constants
tracked here:
I don't think you want to check in bug URLs in anything but a TODO.

https://codereview.webrtc.org/1406903002/diff/100001/talk/app/webrtc/statstyp...
talk/app/webrtc/statstypes.cc:417: return "encoderImplementationName";
I don't see in the bug my earlier comments that the "encoder/decoder" part is
unneccessary, because they will never occur on the same object.

https://codereview.webrtc.org/1406903002/diff/100001/talk/media/base/mediacha...
File talk/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1406903002/diff/100001/talk/media/base/mediacha...
talk/media/base/mediachannel.h:823: std::string encoder_implementation_name;
the "encoder" part is already implicit in the fact it's a member of a
*SenderInfo class.

https://codereview.webrtc.org/1406903002/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/generic_decoder.h (right):

https://codereview.webrtc.org/1406903002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/generic_decoder.h:48: void
OnDecoderImplementationName(const char* implementation_name);
This implies that collecting implementation name does an extra cross-thread hop
(but I'm not familiar enough with the threading model to say for sure). Please
check with tommi about that, if not already done.

https://codereview.webrtc.org/1406903002/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/generic_encoder.cc (right):

https://codereview.webrtc.org/1406903002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/generic_encoder.cc:159:
encoder_->ImplementationName());
Does this imply that the extra thread-hop to fetch implementation name is done
for each frame?

[pbos-webrtc, 2015-10-29 15:57:51]

rebase

[pbos-webrtc, 2015-10-29 16:08:05]

PTAL. May I call it codecImplementationName for PeerConnection since there's no
RTCCodec object?

https://codereview.webrtc.org/1406903002/diff/100001/talk/app/webrtc/statstyp...
File talk/app/webrtc/statstypes.cc (left):

https://codereview.webrtc.org/1406903002/diff/100001/talk/app/webrtc/statstyp...
talk/app/webrtc/statstypes.cc:597: break;
On 2015/10/29 11:35:02, hta-webrtc wrote:
> Unrelated change, will cause compiler error if it would have been hit, so
> probably OK.

Acknowledged.

https://codereview.webrtc.org/1406903002/diff/100001/talk/app/webrtc/statstyp...
File talk/app/webrtc/statstypes.cc (right):

https://codereview.webrtc.org/1406903002/diff/100001/talk/app/webrtc/statstyp...
talk/app/webrtc/statstypes.cc:412: // Codec 'implementationName' constants
tracked here:
On 2015/10/29 11:35:02, hta-webrtc wrote:
> I don't think you want to check in bug URLs in anything but a TODO.

Removed.

https://codereview.webrtc.org/1406903002/diff/100001/talk/app/webrtc/statstyp...
talk/app/webrtc/statstypes.cc:417: return "encoderImplementationName";
On 2015/10/29 11:35:02, hta-webrtc wrote:
> I don't see in the bug my earlier comments that the "encoder/decoder" part is
> unneccessary, because they will never occur on the same object.

Changed to codecImplementationName for both, implementationName makes sense for
RTCCodec but not for sender, this is not the implementation name for the sender,
it's for the sender's codec.

WDYT?

https://codereview.webrtc.org/1406903002/diff/100001/talk/media/base/mediacha...
File talk/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1406903002/diff/100001/talk/media/base/mediacha...
talk/media/base/mediachannel.h:823: std::string encoder_implementation_name;
On 2015/10/29 11:35:02, hta-webrtc wrote:
> the "encoder" part is already implicit in the fact it's a member of a
> *SenderInfo class.

But we're not doing the implementation name of a sender, it's the encoder
implementation. To unify these we could do codec_implementation_name, but I
think encoder makes sense here. I unified the string.

https://codereview.webrtc.org/1406903002/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/generic_decoder.h (right):

https://codereview.webrtc.org/1406903002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/generic_decoder.h:48: void
OnDecoderImplementationName(const char* implementation_name);
On 2015/10/29 11:35:03, hta-webrtc wrote:
> This implies that collecting implementation name does an extra cross-thread
hop
> (but I'm not familiar enough with the threading model to say for sure). Please
> check with tommi about that, if not already done.

No, this will only be called on the encoding thread that usually accesses the
object. Stats are reported to a proxy object which is protected. There's no
additional hop.

https://codereview.webrtc.org/1406903002/diff/100001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/main/source/generic_encoder.cc (right):

https://codereview.webrtc.org/1406903002/diff/100001/webrtc/modules/video_cod...
webrtc/modules/video_coding/main/source/generic_encoder.cc:159:
encoder_->ImplementationName());
On 2015/10/29 11:35:03, hta-webrtc wrote:
> Does this imply that the extra thread-hop to fetch implementation name is done
> for each frame?

Implementation name is (if not constant) expected to be gathered on InitEncode
at the latest. ImplementationName should be fast and simply return a string.

[pbos-webrtc, 2015-10-29 16:08:39]

hta@ feedback

[pbos-webrtc, 2015-11-10 10:32:32]

On 2015/10/29 16:08:39, pbos-webrtc wrote:
> hta@ feedback

Ping?

[pbos-webrtc, 2015-11-10 12:42:59]

rebase

[hta-webrtc, 2015-11-10 13:47:06]

lgtm based on 7->8 diff and reassurances about thread-hopping.

[pbos-webrtc, 2015-11-10 14:08:20]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-10 14:08:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1406903002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1406903002/160001

[commit-bot: I haz the power, 2015-11-10 14:10:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-10 14:10:24]

Dry run: Try jobs failed on following builders:
  mac_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_x64_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_gn_dbg/builds/5163)

[pbos-webrtc, 2015-11-30 12:18:57]

rebase

[pbos-webrtc, 2015-11-30 12:18:58]

The CQ bit was checked by pbos@webrtc.org

[pbos-webrtc, 2015-11-30 12:18:59]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org,
hta@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1406903002/#ps180001 (title:
"rebase")

[commit-bot: I haz the power, 2015-11-30 12:19:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1406903002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1406903002/180001

[commit-bot: I haz the power, 2015-11-30 12:21:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-30 12:21:50]

Try jobs failed on following builders:
  win_x64_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn_rel/builds/5544)

[pbos-webrtc, 2015-11-30 12:25:14]

remove conflict marker

[pbos-webrtc, 2015-12-18 11:43:01]

PTAL

[mflodman, 2015-12-18 13:02:44]

https://codereview.webrtc.org/1406903002/diff/1/webrtc/modules/video_coding/m...
File webrtc/modules/video_coding/main/source/generic_decoder.cc (right):

https://codereview.webrtc.org/1406903002/diff/1/webrtc/modules/video_coding/m...
webrtc/modules/video_coding/main/source/generic_decoder.cc:164:
_callback->OnDecoderImplementationName(_decoder.ImplementationName());
On 2015/10/22 09:33:34, stefan-webrtc (holmer) wrote:
> On 2015/10/16 13:12:05, pbos-webrtc wrote:
> > On 2015/10/16 07:47:03, stefan-webrtc (holmer) wrote:
> > > Can't we call this only when the implementation actually changes?
> > 
> > Problem is that the interface doesn't allow us to know when we change
> > implementation. This might be a fallback and we don't know when that is
being
> > switched to. If you have a good interface idea for that let me know.
> 
> Acknowledged.

I don't understand why we can't know this, let's discuss.

https://codereview.webrtc.org/1406903002/diff/200001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/video_receiver.cc (right):

https://codereview.webrtc.org/1406903002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_receiver.cc:293: if (frame == NULL)
Maybe !frame when this is being changed anyway and that's the style on line 302.

[pbos-webrtc, 2015-12-18 13:07:21]

frame-nullptr

[pbos-webrtc, 2015-12-18 13:07:55]

https://codereview.webrtc.org/1406903002/diff/1/webrtc/modules/video_coding/m...
File webrtc/modules/video_coding/main/source/generic_decoder.cc (right):

https://codereview.webrtc.org/1406903002/diff/1/webrtc/modules/video_coding/m...
webrtc/modules/video_coding/main/source/generic_decoder.cc:164:
_callback->OnDecoderImplementationName(_decoder.ImplementationName());
On 2015/12/18 13:02:43, mflodman wrote:
> On 2015/10/22 09:33:34, stefan-webrtc (holmer) wrote:
> > On 2015/10/16 13:12:05, pbos-webrtc wrote:
> > > On 2015/10/16 07:47:03, stefan-webrtc (holmer) wrote:
> > > > Can't we call this only when the implementation actually changes?
> > > 
> > > Problem is that the interface doesn't allow us to know when we change
> > > implementation. This might be a fallback and we don't know when that is
> being
> > > switched to. If you have a good interface idea for that let me know.
> > 
> > Acknowledged.
> 
> I don't understand why we can't know this, let's discuss.

Software fallback happens silently inside VideoDecoderFallbackWrapper, so the
VCM on the outside doesn't see that. I think we should merge
VideoDecoderFallbackWrapper into VCMGenericDecoder so we can be aware of when
this change occurs. Does that make sense?

https://codereview.webrtc.org/1406903002/diff/200001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/video_receiver.cc (right):

https://codereview.webrtc.org/1406903002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_receiver.cc:293: if (frame == NULL)
On 2015/12/18 13:02:43, mflodman wrote:
> Maybe !frame when this is being changed anyway and that's the style on line
302.

Done.

[mflodman, 2015-12-18 13:46:15]

LGTM

[pbos-webrtc, 2015-12-18 14:01:26]

fix rebase

[pbos-webrtc, 2015-12-18 14:11:11]

The CQ bit was checked by pbos@webrtc.org

[pbos-webrtc, 2015-12-18 14:11:12]

The patchset sent to the CQ was uploaded after l-g-t-m from hta@webrtc.org,
stefan@webrtc.org, mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1406903002/#ps240001 (title:
"fix rebase")

[commit-bot: I haz the power, 2015-12-18 14:11:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1406903002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1406903002/240001

[pbos-webrtc, 2015-12-18 15:01:29]

Description was changed from

==========
Expose codec implementation names in stats.

Used to distinguish between software/hardware encoders/decoders and
other implementation differences. Useful for tracking quality
regressions related to specific implementations.

BUG=webrtc:4897
R=hta@webrtc.org, mflodman@webrtc.org, stefan@webrtc.org
==========

to

==========
Expose codec implementation names in stats.

Used to distinguish between software/hardware encoders/decoders and
other implementation differences. Useful for tracking quality
regressions related to specific implementations.

BUG=webrtc:4897
R=hta@webrtc.org, mflodman@webrtc.org, stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/b7d9a97ce41022e984348efb5...
==========

[pbos-webrtc, 2015-12-18 15:01:30]

Committed patchset #13 (id:240001) manually as
b7d9a97ce41022e984348efb5f28bf6dd6c6b779 (presubmit successful).

[commit-bot: I haz the power, 2015-12-18 15:01:33]

Description was changed from

==========
Expose codec implementation names in stats.

Used to distinguish between software/hardware encoders/decoders and
other implementation differences. Useful for tracking quality
regressions related to specific implementations.

BUG=webrtc:4897
R=hta@webrtc.org, mflodman@webrtc.org, stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/b7d9a97ce41022e984348efb5...
==========

to

==========
Expose codec implementation names in stats.

Used to distinguish between software/hardware encoders/decoders and
other implementation differences. Useful for tracking quality
regressions related to specific implementations.

BUG=webrtc:4897
R=hta@webrtc.org, mflodman@webrtc.org, stefan@webrtc.org

Committed: https://crrev.com/b7d9a97ce41022e984348efb5f28bf6dd6c6b779
Cr-Commit-Position: refs/heads/master@{#11084}
==========

[commit-bot: I haz the power, 2015-12-18 15:01:34]

Patchset 13 (id:??) landed as
https://crrev.com/b7d9a97ce41022e984348efb5f28bf6dd6c6b779
Cr-Commit-Position: refs/heads/master@{#11084}