Issue 12379011: Interfaces for encoded video sources

Ville-Mikko Rautio

Hi again, Here's follow up CL on 12334120 that will build up abstracted interfaces for ...

7 years, 9 months ago (2013-02-28 11:52:47 UTC) #1

wjia(left Chromium)

https://codereview.chromium.org/12379011/diff/3001/media/base/encoded_bitstream_buffer.cc File media/base/encoded_bitstream_buffer.cc (right): https://codereview.chromium.org/12379011/diff/3001/media/base/encoded_bitstream_buffer.cc#newcode19 media/base/encoded_bitstream_buffer.cc:19: golden_frame_(false), droppable_(false), layer_sync_(false) { one on each line. refer ...

7 years, 9 months ago (2013-02-28 18:46:26 UTC) #2

vmr

https://codereview.chromium.org/12379011/diff/3001/media/base/encoded_bitstream_buffer.cc File media/base/encoded_bitstream_buffer.cc (right): https://codereview.chromium.org/12379011/diff/3001/media/base/encoded_bitstream_buffer.cc#newcode19 media/base/encoded_bitstream_buffer.cc:19: golden_frame_(false), droppable_(false), layer_sync_(false) { On 2013/02/28 18:46:26, wjia wrote: ...

7 years, 9 months ago (2013-03-01 14:23:59 UTC) #3

tommi (sloooow) - chröme

Looks nice and clean in general. Just some minor requests and questions. https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstream_buffer.cc File media/base/encoded_bitstream_buffer.cc ...

7 years, 9 months ago (2013-03-01 16:03:43 UTC) #4

Ami GONE FROM CHROMIUM

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstream_buffer.cc File media/base/encoded_bitstream_buffer.cc (right): https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstream_buffer.cc#newcode38 media/base/encoded_bitstream_buffer.cc:38: golden_frame_(false) { what about droppable_/layer_sync_? This is why we ...

7 years, 9 months ago (2013-03-02 10:47:15 UTC) #5

holmer

One comment. https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstream_buffer.h File media/base/encoded_bitstream_buffer.h (right): https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstream_buffer.h#newcode46 media/base/encoded_bitstream_buffer.h:46: void MarkLayerSync(); Can you comment on these ...

7 years, 9 months ago (2013-03-04 13:36:39 UTC) #6

Ville-Mikko Rautio

Same here. Thanks a lot for all the comments. I addressed some of them, but ...

7 years, 9 months ago (2013-03-04 14:02:24 UTC) #7

