Implement scoped_const_refptr template.

Implement scoped_const_refptr template.

The objectives of this hack are:

1. To make it possible to make good use of use const for refcounted
   arguments that are not modified by the method/function.

2. Keep using the const scoped_refptr<Type>& style, to avoid refcount
   updates for the call, but extend it const-declare the referred-to
   object.

3. Not impose any additional storage or runtime overhead.

4. Make it possible to pass a |const scoped_refptr<Type>&| to a
   function expecting a |const scoped_const_refptr<Type>&|, with
   automatic type conversion, and *without* constructing any temporary
   object.

The last requirement is the tricky one. It looks easy to just use
|const scoped_refptr<const Type>&|, but as far as I understand, this
would require a new temporary object and corersponding refcount
updates. The only way I've found (suggested by perkj) to get automatic
conversion and pass a reference to the same ptr object, is to
introduce scoped_const_refptr as a base class of scoped_ref_ptr.

And then requirement (3) implies that the sub class scoped_refptr must
use the same pointer member as the base class, using some ugly but
safe const casts.

All comments appreciated. It's not a really pure and kosher hack, but
I think the benefit, of being able to mark ref counted arguments as
const where appropriate, may be worth it.

Also note that for this to work, AddRef and Release most be const
methods, and the actual refcount declared as mutable. That's already
fixed for the main RefCountedObject base class, but needed some fixes
for older code.

As an initial test of the code, it is used for the source arguments to
some of the I420Buffer methods.

BUG=

[nisse-webrtc, 2016-06-21 10:24:07]

nisse@webrtc.org changed reviewers:
+ kwiberg@webrtc.org, perkj@webrtc.org, tommi@webrtc.org

[nisse-webrtc, 2016-06-21 10:24:07]

This is an experimental cl to make it possible to improve const type checking
for refcounted object. Nothing urgent, but if you think it's a good idea (or if
you see a better way to achieve the same objectives), it would be nice to get
something like this done in the coming months.

[kwiberg-webrtc, 2016-08-11 13:45:12]

On 2016/06/21 10:24:07, nisse-webrtc (ooo August 15) wrote:
> This is an experimental cl to make it possible to improve const type checking
> for refcounted object. Nothing urgent, but if you think it's a good idea (or
if
> you see a better way to achieve the same objectives), it would be nice to get
> something like this done in the coming months.

Do we really pass scoped_refptr<T>& arguments (that we'd like to turn into
scoped_refptr<const T>&) in enough places that this is worth doing? When
answering, note that a bunch of places where we currently pass such parameters
are likely to resemble the code you're changing in video_frame_buffer.cc in that
they really ought to pass e.g. T* or const T& arguments instead, on account of
them only looking at/manipulating the object and not the smart pointer that owns
it. Of those that genuinely do take (shared) ownership of the parameter, many
will do so unconditionally, in which case the smart pointer can be passed by
value (which results in no extra reference count changes because move
construction). Only in the case where a method conditionally takes ownership of
the parameter is passing a scoped_refptr<T>& a good idea, so the set of methods
that would motivate this CL are those that conditionally take ownership of a
scoped_refptr to an object that they're only going to use in a const manner.

[nisse-webrtc, 2016-08-15 07:33:29]

On 2016/08/11 13:45:12, kwiberg-webrtc wrote:
> they really ought to pass e.g. T* or const T& arguments instead, on account of
> them only looking at/manipulating the object and not the smart pointer that
owns
> it.

const T& makes some sense to me. I discussed this with magjed and tommi before
vacation, and if I recall correctly, tommi favored always using
scoped_refptr<T>&.

One case is frame manipulation functions which also return a scoped_refptr,
where the return value may point to the input argument, or to a newly created
object. In this case, the input argument can't be a raw pointer. I have to check
some more context to see if this happens for frame arguments that ought to be
const, but I intend to move towards frames generally being const, initialized at
construction and never modified.

[kwiberg-webrtc, 2016-08-15 07:41:02]

On 2016/08/15 07:33:29, nisse-webrtc (ooo August 15) wrote:
> On 2016/08/11 13:45:12, kwiberg-webrtc wrote:
> > they really ought to pass e.g. T* or const T& arguments instead, on account
of
> > them only looking at/manipulating the object and not the smart pointer that
> owns
> > it.
> 
> const T& makes some sense to me. I discussed this with magjed and tommi before
> vacation, and if I recall correctly, tommi favored always using
> scoped_refptr<T>&.

Hmm. I wonder if that's on account of the bad habit of parts of the code to
create scoped_refptrs to raw pointers just because it happens to work?

> One case is frame manipulation functions which also return a scoped_refptr,
> where the return value may point to the input argument, or to a newly created
> object. In this case, the input argument can't be a raw pointer.

Well, technically it can, but I agree that it really really shouldn't.

> I have to check
> some more context to see if this happens for frame arguments that ought to be
> const, but I intend to move towards frames generally being const, initialized
at
> construction and never modified.

The constness of the object isn't what matters---you can pass mutable objects as
T*---what matters is if the callee wants to do something with the reference
count. If and only if it needs to mess with that should the argument be passed
as any sort of scoped_refptr.

IMHO, of course. :-)

[nisse-webrtc, 2016-09-26 08:31:33]

I'm closing this cl. Style guide now discourages use of pointers and references
to smart pointers
(https://www.chromium.org/developers/smart-pointer-guidelines), and then I think
we can just use scoped_refptr<const T> wherever appropriate. 

May still be relevant to change AddRef and Release to be const methods
everywhere, but that can be changed one place at a time when needed.