Same here. Thanks a lot for all the comments. I addressed
some of them, but I need to carry on tomorrow with more changes and answers.

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
File media/base/encoded_bitstream_buffer.cc (right):

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
media/base/encoded_bitstream_buffer.cc:28:
EncodedBitstreamBuffer::EncodedBitstreamBuffer(uint8* buffer,
On 2013/03/01 16:03:43, tommi wrote:
> const uint8* buffer?
> 
> Actually, would it make sense to transfer ownership of an existing buffer here
> instead of making a new copy?

In the first instance of using this class the buffer will be buffer allocated by
the Browser process and this class is to be used in the Renderer process as
well. Thus, the ownership transfer would not be as straightforward as one could
think in the first place. I think constructor doing ownership transfer cannot be
at least the only constructor for the class.

Constness done.

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
media/base/encoded_bitstream_buffer.cc:38: golden_frame_(false) {
On 2013/03/02 10:47:16, Ami Fischman wrote:
> what about droppable_/layer_sync_?
> 
> This is why we don't write copy/pasta like this :)
> If you have duplicated code, extract it into an Init() function.
> (though, if you take my suggestion and take a metadata struct, this becomes
> moot).

Going for the Metadata.

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
media/base/encoded_bitstream_buffer.cc:84: golden_frame_ = true;
On 2013/03/02 10:47:16, Ami Fischman wrote:
> Is this really how VP8 bitstreams work?

Yes.

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
media/base/encoded_bitstream_buffer.cc:107: base::MD5Digest digest;
On 2013/03/01 16:03:43, tommi wrote:
> this looks like debug only code, right?  Can we exclude it from release
builds?

See the comment on the header.

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
File media/base/encoded_bitstream_buffer.h (right):

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
media/base/encoded_bitstream_buffer.h:17: // General encoded video bitstream
buffer.
On 2013/03/02 10:47:16, Ami Fischman wrote:
> This is not a useful comment.
> Things that are useful to include in class-level comments include: example
> use-cases, lifecycle notes, threading information, and so on.
> (ditto for the other .h file in this CL)

Done.

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
media/base/encoded_bitstream_buffer.h:18: class MEDIA_EXPORT
EncodedBitstreamBuffer :
On 2013/03/02 10:47:16, Ami Fischman wrote:
> Shouldn't the ctor take a EncodingMetadata struct instead of maintaining all
the
> same properties itself?

It could take EncodingMetadata, I added TODO to rebase this one on top of it
once it lands.

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
media/base/encoded_bitstream_buffer.h:18: class MEDIA_EXPORT
EncodedBitstreamBuffer :
On 2013/03/01 16:03:43, tommi wrote:
> this class looks single threaded. Can you either inherit from
> base::NonThreadSafe or add a base::ThreadChecker member and check that methods
> are being called on the correct thread?

I want this class to be readable concurrently from multiple thread contexts.
This class is supposed to be constructed from IPC message coming from another
process and be processed by one or more clients in parallel. To enforce this, I
also took your suggestions to go full const in the accessors.

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
media/base/encoded_bitstream_buffer.h:19: public
base::RefCountedThreadSafe<EncodedBitstreamBuffer> {
On 2013/03/02 10:47:16, Ami Fischman wrote:
> Unless you have a good reason for this to be refcounted, you shouldn't (prefer
> scoped_ptr and explicit ownership to scoped_refptr and shared/vague ownership)

The original purpose of this class has been to carry out buffers from IPC calls
to the sandboxed clients.

As there are many involved threads and amount of data amount is considerable
enough to avoid unnecessary extra deep copies, I chose refcounted to be able to
implement transitional placeholder object who will carry data conveniently from
thread and message loop to another without requiring deep copies and minimal
ownership management.

Now this class works nicely as is:
1. Lifetime of the object is tied to existence of scoped_refptr's (as long as
somebody has the handle on the data).
2. Passing the buffer along threads and message loops without any deep copies is
easy and transparent.

I can't see any problems in the lifetime of the object as long as clients don't
store the scoped_refptr's returned by EVS API for longer than they should. I'll
add extra comment about this into the EVS API.

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
media/base/encoded_bitstream_buffer.h:22: EncodedBitstreamBuffer(size_t size,
base::Time timestamp, int temporal_layer);
On 2013/03/01 16:03:43, tommi wrote:
> nit: const base::Time&

According to Ami copy is more efficient, going with const, though.

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
media/base/encoded_bitstream_buffer.h:22: EncodedBitstreamBuffer(size_t size,
base::Time timestamp, int temporal_layer);
On 2013/03/02 10:47:16, Ami Fischman wrote:
> In what situation will you want this overload?

I thought that this could be a situation for an software encoder, where it wants
to already allocate a buffer for it's output before encoding. But I'll get rid
of it, as software encoder can as well use the other constructor and I decided
to go all const with respect to the buffer contained within this class.

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
media/base/encoded_bitstream_buffer.h:27: base::Time timestamp,
On 2013/03/01 16:03:43, tommi wrote:
> same here

According to Ami copy is more efficient, going with const, though.

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
media/base/encoded_bitstream_buffer.h:31: uint8* buffer() const;
On 2013/03/01 16:03:43, tommi wrote:
> nit: we rarely have const methods that provide write access to the class'
data. 
> Can we either make this:
> const uint8* buffer() const;
> or
> uint8* buffer();

I'll take the full const way to emphasize read-only nature of the buffer for
clients.

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
media/base/encoded_bitstream_buffer.h:33: bool key_frame() const;
On 2013/03/02 10:47:16, Ami Fischman wrote:
> these bool accessors (and members) probably want an is_ prefix

Done.

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
media/base/encoded_bitstream_buffer.h:36: base::Time timestamp() const;
On 2013/03/01 16:03:43, tommi wrote:
> nit: const base::Time& timestamp() const;

Done.

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
media/base/encoded_bitstream_buffer.h:37: int temporal_layer() const;
On 2013/03/02 10:47:16, Ami Fischman wrote:
> _id ?

Done.

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
media/base/encoded_bitstream_buffer.h:42: void MarkKeyFrame();
On 2013/03/02 10:47:16, Ami Fischman wrote:
> s/Mark/Set/ here and below.
> Although since they're simple setters, set_ is probably more correct.

I'm taking these all down for benefit of the EncodingMetadata in the
constructor. Based on the feedback I'm making this class more constant to be
read-only after creation to enforce the thread-safety. These are anyway known
parameters at the time of encoding and should not be required after
initialization.

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
media/base/encoded_bitstream_buffer.h:44: void MarkGolden();
On 2013/03/02 10:47:16, Ami Fischman wrote:
> These make this look like an EncodedVP8BitstreamBuffer, not simply an
> EncodedBitstreamBuffer.

That's right. In fact I had EncodedBitstreamBuffer and specialization for VP8,
VP8EncodedBitstreamBuffer, earlier but I decided to merge them as I had not seen
use case/client for H264 encoder in Chrome. All the code happening for WebRTC
right now is exclusively VP8.

Do you see H264 encoder coming up and how would structure this for generic
support w.r.t IPC and EncodedBitstreamBuffers?

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
media/base/encoded_bitstream_buffer.h:49: virtual ~EncodedBitstreamBuffer(); //
Destructor. Will deallocate the buffers.
On 2013/03/02 10:47:16, Ami Fischman wrote:
> Comment does not add value.
> ~ means destructor
> scoped_ptr<> means |this| owns the member.

Done.

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
media/base/encoded_bitstream_buffer.h:68:
DISALLOW_COPY_AND_ASSIGN(EncodedBitstreamBuffer);
On 2013/03/02 10:47:16, Ami Fischman wrote:
> Add an empty line before this one?

Done.

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
media/base/encoded_bitstream_buffer.h:74: std::ostream& operator<<(std::ostream&
output,
On 2013/03/01 16:03:43, tommi wrote:
> although chromium uses streams for logging, I believe it is generally
> discouraged/disallowed by the style guide.

I know. I just really want to have a way to log these buffers in a uniform way
to ease up troubleshooting the problems we will undoubtedly face with the
production code. Timing dependent protocol issues will be nightmarish to debug
unless we can get meaningful logs from failing runs. Would ToString() member
function work better for such purpose?

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
File media/base/encoded_bitstream_buffer_unittest.cc (right):

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
media/base/encoded_bitstream_buffer_unittest.cc:34: EXPECT_EQ(a->key_frame(),
false);
On 2013/03/01 16:03:43, tommi wrote:
> EXPECT_FALSE(a->key_frame()) ?

Done.

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
media/base/encoded_bitstream_buffer_unittest.cc:44: EXPECT_EQ(a->key_frame(),
true);
On 2013/03/01 16:03:43, tommi wrote:
> EXPECT_TRUE()?

Done.

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
media/base/encoded_bitstream_buffer_unittest.cc:65: }  // namespace media
On 2013/03/02 10:47:16, Ami Fischman wrote:
> Time to rethink this file's existence?

I'm open to removing this as well. On the other hand it's also sort of
placeholder for stuff we might do for SharedMemory handles and other things that
may complicate the class.

Ville-Mikko Rautio

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstream_buffer.h File media/base/encoded_bitstream_buffer.h (right): https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstream_buffer.h#newcode46 media/base/encoded_bitstream_buffer.h:46: void MarkLayerSync(); On 2013/03/04 13:36:40, holmer wrote: > Can ...

7 years, 9 months ago (2013-03-05 13:02:18 UTC) #8

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
File media/base/encoded_bitstream_buffer.h (right):

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
media/base/encoded_bitstream_buffer.h:46: void MarkLayerSync();
On 2013/03/04 13:36:40, holmer wrote:
> Can you comment on these properties? In particular, does MarkAltRef() mean
that
> this frame updates the AltRef reference buffer, or does it mean that it
> references it? Do we need to be able to specify both?

I thought these with "is-a" semantics. Does WebRTC need the reference
information after encoding?

And, as told in comments above, I'll remove Mark* functions and clarify the
"is-a" semantics.

https://codereview.chromium.org/12379011/diff/9006/media/video/encoded_video_...
File media/video/encoded_video_source.h (right):

https://codereview.chromium.org/12379011/diff/9006/media/video/encoded_video_...
media/video/encoded_video_source.h:15: // Class to represent encoded video
bitstream.
On 2013/03/02 10:47:16, Ami Fischman wrote:
> Please make an editorial pass on the comments in this file. 
> (there's a fair bit of obvious things stated, and a fair bit of information
> omitted)

Done.

https://codereview.chromium.org/12379011/diff/9006/media/video/encoded_video_...
media/video/encoded_video_source.h:40: virtual void OnBitstreamConfigChanged(
On 2013/03/01 16:03:43, tommi wrote:
> nit: All the methods in this class have the word "Bitstream" in them. Would it
> make sense to name the class BitstreamClient or BitstreamObserver and change
the
> method names: OnBitstreamStreaming->OnStreaming etc?
> 
> I don't really mind the way it is now but just reading through the methods
> sounds a bit redundant.

Done.

https://codereview.chromium.org/12379011/diff/9006/media/video/encoded_video_...
media/video/encoded_video_source.h:62: };
On 2013/03/02 10:47:16, Ami Fischman wrote:
> Like I wrote in the _messages.h review, this interface seems to be lacking
some
> significant API surface such as Encode(VideoFrame) and lifecycle management ;)

I added fair amount of comments about this. Trying to emphasize that this is not
complete encoder interface... it is more of interface to configure and handle
output from video source that has encoding capability.

https://codereview.chromium.org/12379011/diff/9006/media/video/encoded_video_...
media/video/encoded_video_source.h:83:
base::WeakPtr<EncodedVideoBitstream::Client> client,
On 2013/03/02 10:47:16, Ami Fischman wrote:
> Putting WeakPtr in the API imposes threading constraints.  Please be explicit
> about expected use patterns in class-level comments.

I added some comments on this, but left also TODO to think about the WeakPtr
through again from threading perspective.

holmer

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstream_buffer.h File media/base/encoded_bitstream_buffer.h (right): https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstream_buffer.h#newcode46 media/base/encoded_bitstream_buffer.h:46: void MarkLayerSync(); But what does it mean that it ...

7 years, 9 months ago (2013-03-05 13:32:54 UTC) #9

vmr

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstream_buffer.h File media/base/encoded_bitstream_buffer.h (right): https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstream_buffer.h#newcode46 media/base/encoded_bitstream_buffer.h:46: void MarkLayerSync(); On 2013/03/05 13:32:54, holmer wrote: > But ...

7 years, 9 months ago (2013-03-06 05:51:29 UTC) #10

holmer

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstream_buffer.h File media/base/encoded_bitstream_buffer.h (right): https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstream_buffer.h#newcode46 media/base/encoded_bitstream_buffer.h:46: void MarkLayerSync(); Thanks for clarifying. WebRTC doesn't use it ...

7 years, 9 months ago (2013-03-07 14:20:32 UTC) #11

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
File media/base/encoded_bitstream_buffer.h (right):

https://codereview.chromium.org/12379011/diff/9006/media/base/encoded_bitstre...
media/base/encoded_bitstream_buffer.h:46: void MarkLayerSync();
Thanks for clarifying.

WebRTC doesn't use it explicitly (don't even think it's possible for you to pass
this into WebRTC?). We may use it implicitly though by letting the decoder parse
that information out of the bitstream and then use it for signaling SLI and/or
RPSI messages. That is not currently in use though. Is that an answer to your
question?

I guess it's still not clear to me what the MarkAltRef() and MarkGolden() will
be used for, but now I at least know what they mean. :)

On 2013/03/06 05:51:29, vmr wrote:
> 
> 
> On 2013/03/05 13:32:54, holmer wrote:
> > But what does it mean that it is an AltRef frame? Does it mean that it
> > references the AltRef buffer, that it updates the AltRef buffer or both? And
> > does it mean that it only updates/references the AltRef buffer? Maybe
there's
> a
> > definition in the vp8 spec of what it means if a frame "is an" AltRef, but
> it's
> > not clear to me right now.
> 
> Sorry, I should have been clearer.
> 
> Intention is that the buffer with altref frame flag set updates the alternate
> reference buffer and is the altref reference for future frames. It does not
tell
> anything about the referencing structure used to encode the current frame.
> 
> My follow up question about the referencing structure was that would webrtc
use
> information about exact referencing structure (in addition to temporal layer
id
> plus layer sync and droppable flags)?
> 
> > 
> > On 2013/03/05 13:02:18, Ville-Mikko Rautio wrote:
> > > On 2013/03/04 13:36:40, holmer wrote:
> > > > Can you comment on these properties? In particular, does MarkAltRef()
mean
> > > that
> > > > this frame updates the AltRef reference buffer, or does it mean that it
> > > > references it? Do we need to be able to specify both?
> > > 
> > > I thought these with "is-a" semantics. Does WebRTC need the reference
> > > information after encoding?
> > > 
> > > And, as told in comments above, I'll remove Mark* functions and clarify
the
> > > "is-a" semantics.
> > 
>

Ville-Mikko Rautio

Hey all, This revision integrates CL# 12334120 (IPC + datatypes) with the interfaces in the ...

7 years, 9 months ago (2013-03-14 13:19:20 UTC) #12

Ami GONE FROM CHROMIUM

https://codereview.chromium.org/12379011/diff/77001/content/common/media/encoded_video_source_messages.h File content/common/media/encoded_video_source_messages.h (right): https://codereview.chromium.org/12379011/diff/77001/content/common/media/encoded_video_source_messages.h#newcode12 content/common/media/encoded_video_source_messages.h:12: #define IPC_MESSAGE_START EncodedVideoSourceMsgStart I thought you were going to ...

7 years, 9 months ago (2013-03-18 22:53:44 UTC) #13

Ville-Mikko Rautio

Hey all, Yet another iteration of encoded video source and related IPC. Ville-Mikko https://codereview.chromium.org/12379011/diff/77001/content/common/media/encoded_video_source_messages.h File ...

7 years, 9 months ago (2013-03-19 16:45:43 UTC) #14

Hey all,

Yet another iteration of encoded video source and related IPC.

Ville-Mikko

https://codereview.chromium.org/12379011/diff/77001/content/common/media/enco...
File content/common/media/encoded_video_source_messages.h (right):

https://codereview.chromium.org/12379011/diff/77001/content/common/media/enco...
content/common/media/encoded_video_source_messages.h:12: #define
IPC_MESSAGE_START EncodedVideoSourceMsgStart
On 2013/03/18 22:53:45, Ami Fischman wrote:
> I thought you were going to merge into this CL the code that uses these
> messages.  Both in order to settle otherwise-theoretical debates, and per this
> comment from the other CL: 
> 
> On 2013/03/06 23:35:50, Cris Neckar wrote:
> > We cannot effectively perform a security review of the new IPC messages if
> they
> > are defined prior to being used. Please resubmit the definitions for the
> > messages in the same CLs where you define their handlers.

I agreed offline with Cris that I'll add just empty stubs for now, which I did
now in patch, and include security guys in any follow-up reviews that add the
handlers for these messages.

https://codereview.chromium.org/12379011/diff/77001/content/common/media/enco...
content/common/media/encoded_video_source_messages.h:49: int /* stream_id */,
On 2013/03/18 22:53:45, Ami Fischman wrote:
> How does the renderer get these IDs?  (device & stream)

Added comments on device_id & stream_id.

https://codereview.chromium.org/12379011/diff/77001/content/common/media/enco...
content/common/media/encoded_video_source_messages.h:58:
IPC_MESSAGE_CONTROL2(EncodedVideoSourceHostMsg_RemoveBitstream,
On 2013/03/18 22:53:45, Ami Fischman wrote:
> Now you have Add/Remove -> Added/Removed but Removed also encompasses Error.
> I think these would be better as 
> Create -> Created
> Destroy -> Destroyed
> then if Destroyed encompassed Error it wouldn't be as surprising. 

Changed wording for Add/Remove wording to Create/Destroy.

> But I still think conflating Removed & Error is a bad move.  It'll make
testing
> harder, and it'll make it impossible to write well-behaved clients (which may
be
> happy to see a stream end but unhappy to see it error out).

And also added status code for the bitstream destroyed message to differentiate
between different reasons why it might occur.

https://codereview.chromium.org/12379011/diff/77001/content/common/media/enco...
content/common/media/encoded_video_source_messages.h:65: // not.
On 2013/03/18 22:53:45, Ami Fischman wrote:
> This guarantee is not present in the .h.

Added to the header.

https://codereview.chromium.org/12379011/diff/77001/content/common/media/enco...
content/common/media/encoded_video_source_messages.h:77: // format-specific
manner using enumerations.
On 2013/03/18 22:53:45, Ami Fischman wrote:
> s/format/codec/

Done.

https://codereview.chromium.org/12379011/diff/77001/content/common/media/enco...
content/common/media/encoded_video_source_messages.h:116: // TODO(vmr): Consider
using SharedMemory instead.
On 2013/03/18 22:53:45, Ami Fischman wrote:
> I really think you want this now, not in the future.
> Since shmem is internally refcounted anyway (by the OS) it doesn't make coding
> for multiple clients any harder than it would otherwise be.

Fair enough. Done.

https://codereview.chromium.org/12379011/diff/77001/media/base/encoded_bitstr...
File media/base/encoded_bitstream_buffer.h (right):

https://codereview.chromium.org/12379011/diff/77001/media/base/encoded_bitstr...
media/base/encoded_bitstream_buffer.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2013/03/18 22:53:45, Ami Fischman wrote:
> Drop the (c) (here and elsewhere)
> https://code.google.com/p/chromium/issues/detail?id=140977

Done.

https://codereview.chromium.org/12379011/diff/77001/media/base/encoded_bitstr...
media/base/encoded_bitstream_buffer.h:23: // held by the clients for the
duration of the required processing.
On 2013/03/18 22:53:45, Ami Fischman wrote:
> The sentence of l.20-23 sounds like PR for scoped_refptr, and I feel is almost
> exactly backwards.
> 
> Nobody knows what the lifetime of a refcounted object is supposed to be, much
> less what it actually is.
> RefCountedThreadSafe doesn't buy you anything vis-a-vis deep copies.
> Ref-counting, once added, becomes much more difficult to remove than it would
> have been to avoid in the first place.

I am still defending the use of reference counting mechanism on the buffers. I
think it is very effective way to pass over buffers over thread boundaries and
not worry about ownership of these objects that are for one-shot consumption
only.

> Of course, this is an entirely theoretical argument, because there is no code
in
> this CL that actually instantiates this class.  See my first comment in this
> review.

I have to draw the line somewhere on how wide I am going to expand this CL to
ever get it in, which is why I put stubs into IPC in the latest patch. I am
completely ok to get rid of the reference counting if nobody else sees the
benefits it provides on the follow up CLs that instantiate this class.

https://codereview.chromium.org/12379011/diff/77001/media/base/encoded_bitstr...
media/base/encoded_bitstream_buffer.h:41: const base::Time timestamp() const;
On 2013/03/18 22:53:45, Ami Fischman wrote:
> For this and all the following accessors, why not just have
> const media::BufferEncodingMetadata& metadata() { return metadata_; }
> ?

Done.

https://codereview.chromium.org/12379011/diff/77001/media/base/encoded_bitstr...
media/base/encoded_bitstream_buffer.h:65: const media::EncodedBitstreamBuffer&
b);
On 2013/03/18 22:53:45, Ami Fischman wrote:
> This isn't landable.
> You want a ToDebugString() method instead.

Done.

https://codereview.chromium.org/12379011/diff/77001/media/video/encoded_video...
File media/video/encoded_video_source.h (right):

https://codereview.chromium.org/12379011/diff/77001/media/video/encoded_video...
media/video/encoded_video_source.h:18: // objects representing the same concrete
bitstream from a EncodedVideoSource.
On 2013/03/18 22:53:45, Ami Fischman wrote:
> What is the concrete use-case here?

E.g. considering two peerconnections on the same page looking to stream 720p and
a device that can do 1x720p. The device could still serve both by duplicating
the stream into two separate clients.

https://codereview.chromium.org/12379011/diff/77001/media/video/encoded_video...
media/video/encoded_video_source.h:20: // Client can close the bitstream simply
by discarding the scoped_refptr to the
On 2013/03/18 22:53:45, Ami Fischman wrote:
> How does this work?  The concrete impl of EVB will have a pointer to the
Client,
> and it's quite likely that the Client will not be the only thing that's
holding
> a ref on the EVB impl.  Therefore how do you prevent the EVB from making
further
> calls on Client (which may be gone by then)?

I implemented it by having proxy object (owned by client) derive from EVB and
delegating the functionality to concrete object (owned by EVS). If client goes
away, proxy object will go away and inform the concrete object that the proxy
went away and EVS may choose to destroy the concrete object if no more clients
are there for the concrete object. If concrete object goes away, it informs the
proxy (and consequently client through the proxy) and client can discard the
proxy without causing calls to a dangling pointer.

> The problem would be more apparent if you documented the lifetime/ownership
> requirements on OpenBitstream for the client pointer, which are that Client
must
> outlive the returned EVB, but Client can't guarantee that without owning the
> EVB, and it can't own the EVB while refcounting is happening.

https://codereview.chromium.org/12379011/diff/77001/media/video/encoded_video...
media/video/encoded_video_source.h:28: // After OnStreaming callback stream
shall be considered to be streaming.
On 2013/03/18 22:53:45, Ami Fischman wrote:
> doco modifiable, probably rename to "is_mutable".

Done.

https://codereview.chromium.org/12379011/diff/77001/media/video/encoded_video...
media/video/encoded_video_source.h:28: // After OnStreaming callback stream
shall be considered to be streaming.
On 2013/03/18 22:53:45, Ami Fischman wrote:
> doco modifiable, probably rename to "is_mutable".

Done.

https://codereview.chromium.org/12379011/diff/77001/media/video/encoded_video...
media/video/encoded_video_source.h:46: // change during runtime.
On 2013/03/18 22:53:45, Ami Fischman wrote:
> What about the initial parameters?  Shouldn't they be an arg to OnStreaming?

Original idea was that if parameters given in OpenBitstream are not exactly
obeyed, initialization would fail. Behaviour was changed to "init with closest
possible parameters" and for this reason and your comment I added initial
parameters to OnStreaming.

https://codereview.chromium.org/12379011/diff/77001/media/video/encoded_video...
media/video/encoded_video_source.h:54: // callback has been issued with value
signalling successful change.
On 2013/03/18 22:53:45, Ami Fischman wrote:
> Please rewrite this comment.

Done.

https://codereview.chromium.org/12379011/diff/77001/media/video/encoded_video...
media/video/encoded_video_source.h:90: class MEDIA_EXPORT EncodedVideoSource {
On 2013/03/18 22:53:45, Ami Fischman wrote:
> This is conceptually the entry point to this file, so it should be at the top
of
> the file.

I considered this earlier. Problem with that is inner class
EncodedVideoBitstream::Client which cannot be forward declared, if kept as inner
class. Of course I could move it outside EVB, but I think having it as inner
class in EVB emphasizes the client-server relationship between the EVB and
clients, which carries some value by itself.

https://codereview.chromium.org/12379011/diff/77001/media/video/encoded_video...
media/video/encoded_video_source.h:100: // called with the id. Client should
check the parameters agains limits
On 2013/03/18 22:53:45, Ami Fischman wrote:
> typo: agains

Done.

https://codereview.chromium.org/12379011/diff/77001/media/video/encoded_video...
media/video/encoded_video_source.h:101: // reported by GetCapabilities before
trying to issue an request to add an
On 2013/03/18 22:53:45, Ami Fischman wrote:
> s/an request/a request/

Done.

https://codereview.chromium.org/12379011/diff/77001/media/video/video_encode_...
File media/video/video_encode_types.h (right):

https://codereview.chromium.org/12379011/diff/77001/media/video/video_encode_...
media/video/video_encode_types.h:22: };
On 2013/03/18 22:53:45, Ami Fischman wrote:
> These values are not orthogonal and therefore are not truly flags.  What does
a
> value of 5 mean?
> If you want orthogonal values, make each of the values imply a single bit of
> information.
> If you only need to support key+golden+altref, golden, and altref, make it a
> non-flag enum of those things.

Done.

https://codereview.chromium.org/12379011/diff/77001/media/video/video_encode_...
media/video/video_encode_types.h:37: VideoCodec codec;
On 2013/03/18 22:53:45, Ami Fischman wrote:
> Multi-profile codecs are going to need the profile here.
> (if you're only supporting vp8, then no need for this field; if you're
> contemplating support for other things, put in the right hooks now).

Done.

https://codereview.chromium.org/12379011/diff/77001/media/video/video_encode_...
media/video/video_encode_types.h:39: std::vector<int> frames_per_second;  //
Maximum fps choices for stream.
On 2013/03/18 22:53:45, Ami Fischman wrote:
> s/for stream//

Done.

https://codereview.chromium.org/12379011/diff/77001/media/video/video_encode_...
media/video/video_encode_types.h:55: // also disabled regardless of their
parameters.
On 2013/03/18 22:53:45, Ami Fischman wrote:
> Does that mean that to tell if a layer is disabled one must check its
transitive
> dependencies?  That seems like a bad time.

It says that a layer is disabled, all the direct and transitive dependencies are
automatically disabled as well.

Temporal layers are generally encoded in a way that temporal layers with higher
ids are depending on ones with lower ids. And what I am trying to say, is that
if client in case with three temporal layers (0, 1 and 2) disables layer 1 (by
setting enabled to false for it), it will automatically disable layer 2 as well.

https://codereview.chromium.org/12379011/diff/77001/media/video/video_encode_...
media/video/video_encode_types.h:56: int target_bitrate;  // Target bitrate for
the layer.
On 2013/03/18 22:53:45, Ami Fischman wrote:
> specify units: "in bytes/second" (if that's the case).

Done.

https://codereview.chromium.org/12379011/diff/77001/media/video/video_encode_...
media/video/video_encode_types.h:72: };
On 2013/03/18 22:53:45, Ami Fischman wrote:
> Shouldn't there be a way to indicate that the client doesn't care about given
> field?  IOW I want a certain FPS at a certain bitrate and am willing to let
the
> decoder decide the QP/temporal layer params to achieve my desires.

Done.

https://codereview.chromium.org/12379011/diff/77001/media/video/video_encode_...
media/video/video_encode_types.h:77: VideoCodec codec;
On 2013/03/18 22:53:45, Ami Fischman wrote:
> ditto profile

Done.

https://codereview.chromium.org/12379011/diff/77001/media/video/video_encode_...
media/video/video_encode_types.h:79: int temporal_layer_count;
On 2013/03/18 22:53:45, Ami Fischman wrote:
> this is redundant to runtime_params.temporal_layer_params.size() and should be
> dropped.

Done.

https://codereview.chromium.org/12379011/diff/77001/media/video/video_encode_...
media/video/video_encode_types.h:94: // Pretty print operators to log structures
found in the header conveniently.
On 2013/03/18 22:53:45, Ami Fischman wrote:
> ditto convert to ToDebugString()s on the respective types.

Done.

Ami GONE FROM CHROMIUM

https://codereview.chromium.org/12379011/diff/87001/content/common/media/encoded_video_source_messages.h File content/common/media/encoded_video_source_messages.h (right): https://codereview.chromium.org/12379011/diff/87001/content/common/media/encoded_video_source_messages.h#newcode47 content/common/media/encoded_video_source_messages.h:47: // from choking to the comma. s/to/on/ also, :'( ...

7 years, 9 months ago (2013-03-19 18:01:49 UTC) #15

vmr

https://codereview.chromium.org/12379011/diff/87001/content/common/media/encoded_video_source_messages.h File content/common/media/encoded_video_source_messages.h (right): https://codereview.chromium.org/12379011/diff/87001/content/common/media/encoded_video_source_messages.h#newcode47 content/common/media/encoded_video_source_messages.h:47: // from choking to the comma. On 2013/03/19 18:01:49, ...

7 years, 9 months ago (2013-03-20 12:09:14 UTC) #16

https://codereview.chromium.org/12379011/diff/87001/content/common/media/enco...
File content/common/media/encoded_video_source_messages.h (right):

https://codereview.chromium.org/12379011/diff/87001/content/common/media/enco...
content/common/media/encoded_video_source_messages.h:47: // from choking to the
comma.
On 2013/03/19 18:01:49, Ami Fischman wrote:
> s/to/on/
> also, :'(

Done.

https://codereview.chromium.org/12379011/diff/87001/content/common/media/enco...
content/common/media/encoded_video_source_messages.h:56: // functionality.
Renderer is responsible for generating unique stream_id within
On 2013/03/19 18:01:49, Ami Fischman wrote:
> I think the sentence of l.53-56 is a convoluted way of saying "use the same
> device_id as VideoCapture uses", which emphasizes that this message is
> camera-specific (not generic encoder relevant).
> 
> A key observation, perhaps: when we have generic encoding, the CreateBitstream
> message will probably go to the GPU process, not the browser process.  I
> therefore wonder how generic this proposal is.
> 
> Perhaps while the API in media/video is left generic, this messages should
> acknolwedge that they are in fact all about cameras, not encoders?  (and thus
> move to live near the other VideoCapture messages)
> 
> (the same point, arrived at from the other direction: why do the handlers for
> generic Encode APIs go through Video*Capture*MessageFilter?)

This is probably the least good part of this feature. Back in December in Taipei
we decided to try to define a set of messages that could be used with a encoder
as well as with a encoding camera and we came to conclusion that applications
are not interested in encoding functionality per se, but the output of the
encoding process which is encapsulated in the form of a Bitstream, whose
properties can be manipulated. As we have been working with a camera-based
implementation that perspective on the design got more attention and some
decisions might have been made otherwise to generalize it better.

Anyway the underlying idea that we could have abstracted "bitstream subscription
and control" interface available for applications is valid. I am not sure how to
bend this CL towards that direction right now, though.

https://codereview.chromium.org/12379011/diff/87001/content/common/media/enco...
content/common/media/encoded_video_source_messages.h:115: BitstreamBufferMap /*
buffers */)
On 2013/03/19 18:01:49, Ami Fischman wrote:
> This requires you to decide how many buffers to allocate at bitstream creation
> time, and gives you no way to react to fast/slow consumers of the bitstream.

This is fair point.

> Why not instead have BitstreamReady pass both a shmem handle and a
> (browser-generated) buffer id and then have Consumed take the ID back?

I was thinking about the cost of open+map of a shm handle (which I don't know
and should test). In current way of things they get opened and mapped once,
whereas your proposal requires opening and mapping them each time.

https://codereview.chromium.org/12379011/diff/87001/media/base/encoded_bitstr...
File media/base/encoded_bitstream_buffer.cc (right):

https://codereview.chromium.org/12379011/diff/87001/media/base/encoded_bitstr...
media/base/encoded_bitstream_buffer.cc:19: : shm_(new base::SharedMemory(handle,
true)),
On 2013/03/19 18:01:49, Ami Fischman wrote:
> Why is this a pointer and not a regular member?

No good reason. Changed to member variable.

https://codereview.chromium.org/12379011/diff/87001/media/base/encoded_bitstr...
media/base/encoded_bitstream_buffer.cc:22: if (!shm_->Map(size_))
On 2013/03/19 18:01:49, Ami Fischman wrote:
> CHECK-fail instead?

Done.

https://codereview.chromium.org/12379011/diff/87001/media/base/encoded_bitstr...
media/base/encoded_bitstream_buffer.cc:30: return static_cast<const
uint8*>(shm_->memory());
On 2013/03/19 18:01:49, Ami Fischman wrote:
> s/static/reinterpret/

Done.

https://codereview.chromium.org/12379011/diff/87001/media/base/encoded_bitstr...
media/base/encoded_bitstream_buffer.cc:65: " time: %.2f",
On 2013/03/19 18:01:49, Ami Fischman wrote:
> nit: 0.3 gets you milliseconds; not sure if we have any other logging-related
> stuff that emits centi-seconds :)

Done.

https://codereview.chromium.org/12379011/diff/87001/media/base/encoded_bitstr...
File media/base/encoded_bitstream_buffer.h (right):

https://codereview.chromium.org/12379011/diff/87001/media/base/encoded_bitstr...
media/base/encoded_bitstream_buffer.h:41: media::BufferEncodingMetadata
metadata() const;
On 2013/03/19 18:01:49, Ami Fischman wrote:
> Am I missing something or could this return a const-ref instead of a copy?

It could. I returned by value to make 100 % sure somebody won't end up meddling
with the member values. I changed it to const ref.

https://codereview.chromium.org/12379011/diff/87001/media/video/encoded_video...
File media/video/encoded_video_source.h (right):

https://codereview.chromium.org/12379011/diff/87001/media/video/encoded_video...
media/video/encoded_video_source.h:28: enum StatusCode {
On 2013/03/19 18:01:49, Ami Fischman wrote:
> this name is unfortunately general.
> DestructionReason
> ?

SGTM. Done.

https://codereview.chromium.org/12379011/diff/87001/media/video/encoded_video...
media/video/encoded_video_source.h:41: // successfully created and is streaming.
Parameter modifiable lets the
On 2013/03/19 18:01:49, Ami Fischman wrote:
> s/modifiable/is_mutable/

Done.

https://codereview.chromium.org/12379011/diff/87001/media/video/encoded_video...
media/video/encoded_video_source.h:42: // client know, whether it is allowed to
change the runtime configuration of
On 2013/03/19 18:01:49, Ami Fischman wrote:
> s/,//

Done.

https://codereview.chromium.org/12379011/diff/87001/media/video/encoded_video...
media/video/encoded_video_source.h:43: // the bitstream.
On 2013/03/19 18:01:49, Ami Fischman wrote:
> If two clients A & B open the same stream, and A gets is_mutable=true and B
gets
> is_mutable=false, and then A destroys the bitstream, B is unfortunately unable
> to modify the bitstream.  I wonder how much value there is to exposing the
> modifability bit to the client here, as opposed to maintaining modifiability
in
> the browser process, using a "mutable" token per device which only one client
> can own, and moving the token around as clients depart.

It's a corner case and could be handled just by rejecting TryConfigure in
implementation from clients that aren't supposed to modify the properties (until
they are :).

https://codereview.chromium.org/12379011/diff/87001/media/video/encoded_video...
media/video/encoded_video_source.h:44: virtual void OnStreaming(
On 2013/03/19 18:01:49, Ami Fischman wrote:
> On 2013/03/19 16:45:43, Ville-Mikko Rautio wrote:
> > On 2013/03/18 22:53:45, Ami Fischman wrote:
> > > What about the initial parameters?  Shouldn't they be an arg to
OnStreaming?
> > 
> > Original idea was that if parameters given in OpenBitstream are not exactly
> > obeyed, initialization would fail. Behaviour was changed to "init with
closest
> > possible parameters" and for this reason and your comment I added initial
> > parameters to OnStreaming.
> 
> I don't see this here (but it is present in the message).

Done.

https://codereview.chromium.org/12379011/diff/87001/media/video/video_encode_...
File media/video/video_encode_types.cc (right):

https://codereview.chromium.org/12379011/diff/87001/media/video/video_encode_...
media/video/video_encode_types.cc:14: return debug_string;
On 2013/03/19 18:01:49, Ami Fischman wrote:
> fwiw 
> return StringPrintf("%i...%i@%i", min, max, step);
> saves a couple of lines.

Done.

https://codereview.chromium.org/12379011/diff/87001/media/video/video_encode_...
File media/video/video_encode_types.h (right):

https://codereview.chromium.org/12379011/diff/87001/media/video/video_encode_...
media/video/video_encode_types.h:22: kVP9GoldenAndAltrefFrame,  // Golden and
altref refresh without key frame.
On 2013/03/19 18:01:49, Ami Fischman wrote:
> whoah vp9??

I've looked at this for too long and getting blind to it. :)

https://codereview.chromium.org/12379011/diff/87001/media/video/video_encode_...
media/video/video_encode_types.h:43: VideoCodecProfile profile;
On 2013/03/19 18:01:49, Ami Fischman wrote:
> Having profile means you don't need codec.

Done.

https://codereview.chromium.org/12379011/diff/87001/media/video/video_encode_...
media/video/video_encode_types.h:85: VideoCodec codec;
On 2013/03/19 18:01:49, Ami Fischman wrote:
> ditto drop codec in favor of profile.

Done.

Cris Neckar

Thank you for including TODOs for handler security reviews. LGTM for now on the new ...

7 years, 9 months ago (2013-03-20 19:51:48 UTC) #17

tommi (sloooow) - chröme

looks good overall. I would like to not include the ToDebugString() methods in official builds ...

7 years, 9 months ago (2013-03-21 14:48:05 UTC) #18

Ami GONE FROM CHROMIUM

7 years, 9 months ago (2013-03-21 18:25:45 UTC) #19

Per off-line convo, this CL will be closed unsubmitted as project priorities
have shifted.  Rietveld will keep a copy of the code for if/when we need it.

Issue 12379011: Interfaces for encoded video sources (Closed)

Description

Patch Set 1 #

Patch Set 2 : Changed Client to be accessed through WeakPtr #

Patch Set 3 : EncodedBitstreamBuffer now holds scoped_ptr + minor fixes #

Patch Set 4 : EncodedBitstreamBuffer now holds scoped_ptr and some minor fixes #

Patch Set 5 : Stylization #

Patch Set 6 : Addressed some of the comments #

Patch Set 7 : Further refinement + includes now IPC changes as well #

Patch Set 8 : Added default constructors and destructors for complex structs #

Patch Set 9 : Compile error fix #

Patch Set 10 : More compilation fixes #

Patch Set 11 : Add MEDIA_EXPORT for video encode types #

Patch Set 12 : Further refining interfaces and data types. #

Patch Set 13 : Further refinement, pulled in data types and IPC from 12334120 #

Patch Set 14 : Fixed includes and improves comments #

Patch Set 15 : Pruning includes and unnecessary unit tests #

Patch Set 16 : #

Patch Set 17 : Build fixes #

Patch Set 18 : Add/Remove=>Create/Destroy, IPC stubs, renaming, copyright fixes #

Patch Set 19 : style nit #

Patch Set 20 : Rebase #

Patch Set 21 : Further all-around refinement #

Patch Set 22 : #

Patch Set 23 : #

Patch Set 24 : #

Patch Set 25 : #

Messages