Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1577953002:
Motown in-proc streaming framework used to implement media services. (Closed)
DescriptionMotown in-proc streaming framework used to implement media services.
Includes the engine that hosts the various 'parts' that make up in-proc
streaming graphs, as well as the parts that aren't dependendent on
ffmpeg or mojo. framework_create is where ffmpeg and other technologies
will be integrated. For now, the 'Create' functions simply fail.
R=johngro@google.com
Committed: https://chromium.googlesource.com/external/mojo/+/bd2a17d65fabcdb7785c67b43769e5a52cf6f8aa
Patch Set 1 #Patch Set 2 : removed build/util/LASTCHANGE #
Total comments: 52
Patch Set 3 : Changed lpcm input/output to use packets for supplying frames. Some name changes. Synced to master. #
Total comments: 26
Patch Set 4 : Addressed feedback including non-const ref parameters. #
Total comments: 74
Patch Set 5 : Various fixes based on feedback. #
Total comments: 26
Patch Set 6 : Fixes based on feedback. #Patch Set 7 : Fixes based on feedback. #
Total comments: 42
Patch Set 8 : Sync, updates based on feedback, some functions declared const. #
Total comments: 129
Patch Set 9 : Many changes in response to feedback. Sync. #Patch Set 10 : sync #Messages
Total messages: 22 (3 generated)
Description was changed from ========== Motown in-proc streaming framework used to implement media services. Includes the engine that hosts the various 'parts' that make up in-proc streaming graphs, as well as the parts that aren't dependendent on ffmpeg or mojo. framework_create is where ffmpeg and other technologies will be integrated. For now, the 'Create' functions simply fail. ========== to ========== Motown in-proc streaming framework used to implement media services. Includes the engine that hosts the various 'parts' that make up in-proc streaming graphs, as well as the parts that aren't dependendent on ffmpeg or mojo. framework_create is where ffmpeg and other technologies will be integrated. For now, the 'Create' functions simply fail. ==========
dalesat@chromium.org changed reviewers: + jamesr@chromium.org, jeffbrown@google.com, johngro@google.com
publishing comments in progress. More to come https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/allocator.cc (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/allocator.cc:34: static DefaultAllocator default_allocator; I think that static-scoped classes are supposed to be marked constexpr to insure that they have no runtime initialization. Static and global classes with runtime initialization are disallowed by style to prevent initialization ordering bugs. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/allocator.h (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/allocator.h:21: virtual void* AllocatePayloadBuffer(uint64_t size) = 0; <nit> for in-proc non-mojom'ed stuff, we should use size_t instead of u64 here. </nit> https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/conversion_pipeline_builder.cc (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/conversion_pipeline_builder.cc:192: NOTREACHED() << "conversion requires mixdown/up - not supported"; why not just LOG(ERROR) or MOJO_LOG(ERROR) instead of NOTREACHED? is it that the not reached form ASSERTs in the process? https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/conversion_pipeline_builder.h (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/conversion_pipeline_builder.h:19: bool BuildConversionPipeline( re: style We are not supposed to be passing mutable references (engine), only const references. In theory, we are supposed to pass pointers instead when using in/out parameters. Personally, I think that this requirements is silly since the reference syntax makes it very difficult to misconstrue the intent of the author and believe that a pointer parameter is optional and may be nullptr. The style argument is that C++ provides no way at the invocation site to know if you are passing a value or a reference. Either way, we are supposed to shift to a pointer here instead of a ref. re: design It feels like I am missing something here... Why do we need in_type? It's the type of output's output stream which is going to be fed into the rest of the constructed graph, yes? If so, why do we need to specify it? Shouldn't it be a property of the output already? Is it that the output might support multiple output types and has not been configured for a particular one yet? https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/engine.cc (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/engine.cc:10: uint32_t Engine::Part::input_count() { Would it be a good idea to inline most of these accessors? https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/engine.cc:16: DCHECK(stage_ != nullptr && index < stage_->input_count()); FWIW, the 2 parameter form of the Input constructor already DCHECKs this (same for output below) https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/engine.cc:93: std::unique_ptr<Stage> to_remove(stage); // Release this duplicate unique_ptr! something feel wrong about this... if stages_ is a list of unique ptrs, then you should not be able to be holding a pointer to anything that the unique pointer points to. Other objects could hold a const std::unique_ptr<>&, provided you are sure that the other objects will have a shorter life than the unique pointer itself, but (in theory) you should never be manipulating the naked pointer at all, since it violates the unique-ness of the pointer and the lifetime management safety that unique_ptr is supposed to provide. the fact that the same stage pointer is a member of the sources_ and sinks_ collection while also being part of the stages_ collection seems to highlight this point. stages_ is a collection of unique pointers while sources_ and sinks_ are naked pointer which can point to the same thing as the pointers in stages_. IOW - the pointers held in the stages_ collection seem to be far from unique. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/engine.cc:234: stages_.push_back(std::unique_ptr<Stage>(stage)); related to the question about unique pointers earler; why is this becoming unique now? Isn't the proper pattern to make it unique at construction time and then to follow move-only semantics everywhere? https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/engine.cc:284: while (true) { something to consider moving forward; If this is running on a mojo message loop, the loop is being blocked while this is being run. Re-queueing update jobs to the loop and un-winding out would allow other messages to be processed while still running to completion. If this is meant to be run on a dedicated thread, then some attention will need to be paid to some form of external "shutdown now" signal so we can shut down promptly without needing to run the processing loop to completion. Given the nature of the system (where stages and queue more work during the update call), we cannot prove that this will ever run to completion, so it seems important for there be a way to externally trigger a clean shutdown of the engine. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/engine.cc:338: Stage* stage = supply_backlog_.front(); the comment above indicates that this is supposed to be a stack (LIFO), but the code here is pushing to the back and popping from the front, which is a queue (FIFO) not a stack. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/engine.h (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/engine.h:33: Input input(uint32_t index); re: the accessors here which are making copies of Inputs, Outputs and Parts Is there an advantage to doing this? Why not just return a const& and let the caller make a copy using the default copy constructor if they need a mutable copy? https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/engine.h:205: void reset(); Reset
https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/conversion_pipeline_builder.cc (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/conversion_pipeline_builder.cc:192: NOTREACHED() << "conversion requires mixdown/up - not supported"; On 2016/01/20 00:25:32, johngro wrote: > why not just LOG(ERROR) or MOJO_LOG(ERROR) instead of NOTREACHED? is it that > the not reached form ASSERTs in the process? The NOTREACHED will be removed as part of addressing the TODO. In the mean time, I want the code to fail noisily if I try to make it do things that aren't implemented. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/conversion_pipeline_builder.h (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/conversion_pipeline_builder.h:19: bool BuildConversionPipeline( On 2016/01/20 00:25:32, johngro wrote: > re: style > We are not supposed to be passing mutable references (engine), only const > references. In theory, we are supposed to pass pointers instead when using > in/out parameters. Personally, I think that this requirements is silly since > the reference syntax makes it very difficult to misconstrue the intent of the > author and believe that a pointer parameter is optional and may be nullptr. The > style argument is that C++ provides no way at the invocation site to know if you > are passing a value or a reference. Either way, we are supposed to shift to a > pointer here instead of a ref. > > re: design > It feels like I am missing something here... Why do we need in_type? It's the > type of output's output stream which is going to be fed into the rest of the > constructed graph, yes? If so, why do we need to specify it? Shouldn't it be a > property of the output already? Is it that the output might support multiple > output types and has not been configured for a particular one yet? Regarding references, I understand and will comply. The benefit of using a pointer for in/out is clear...not so much when the intent is to pass a mutable object. I haven't disallowed engine copies, but I easily could, in which case the intent of passing by non-const reference would be clear. Regarding in_type, Engine knows nothing about StreamTypes, and I want to keep it that way. The Engine::Part/Input/Output stuff provides no way to interrogate an output for its type. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/engine.cc (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/engine.cc:284: while (true) { On 2016/01/20 00:25:32, johngro wrote: > something to consider moving forward; > > If this is running on a mojo message loop, the loop is being blocked while this > is being run. Re-queueing update jobs to the loop and un-winding out would > allow other messages to be processed while still running to completion. > > If this is meant to be run on a dedicated thread, then some attention will need > to be paid to some form of external "shutdown now" signal so we can shut down > promptly without needing to run the processing loop to completion. Given the > nature of the system (where stages and queue more work during the update call), > we cannot prove that this will ever run to completion, so it seems important for > there be a way to externally trigger a clean shutdown of the engine. Agreed. The threading model isn't complete. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/engine.h (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/engine.h:33: Input input(uint32_t index); On 2016/01/20 00:25:32, johngro wrote: > re: the accessors here which are making copies of Inputs, Outputs and Parts > > Is there an advantage to doing this? Why not just return a const& and let the > caller make a copy using the default copy constructor if they need a mutable > copy? The intent is that Part, Input and Output are value types comparable to iterators. Part is pointer-sized and there's no benefit to passing it by reference. Input and Output are pointer + 32 bits, so your suggestion has some benefit for those. Regardless, I don't want to introduce any concerns about lifetime.
publishing more comments so I can put this revision down and move onto the next one https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/lpcm_util.cc (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/lpcm_util.cc:38: void LpcmUtilImpl<T>::Silence(void* buffer, uint64_t frame_count) const { for the cases of filling a non-float, signed buffer with zeros or filling a uint8_t buffer with 0x80, ::memset is probably a better option than a for loop as the environment's std library implementation of memset can pull some nice tricks for speed's sake. float and multibyte unisgned int types probably still need a for loop. Depending on how much typing you want to do, you could use type traits to separate out the memset version and the for loop versions (the memset version should run if T is uint8_t or if T is a signed integral type) https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/lpcm_util.cc:108: uint64_t in_channel_stride = in_byte_count / channels; see comment in header re: in_byte_count. That said, if we are keeping in_byte_count, then we should ASSERT both channels and !(in_byte_count % channels) https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/lpcm_util.h (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/lpcm_util.h:16: static LpcmUtil* Create(const LpcmStreamType& stream_type); virtual ~LpcmUtil() { } https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/lpcm_util.h:29: void* in, const void* for the in param, here and below https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/lpcm_util.h:30: uint64_t in_byte_count, in_byte_count seems redundant here. Don't we know everything we need to know (number of channels in a frame, number of bytes in a sample) from the stream type which was specified at creation time? It this to allow us to interleave only part of a non-interleaved packet? If so, how do we manage to interleave the rest of the data later on (we will be offset into the first "plane" and subsequent planes will be larger) https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/lpcm_util.h:37: class LpcmUtilImpl : public LpcmUtil { In theory, you can bury the Impl class definition inside of the .cc file, since everyone should be using LpcmUtil to create their Utils instances anyway. This will give you the freedom to have all sorts of implementations, not just the ones based on this particular template. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/lpcm_util.h:39: LpcmUtilImpl(const LpcmStreamType& stream_type); probably want to make this private and make the interface class a friend so that everyone is forced to use the interface's Create method. Also might want to consider having Create return a std::unique_ptr
https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/allocator.cc (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/allocator.cc:34: static DefaultAllocator default_allocator; On 2016/01/20 00:25:32, johngro wrote: > I think that static-scoped classes are supposed to be marked constexpr to insure > that they have no runtime initialization. Static and global classes with > runtime initialization are disallowed by style to prevent initialization > ordering bugs. I'm not sure what to do here in that case. I can't declare default_allocator constexpr, because GetDefault returns a non-const pointer. This is true even when I make the constructor constexpr. The compiler won't allow me to also make the method implementations constexpr. I could lazy-create the default allocator, but would require a lock. Suggestions? https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/conversion_pipeline_builder.h (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/conversion_pipeline_builder.h:19: bool BuildConversionPipeline( On 2016/01/25 17:47:29, dalesat wrote: > On 2016/01/20 00:25:32, johngro wrote: > > re: style > > We are not supposed to be passing mutable references (engine), only const > > references. In theory, we are supposed to pass pointers instead when using > > in/out parameters. Personally, I think that this requirements is silly since > > the reference syntax makes it very difficult to misconstrue the intent of the > > author and believe that a pointer parameter is optional and may be nullptr. > The > > style argument is that C++ provides no way at the invocation site to know if > you > > are passing a value or a reference. Either way, we are supposed to shift to a > > pointer here instead of a ref. > > > > re: design > > It feels like I am missing something here... Why do we need in_type? It's the > > type of output's output stream which is going to be fed into the rest of the > > constructed graph, yes? If so, why do we need to specify it? Shouldn't it be > a > > property of the output already? Is it that the output might support multiple > > output types and has not been configured for a particular one yet? > > Regarding references, I understand and will comply. The benefit of using a > pointer for in/out is clear...not so much when the intent is to pass a mutable > object. I haven't disallowed engine copies, but I easily could, in which case > the intent of passing by non-const reference would be clear. > > Regarding in_type, Engine knows nothing about StreamTypes, and I want to keep it > that way. The Engine::Part/Input/Output stuff provides no way to interrogate an > output for its type. Done. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/engine.cc (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/engine.cc:10: uint32_t Engine::Part::input_count() { On 2016/01/20 00:25:32, johngro wrote: > Would it be a good idea to inline most of these accessors? These are only used for graph building, so they aren't called at high frequency. The memory penalty of inline is probably not justified for these. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/engine.cc:93: std::unique_ptr<Stage> to_remove(stage); // Release this duplicate unique_ptr! On 2016/01/20 00:25:32, johngro wrote: > something feel wrong about this... > > if stages_ is a list of unique ptrs, then you should not be able to be holding a > pointer to anything that the unique pointer points to. Other objects could hold > a const std::unique_ptr<>&, provided you are sure that the other objects will > have a shorter life than the unique pointer itself, but (in theory) you should > never be manipulating the naked pointer at all, since it violates the > unique-ness of the pointer and the lifetime management safety that unique_ptr is > supposed to provide. > > the fact that the same stage pointer is a member of the sources_ and sinks_ > collection while also being part of the stages_ collection seems to highlight > this point. stages_ is a collection of unique pointers while sources_ and > sinks_ are naked pointer which can point to the same thing as the pointers in > stages_. IOW - the pointers held in the stages_ collection seem to be far from > unique. A good analogy is that engine is a container like std::list, Stage is like a list node, and Part, Input and Output are like iterators. I used unique_ptr as an expedient so the stages get deleted automatically. I understand this flies in the face of unique_ptr dogma, so I'll just use raw pointers. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/engine.h (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/engine.h:33: Input input(uint32_t index); On 2016/01/25 17:47:29, dalesat wrote: > On 2016/01/20 00:25:32, johngro wrote: > > re: the accessors here which are making copies of Inputs, Outputs and Parts > > > > Is there an advantage to doing this? Why not just return a const& and let the > > caller make a copy using the default copy constructor if they need a mutable > > copy? > > The intent is that Part, Input and Output are value types comparable to > iterators. Part is pointer-sized and there's no benefit to passing it by > reference. Input and Output are pointer + 32 bits, so your suggestion has some > benefit for those. Regardless, I don't want to introduce any concerns about > lifetime. Also, Input and Output are synthesized by these methods (from a stage pointer and an input/output index). There's nothing to reference if we wanted to return a reference. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/engine.h:205: void reset(); On 2016/01/20 00:25:32, johngro wrote: > Reset Done. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/lpcm_util.cc (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/lpcm_util.cc:38: void LpcmUtilImpl<T>::Silence(void* buffer, uint64_t frame_count) const { On 2016/01/25 18:55:43, johngro wrote: > for the cases of filling a non-float, signed buffer with zeros or filling a > uint8_t buffer with 0x80, ::memset is probably a better option than a for loop > as the environment's std library implementation of memset can pull some nice > tricks for speed's sake. > > float and multibyte unisgned int types probably still need a for loop. > > Depending on how much typing you want to do, you could use type traits to > separate out the memset version and the for loop versions (the memset version > should run if T is uint8_t or if T is a signed integral type) Done. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/lpcm_util.cc:108: uint64_t in_channel_stride = in_byte_count / channels; On 2016/01/25 18:55:43, johngro wrote: > see comment in header re: in_byte_count. That said, if we are keeping > in_byte_count, then we should ASSERT both channels and !(in_byte_count % > channels) Done. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/lpcm_util.h (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/lpcm_util.h:16: static LpcmUtil* Create(const LpcmStreamType& stream_type); On 2016/01/25 18:55:43, johngro wrote: > virtual ~LpcmUtil() { } Done. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/lpcm_util.h:29: void* in, On 2016/01/25 18:55:43, johngro wrote: > const void* for the in param, here and below Done for all inputs. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/lpcm_util.h:30: uint64_t in_byte_count, On 2016/01/25 18:55:43, johngro wrote: > in_byte_count seems redundant here. Don't we know everything we need to know > (number of channels in a frame, number of bytes in a sample) from the stream > type which was specified at creation time? It this to allow us to interleave > only part of a non-interleaved packet? If so, how do we manage to interleave > the rest of the data later on (we will be offset into the first "plane" and > subsequent planes will be larger) The layout of non-interleaved samples we get from ffmpeg is such that the payload is evenly divided into a buffer for each frame (so each buffer is in_byte_count / frames_per_sample in size). The samples for a frame are contiguous in the buffer starting at the beginning of the buffer, but they don't necessarily fill the buffer. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/lpcm_util.h:37: class LpcmUtilImpl : public LpcmUtil { On 2016/01/25 18:55:43, johngro wrote: > In theory, you can bury the Impl class definition inside of the .cc file, since > everyone should be using LpcmUtil to create their Utils instances anyway. This > will give you the freedom to have all sorts of implementations, not just the > ones based on this particular template. Done. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/lpcm_util.h:39: LpcmUtilImpl(const LpcmStreamType& stream_type); On 2016/01/25 18:55:43, johngro wrote: > probably want to make this private and make the interface class a friend so that > everyone is forced to use the interface's Create method. Also might want to > consider having Create return a std::unique_ptr Done.
got down to parts/lpcm_refomatter. Will resume in the morning, working on PS#4 https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... File services/media/framework/metadata.h (right): https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/metadata.h:19: class Metadata { As a top level design thing, this class seems to be a starting point for asset metadata (either a live stream or from a static asset). As time goes on, this is going to need to become more flexible than what we have here, and may even need to become dynamic. https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/metadata.h:30: Metadata( If the intention here is to make this class the type of thing which people need to manipulate via unique_ptr patterns (IOW - use only New, Clone and std::move; disallow implicit copying and stack allocation), then you should make the constructor private and delete the implicit copy constructor. https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... File services/media/framework/models/active_sink.h (right): https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/models/active_sink.h:27: virtual Allocator* allocator() = 0; Is it OK for a user of an ActiveSink to use something other than the default allocator if the ActiveSink instance returns nullptr? IOW - does nullptr mean; "You really should use Allocator::GetDefault, but using something else is OK too." If not, then why not make this return an Allocator& and provide a default base class implementation which returns *Allocator::GetDefault(). If so, then why not keep the return type as an Allocator* and provide a base impl which returns Allocator::GetDefault(). Subclasses could override in order to provide a specific allocator instance, or to indicate that they really don't care (nullptr) which allocator is used. https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... File services/media/framework/models/lpcm_frames.h (right): https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/models/lpcm_frames.h:28: uint32_t bytes_per_frame() { this method is const. Same goes for the other accessors below (buffer and frame_count) https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/models/lpcm_frames.h:84: PacketPtr* reset_when_exhausted_; I'm unclear on why we need reset_when_exhausted_ here. From what I can tell, PacketPtr is a std::unique_ptr with a non-standard deleter. When a PacketPtr instance is reset (or has nullptr assigned to it), the non-standard deleter will be called, and presumably the packet memory will be returned to the allocator and the packet object itself perhaps placed on a free list somewhere. Why not just have a PacketPtr member which does this job, instead of a pointer to one? The LpcmFrames instance has ownership of the Packet, and when remaining_frame_count hits zero, you can just packet_ptr_.reset(). You get the same behavior in the destructor for free. Doing it like this seems to allow the LpcmFrames object to manipulate the bookkeeping of its user which seems odd. It also introduces a lifetime management issue; how do we know that our pointer to our PacketPtr (IOW - our pointer to our user's bookkeeping) is going to have a lifetime <= the PacketPtr instance itself? What prevents someone on the outside from resetting our PacketPtr (presumably freeing the memory which backs it) and leaving us with a stale buffer pointer (in the form of a non-null remaining_buffer_) https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... File services/media/framework/ostream.h (right): https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/ostream.h:36: static std::string by; Indenter::by could be constexpr. At the very least, it should be const (which means that it should be named BY, instead of by). Re: level; this is not thread safe, but I assume that you know this. This needs to be thread local, not static global, in order to be thread safe. I don't know our policy on the C++11 threadlocal keyword and if it is permitted or not. A hack which would keep the counts consistent (by the indenting sometimes incorrect) would be to use std::atomic to manipulate the level in the constructor/destructors, instead of ++/-- Finally, level should probably be private with a static accessor, and suffixed with _. The only way to manipulate the value level should be via the constructor/destructor. https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... File services/media/framework/packet.h (right): https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/packet.h:37: bool end_of_stream, <design_point note="No code change requested> PTS, duration and EOS are all pieces of metadata for a Packet (payload, media access unit, whatever), but do not represent the sum total of per-packet metadata we may wish to express. Other pieces of metadata which can be useful for some (but not all) packets include things like... 1) Random-access-point, drop-able flags for codecs with interpacket dependencies. 2) Colorspaces, packing format (subsampling, plane construction, etc..) for raw frames of video. 3) Mandated downmix matricies (or references to them) for decoded audio. 4) Language codes for subpicture packets. and so on. It may be good to consider decoupling the metadata from packet and having the packet just hold onto a reference to the metadata. </design_point> Stepping back from the design question, I could be convinced that the one piece of universal metadata which is useful to all packets is the presentation timestamp. Even though it sometimes takes on the special value "Unknown", it is so commonly accessed that I can understand hoisting it up to the class Packet level of things and using a sentinel value for "Unknown". In this case, why are we accessing the PTS via a virtual function? Wouldn't it be better to make the timestamp a private member of the base Packet class (accessible using an inline public accessor) and require its definition by the subclass using an explicit protected constructor? https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/packet.h:40: Allocator* allocator); Looking at the implementation (next file), it seems like we have 3 things here (size, payload and allocator) which all belong together. The Packet class is going to hold a pointer which came from an allocator and refers to a particular size allocation. When it is time to release, the Packet is going to call the allocator's free equivalent. If any of these things are mismatched, Bad Things could happen. We don't want to return a payload to the incorrect allocator, we don't want the size to be off when we do so, and so on. Would it be a good idea to introduce the concept of an Allocator::Buffer? All Allocator::Buffers would have a void* member which points to the buffer memory itself. For a default allocator (based on malloc/free or new/delete), this would be all that is needed. For other Allocator implementations, their Buffer implementation could contain a reference to the allocator which generated the Buffer as well as any bookkeeping (like size) which would be needed by the allocator on a per-packet basis to properly free the buffer. Alternatively, if there is bookkeeping which is useful to external users of the buffer (size, for example), it could be pushed down to the base Buffer implementation and accessed with an inline accessor (like payload). Finally, if allocators create std::unique_ptr<Allocator::Buffer>s, and Allocator::Buffer enforces move-semantics, then we have an extensible package of all the things we need to return the buffer to its owner allocator as well as implicit lifetime management, and we can pass just the unique_ptr to the buffer to this class instead of needing to pass all three members which need to stick together in order to be coherent. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/parts/file_reader.h (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/parts/file_reader.h:37: FILE* file_; I don't think that we should be using stdio FILE structure here. If we are going to continue to use them here, we should probably bury that fact using a FileReader/FileReaderImpl pattern (where FileReaderImpl is declared in the .cc file) In general, I think we really want to... 1) be designing our classes to use the underlying Mojo services for things like files and network access. 2) Design all of our I/O classes with an async IO model in mind, not a synchronous one. There are multiple ways to fake out ffmpeg's transport interfaces if we go this route. The simplest way would be to use a mojo service to get a data pipe and then just use the data pipe in a synchronous. There are other ways (which usually involve a dedicated thread) to fake out ffmpeg so it feels that it is accessing data synchronously when it is actually async underneath at the transport implementation level.
https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... File services/media/framework/metadata.h (right): https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/metadata.h:19: class Metadata { On 2016/01/26 01:32:38, johngro wrote: > As a top level design thing, this class seems to be a starting point for asset > metadata (either a live stream or from a static asset). As time goes on, this > is going to need to become more flexible than what we have here, and may even > need to become dynamic. Acknowledged. https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/metadata.h:30: Metadata( On 2016/01/26 01:32:38, johngro wrote: > If the intention here is to make this class the type of thing which people need > to manipulate via unique_ptr patterns (IOW - use only New, Clone and std::move; > disallow implicit copying and stack allocation), then you should make the > constructor private and delete the implicit copy constructor. Done. https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... File services/media/framework/models/active_sink.h (right): https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/models/active_sink.h:27: virtual Allocator* allocator() = 0; On 2016/01/26 01:32:39, johngro wrote: > Is it OK for a user of an ActiveSink to use something other than the default > allocator if the ActiveSink instance returns nullptr? IOW - does nullptr mean; > "You really should use Allocator::GetDefault, but using something else is OK > too." > > If not, then why not make this return an Allocator& and provide a default base > class implementation which returns *Allocator::GetDefault(). > > If so, then why not keep the return type as an Allocator* and provide a base > impl which returns Allocator::GetDefault(). Subclasses could override in order > to provide a specific allocator instance, or to indicate that they really don't > care (nullptr) which allocator is used. Allocator assignment is handled during Prepare by Stage, StageInput and StageOutput instances. The only code that calls this method, for example, is in ActiveSinkStage. Enlisting the default allocator is only done in a couple of places, so there's no benefit to supplying it here. We get maximum flexibility when 'parts' don't do anything they don't have to. I think your suggestion would be the right way to go if allocator logic were more distributed, but it's not. https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... File services/media/framework/models/lpcm_frames.h (right): https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/models/lpcm_frames.h:28: uint32_t bytes_per_frame() { On 2016/01/26 01:32:39, johngro wrote: > this method is const. Same goes for the other accessors below (buffer and > frame_count) Good catch. Still not in the habit of looking for these. https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/models/lpcm_frames.h:84: PacketPtr* reset_when_exhausted_; On 2016/01/26 01:32:39, johngro wrote: > I'm unclear on why we need reset_when_exhausted_ here. > > From what I can tell, PacketPtr is a std::unique_ptr with a non-standard > deleter. When a PacketPtr instance is reset (or has nullptr assigned to it), > the non-standard deleter will be called, and presumably the packet memory will > be returned to the allocator and the packet object itself perhaps placed on a > free list somewhere. > > Why not just have a PacketPtr member which does this job, instead of a pointer > to one? The LpcmFrames instance has ownership of the Packet, and when > remaining_frame_count hits zero, you can just packet_ptr_.reset(). You get the > same behavior in the destructor for free. > > Doing it like this seems to allow the LpcmFrames object to manipulate the > bookkeeping of its user which seems odd. It also introduces a lifetime > management issue; how do we know that our pointer to our PacketPtr (IOW - our > pointer to our user's bookkeeping) is going to have a lifetime <= the PacketPtr > instance itself? What prevents someone on the outside from resetting our > PacketPtr (presumably freeing the memory which backs it) and leaving us with a > stale buffer pointer (in the form of a non-null remaining_buffer_) This feature is used in LpcmStageInput, and the hazards you mention are avoided. I take your point that this use of PacketPtr* constitutes an anti-pattern. I'll address this by removing all references to PacketPtr here (making this class more independent) and providing a callback mechanism so LpcmStageInput and other callers can take action when the frames are exhausted as needed. https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... File services/media/framework/ostream.h (right): https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/ostream.h:36: static std::string by; On 2016/01/26 01:32:39, johngro wrote: > Indenter::by could be constexpr. At the very least, it should be const (which > means that it should be named BY, instead of by). > > Re: level; this is not thread safe, but I assume that you know this. This needs > to be thread local, not static global, in order to be thread safe. I don't know > our policy on the C++11 threadlocal keyword and if it is permitted or not. > > A hack which would keep the counts consistent (by the indenting sometimes > incorrect) would be to use std::atomic to manipulate the level in the > constructor/destructors, instead of ++/-- > > Finally, level should probably be private with a static accessor, and suffixed > with _. The only way to manipulate the value level should be via the > constructor/destructor. This stuff is for debugging. I'd rather leave it out of the CL than spend a lot of time getting it perfect, though copying it around from branch to branch would be a hassle. I messed with it for awhile without great results. I switched to using custom stream manipulators, which is probably a better pattern, but that doesn't really address your concerns. https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... File services/media/framework/packet.h (right): https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/packet.h:37: bool end_of_stream, On 2016/01/26 01:32:39, johngro wrote: > <design_point note="No code change requested> > PTS, duration and EOS are all pieces of metadata for a Packet (payload, media > access unit, whatever), but do not represent the sum total of per-packet > metadata we may wish to express. Other pieces of metadata which can be useful > for some (but not all) packets include things like... > > 1) Random-access-point, drop-able flags for codecs with interpacket > dependencies. > 2) Colorspaces, packing format (subsampling, plane construction, etc..) for raw > frames of video. > 3) Mandated downmix matricies (or references to them) for decoded audio. > 4) Language codes for subpicture packets. > > and so on. It may be good to consider decoupling the metadata from packet and > having the packet just hold onto a reference to the metadata. > </design_point> > > Stepping back from the design question, I could be convinced that the one piece > of universal metadata which is useful to all packets is the presentation > timestamp. Even though it sometimes takes on the special value "Unknown", it is > so commonly accessed that I can understand hoisting it up to the class Packet > level of things and using a sentinel value for "Unknown". In this case, why are > we accessing the PTS via a virtual function? Wouldn't it be better to make the > timestamp a private member of the base Packet class (accessible using an inline > public accessor) and require its definition by the subclass using an explicit > protected constructor? Ack comments about more metadata. Are you concerned about performance issues with PTS? The entire thing is abstract, because some implementations are backed by structures defined by e.g. ffmpeg (AVFrame and AVPacket are examples). https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/packet.h:40: Allocator* allocator); On 2016/01/26 01:32:39, johngro wrote: > Looking at the implementation (next file), it seems like we have 3 things here > (size, payload and allocator) which all belong together. The Packet class is > going to hold a pointer which came from an allocator and refers to a particular > size allocation. When it is time to release, the Packet is going to call the > allocator's free equivalent. > > If any of these things are mismatched, Bad Things could happen. We don't want > to return a payload to the incorrect allocator, we don't want the size to be off > when we do so, and so on. > > Would it be a good idea to introduce the concept of an Allocator::Buffer? All > Allocator::Buffers would have a void* member which points to the buffer memory > itself. For a default allocator (based on malloc/free or new/delete), this > would be all that is needed. For other Allocator implementations, their Buffer > implementation could contain a reference to the allocator which generated the > Buffer as well as any bookkeeping (like size) which would be needed by the > allocator on a per-packet basis to properly free the buffer. > > Alternatively, if there is bookkeeping which is useful to external users of the > buffer (size, for example), it could be pushed down to the base Buffer > implementation and accessed with an inline accessor (like payload). > > Finally, if allocators create std::unique_ptr<Allocator::Buffer>s, and > Allocator::Buffer enforces move-semantics, then we have an extensible package of > all the things we need to return the buffer to its owner allocator as well as > implicit lifetime management, and we can pass just the unique_ptr to the buffer > to this class instead of needing to pass all three members which need to stick > together in order to be coherent. That's a worthy design idea. Can we save it for another CL?
first pass complete, time for me to start again and go over your responses to my earlier, published rounds of feedback. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/parts/lpcm_reformatter.cc (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/parts/lpcm_reformatter.cc:14: switch (in_type.sample_format()) { up to you, but you can reduce the amount of duplication of the inner switch block here by templating this method. The idea would be... 1) Move the class definition of LpcmReformatterImpl into this transitional unit. You can stuff it into an anon namespace if you want to really bury it. 2) Make the impl class's constructor public. 3) Make a static function templated on typename TIn. Have it implement the inner switch to select the out type, and use the passed template parameter to select the in type. 4) Implement the outer switch in LpcmReformatter::New, use it to select the in type and then call the templated method to select the out type. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/parts/lpcm_reformatter.cc:128: inline void CopySample(uint8_t* dest, float* source) { Source needs to be clamped to the range [-1.0, 1.0] here. This goes for all of the instances of CopySample which take float as a source. During the demo, this was causing some clicks and pops as the decoder spit out samples slightly above and below [-1.0, 1.0] FWIW - the same can be said for the 24-in-32 source format as well since, like the float, not all of the input value are valid for the encoding. Long term, I suppose we need to decide whose job this ultimately is, but in the short term, we have badly behaved codecs and no one fixing up their crap. In case its helpful, I just ended up defining an inline templated clamp function and applying it to the formats with float sources. I skipped the 24-in-32 formats as they were not relevant to the demo, but it should be simple enough to use the same function. template <typename T> inline constexpr T Clamp(T val, T min, T max) { return (val > max) ? max : ((val < min) ? min : val); } inline void CopySample(uint8_t* dest, float* source) { *dest = static_cast<uint8_t>((Clamp(*source, -1.0f, 1.0f) * 0x7f) + 128); } inline void CopySample(int16_t* dest, float* source) { *dest = static_cast<int16_t>(Clamp(*source, -1.0f, 1.0f) * 0x7fff); } inline void CopySample(int32_t* dest, float* source) { *dest = static_cast<int32_t>(Clamp(*source, -1.0f, 1.0f) * 0x7fffff); } https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/parts/lpcm_reformatter.cc:250: uint8_t* out_channel = reinterpret_cast<uint8_t*>(dest->buffer()); FWIW - when casing a pointer to a PoD to another pointer to a PoD, you can just static_cast, it should do exactly the same thing. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/parts/lpcm_reformatter.cc:255: if (mix) { if you are going to put the mix decision in here, you should put it outside of the loop, not inside. The compiler may attempt to make this optimization for you, but this may be getting a bit complicated for it. Alternatively, if you make a private template of this method which takes a bool DoMix parameter, you can call if from the public method where mix is decided at runtime. Then, whether you put the if (DoMix) inside or outside of the loop the compiler will kill the unused path in each of the generated copies of the template expansion. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/parts/lpcm_reformatter.h (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/parts/lpcm_reformatter.h:27: return LpcmReformatterPtr(NewImpl(in_type, out_type)); What is the advantage of having an inline public new which just calls a private NewImpl? Why not just a public New whose implementation lives in the .cc file? https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/parts/lpcm_reformatter.h:39: class LpcmReformatterImpl : public LpcmReformatter { You can just hide this class in the .cc file. There is no need to expose it's insides to the world. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/parts/null_sink.cc (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/parts/null_sink.cc:24: demand_callback(Demand::kNeutral); You are making a demand callback inside the RegisterDemand call for an ActiveSink; is this OK? I thought that the model was that the initial demand got expressed when the engine called Prime on all of the participants at startup. Seems like the demand callback instance should be stored in the sink and called once during prime. While we are on the subject, shouldn't ActiveSink have a protected demand_callback_ member and implement RegisterDemandCallback itself? The method could be made virtual if a derived class had custom work to do during the registration/link-up phase, but it feels like this is really just bookkeeping, and that any startup actions should take place during Prime. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/ptr.h (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/ptr.h:36: UniquePtr Clone() const { return *this ? this->get()->Clone() : UniquePtr(); } Do we really want to sub-class std::unique_ptr to get this functionality? Mostly, this just allows us to say.. UniquePtr<ClassA> obj = make_unique<ClassA>(...); UniquePtr<ClassA> cloned = obj.Clone(); Instead of... UniquePtr<ClassA> cloned = obj->Clone(); It also provides an automatic null check and will produce a new NULL unique_ptr if the cloned pointer is NULL. I'm not sure I see the value in this (why is someone trying to clone an object which is not there?), but if you really want this behavior, you should be able to say something like... template <typename T> std::unique_ptr<T> SafeClone(const std::unique_ptr<T> obj) { return obj ? obj->Clone() : nullptr; } and the code above becomes std::unique_ptr<ClassA> cloned = SafeClone(obj); without needing to extend std::unique_ptr https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/ptr.h:39: // shared_ptr with upcast to TBase. Why do this? std::shared_ptr already has 3 static methods which handle static, dynamic and const casting for you. To do what you are doing with the overloaded operator below, you can just do things like... std::shared_ptr<MyDerived> derived = std::make_shared<MyDerived>(); std::shared_ptr<MyBase> base = std::static_pointer_cast<MyBase>(derived); GiveBaseToSomeoneElse(base); This is permitted for both up and down casting. Clearly you need to know what you are doing when down casting, since we don't use RTTI and cannot use dynamic cast. Also, if you are not going to give the pointer to someone else (who might retain it), and you don't want to make a temp shared_ptr, you can just cast the underlying pointer. Obviously, you would only ever need to do this in a down-cast scenario void GiveBaseToSomeoneElse(const std::shared_ptr<MyBase>& base) { // Danger, we had better know that base is actually a MyDerived MyDerived* derived = static_cast<MyDerived*>(base.get()); derived->MethodOnlyInMyDerived(...); } https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/result.h (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/result.h:13: // Abstract base class for objects that allocate buffers for packets. cut-n-paste error, this is not the comment you are looking for <jedi_wave/> https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/result.h:14: enum class Result { So, I did the same thing (defined a media result in .mojom) and got chewed out by Trung and James. Trung has a vision for an error code system that he wants us all to use. I had difficulty figuring out how it was going to solve my problems, but they made me comment all of my codes with which one of Trung's codes they would become someday. No action requested here, but we may want to consider defining these in .mojom so they can be used over mojom interfaces. When we revisit, we should probably take a second look Trung's proposal and make an effort to make use of it. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/stages/active_sink_stage.h (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stages/active_sink_stage.h:17: class ActiveSinkStage : public SinkStage, public SingleInputStage { see comments in stage.h about MI, this applies to most of the stage/* classes https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/stages/stage.h (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stages/stage.h:44: using UpdateCallback = std::function<void(Stage* stage)>; public typedefs go at the top of the class. see https://google.github.io/styleguide/cppguide.html#Declaration_Order https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stages/stage.h:68: class SourceStage : public virtual Stage { so,this type of MI is pretty much banned by google3 style and *strongly* discouraged by the chrome style (which is based on google3). https://google.github.io/styleguide/cppguide.html#Multiple_Inheritance Is there another way that this could be decomposed to avoid the need for MI with diamond inheritance? Perhaps Jeff has some ideas? https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/stages/stage_input.h (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stages/stage_input.h:33: void connect(Stage* upstream_stage, uint32_t output_index) { It would be good to DCHECK that we are not already connected, and that the connection we are making is valid (eg, not nullptr). https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stages/stage_input.h:55: return must_allocate_; must_allocate_ is only really defined after Prepare gets called. It would be good to be able to assert that the StageInput had been properly prepared before people start to call the allocator or must_allocate accessors. It would be convenient to be able to use allocator_'s not-null-ness to test for this, but currently, allocator_ is permitted to be nullptr, so that won't work. One idea (mentioned in my feedback in allocator.h) would be to disallow a nullptr allocator. This would require callers of Prepare to (at least) provide the default allocator. If this is not workable, a state could be introduced (either a bool flag, or a state enum) to note that the system had been properly prepared before these calls got made. Since this serves to purpose aside from debugging, it would need to be put under a debug build #def conditional, which is something of a PITA. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stages/stage_input.h:74: Stage* upstream_stage_; FWIW, you can initialize PoD members at declaration time in C++11 now. This is particularly useful when you have multiple constructors (not the case here), but it can help readability as well, IMO. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/stages/stage_output.cc (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stages/stage_output.cc:50: if (demand_ == Demand::kNegative || mate().packet_from_upstream()) { this check (demand == negative) is redundant. It will fall out naturally from the return demand_ below if the mate does not have a packet. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/stages/stage_output.h (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stages/stage_output.h:56: void supply_packet(PacketPtr packet, Engine* engine) const; this method is not a simple accessor, it takes action. It should probably be named SupplyPacket. Same for update_demand_internal below. Also, since it is in the public scope, it should probably be just UpdateDemand, not UpdateDemandInternal. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stages/stage_output.h:66: void supply_packet_internal(PacketPtr packet, Engine* engine) const; SupplyPacketInternal https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/stream_type.h (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stream_type.h:6: #define SERVICES_MEDIA_FRAMEWORK_STREAM_TYPE_H_ top level comment. This whole file feels like a custom, in-proc-centric C++ implementation of the media types which you spec'ed in mojom way back when. Why are we not using the mojom generated classes here? How are we going to manage this when we have to start to inter-operate with media services (potentially implemented in other languages) across mojo transport boundaries? If we choose to do anything at all about this in the short term, it should probably be handled with a follow-up change, but I suspect that this will start to be an issue in the short-to-mid term. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stream_type.h:33: StreamTypes(size_t size) : std::vector<StreamTypePtr>(size) {} if we are imposing move semantics on this class, this constructor should be private, and the copy constructor should be removed. I think that the Mojo framework already has a handy macro for this, but I'm not sure. If we are not imposing move semantics on this class, then we should probably just say... using StreamTypes = std::vector<StreamTypePtr>; and leave it at that. This seems to apply to all of the extensions of std::vector below. They all seem to add New and Clone to the mix to force unique_ptr and move patterns, but they all expose a direct constructor as well. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stream_type.h:47: static BytesPtr New(uint8_t* data, size_t size) { DCHECK(data || !size); memcpy's behavior when passing nullptr for the source is not super consistent across std C library implementations, but its probably pretty safe to call it with nullptr, provided that size is zero. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stream_type.h:53: Bytes(size_t size) : std::vector<uint8_t>(size) {} explicit Bytes(size_t size) ... also, this should probably be private. Same for the copy constructor below. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stream_type.h:57: const uint8_t* data() const { return &(*this)[0]; } Why is the base class's implementation of data (here and below) not good enough for this job? https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stream_type.h:109: Range(T min_param, T max_param) : min(min_param), max(max_param) {} DCHECK(min <= max); https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stream_type.h:114: bool contains(T t) const { inline, constexpr Also, this is probably only for primitive data types, by you might want to spec this as const T& instead of passing T by value. When the method gets inlined, the compiler will "do the right thing" for PoDs. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stream_type.h:149: ~StreamTypeSet(); virtual destructor. I'm pretty sure that style mandates that you always have a virtual destructor if you have virtual members. Also in the derived classes. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stream_type.h:155: virtual const MultiplexedStreamTypeSet* multiplexed() const; If only we allowed dynamic_cast, you would not need to do this. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stream_type.h:447: class VideoStreamType : public StreamType { I think that we will need to revisit this at some point in time. I'm not all that sure that using enums in header files is a long term scale-able solution to communicating this information (this really starts to show as you start to enumerate all of the profiles of H.264 below, but have not mixed in their concept of levels as well) The pixel packing section has related issues. In addition to needing to enumerate all of the combinations in the world (ARGB, ABGR, BGRA, etc...), just saying that a frame is YUYV can miss hardware imposed line stride requirements (I420 as issues with both line and plane stride, etc...) No change requested now, but I think we need to come back to this and sit down with the graphics team at some point and figure out a final solution. It may end up being enums, but I'm not convinced that this will be the proper thing to do in the long run. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stream_type.h:521: VideoStreamType( again, something to revisit later on, but I am not convinced that these are necessarily properties of the video stream. Things like the colorspace, the width/height, the active video region, the pixel aspect ratio and so on can actually change on a per-frame basis. Whether or not we support this at the per-frame level or we support this by tearing down and rebuilding video sub-streams in a seamless fashion (or through some completely other means) is something I am not clear on, but it is something I have been burnt on when attempting to use other media frameworks to create broadcast video devices in the past. I tossed out some starting points for how this might be approached in the video-pipe strawman doc I wrote for James forever ago, but I'm not certain that anyone ever took the time to read through it.
publishing feedback on the current responses to the first round of feedback. Looks like you got up to the PS#4 feedback, so I stopped there. Top level comment; it looks like you are reworking a bunch of this code to make the engine thread safe. Next time around, lets leave work like this until a follow-up change. Once published for review, a changes to a CL should probably limited to small things (spelling errors, typos, etc..) and changes requested during review. This helps me (at least) to focus on making sure that we have come to consensus on the code as submitted. Significant changes (either requested by a reviewer or self-initiated) can be postponed (unless they are critically life threatening) and allow the follow up change to be easily evaluated as the changes required to implement the feature/refactor/whatever, instead of being mixed in with everything else. I'm going to go back and start to re-read the changes to the Engine threading model now. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/allocator.cc (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/allocator.cc:34: static DefaultAllocator default_allocator; On 2016/01/25 23:29:37, dalesat wrote: > On 2016/01/20 00:25:32, johngro wrote: > > I think that static-scoped classes are supposed to be marked constexpr to > insure > > that they have no runtime initialization. Static and global classes with > > runtime initialization are disallowed by style to prevent initialization > > ordering bugs. > > I'm not sure what to do here in that case. I can't declare default_allocator > constexpr, because GetDefault returns a non-const pointer. This is true even > when I make the constructor constexpr. The compiler won't allow me to also make > the method implementations constexpr. I could lazy-create the default allocator, > but would require a lock. Suggestions? const_cast is your friend. I banged out a simple example below which follows the pattern you are attempting to implement here. Change any of the #if 0s to #if 1s to see why the constexpr-ness of the static declaration prevents runtime side effects during construction/destruction, but still allows non-const member functions. Also, note that you can still have member variables with initializers and constructors which take parameters, provided that they are all evaluate-able at compile time (eg, constexpr). uint32_t ThisFunctionIsNotConstexpr() { return 4; } class MyStaticBaseClass { public: static MyStaticBaseClass* GetDefault(); virtual void Foo() = 0; }; class MyStaticClass : public MyStaticBaseClass { public: #if 0 constexpr MyStaticClass() { foo_var_ = ThisFunctionIsNotConstexpr(); } #else constexpr MyStaticClass() { } #endif #if 0 ~MyStaticClass() { } #endif void Foo() override; private: #if 0 uint32_t foo_var_ = ThisFunctionIsNotConstexpr(); #else uint32_t foo_var_ = 5; #endif }; static constexpr MyStaticClass default_static_class; MyStaticBaseClass* MyStaticBaseClass::GetDefault() { return const_cast<MyStaticClass*>(&default_static_class); } void MyStaticClass::Foo() { LOG(INFO) << "I was called " << __PRETTY_FUNCTION__ << " foo_var was " << foo_var_; } int main(int argc, char** argv) { MyStaticBaseClass::GetDefault()->Foo(); return 0; } https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/allocator.h (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/allocator.h:21: virtual void* AllocatePayloadBuffer(uint64_t size) = 0; On 2016/01/20 00:25:32, johngro wrote: > <nit> > for in-proc non-mojom'ed stuff, we should use size_t instead of u64 here. > </nit> Checking in; any thoughts on using size_t instead of uint64_t here? https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/conversion_pipeline_builder.cc (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/conversion_pipeline_builder.cc:192: NOTREACHED() << "conversion requires mixdown/up - not supported"; On 2016/01/25 17:47:29, dalesat wrote: > On 2016/01/20 00:25:32, johngro wrote: > > why not just LOG(ERROR) or MOJO_LOG(ERROR) instead of NOTREACHED? is it that > > the not reached form ASSERTs in the process? > > The NOTREACHED will be removed as part of addressing the TODO. In the mean time, > I want the code to fail noisily if I try to make it do things that aren't > implemented. Understood, but not really what I was asking. I wanted to know what the behavior of the macro was, and did it assert/dcheck. I researched the answer to my own question; NOTREACHED is defined in base logging. In a non-chromeOS build, it evaluates to DCHECK(false). DCHECK(false) (in addition to dchecking) provides a lazy logstream at level DCHECK to eat the ostream parameters. On chromeOS, a special function (LogErrorNotReached) is called, and the ostream parameters are always discarded. TL;DR? Ack https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/engine.cc (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/engine.cc:10: uint32_t Engine::Part::input_count() { On 2016/01/25 23:29:37, dalesat wrote: > On 2016/01/20 00:25:32, johngro wrote: > > Would it be a good idea to inline most of these accessors? > > These are only used for graph building, so they aren't called at high frequency. > The memory penalty of inline is probably not justified for these. Acknowledged. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/engine.cc:93: std::unique_ptr<Stage> to_remove(stage); // Release this duplicate unique_ptr! On 2016/01/25 23:29:37, dalesat wrote: > On 2016/01/20 00:25:32, johngro wrote: > > something feel wrong about this... > > > > if stages_ is a list of unique ptrs, then you should not be able to be holding > a > > pointer to anything that the unique pointer points to. Other objects could > hold > > a const std::unique_ptr<>&, provided you are sure that the other objects will > > have a shorter life than the unique pointer itself, but (in theory) you should > > never be manipulating the naked pointer at all, since it violates the > > unique-ness of the pointer and the lifetime management safety that unique_ptr > is > > supposed to provide. > > > > the fact that the same stage pointer is a member of the sources_ and sinks_ > > collection while also being part of the stages_ collection seems to highlight > > this point. stages_ is a collection of unique pointers while sources_ and > > sinks_ are naked pointer which can point to the same thing as the pointers in > > stages_. IOW - the pointers held in the stages_ collection seem to be far > from > > unique. > > A good analogy is that engine is a container like std::list, Stage is like a > list node, and Part, Input and Output are like iterators. I used unique_ptr as > an expedient so the stages get deleted automatically. I understand this flies in > the face of unique_ptr dogma, so I'll just use raw pointers. Acknowledged. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/engine.cc:284: while (true) { On 2016/01/25 17:47:29, dalesat wrote: > On 2016/01/20 00:25:32, johngro wrote: > > something to consider moving forward; > > > > If this is running on a mojo message loop, the loop is being blocked while > this > > is being run. Re-queueing update jobs to the loop and un-winding out would > > allow other messages to be processed while still running to completion. > > > > If this is meant to be run on a dedicated thread, then some attention will > need > > to be paid to some form of external "shutdown now" signal so we can shut down > > promptly without needing to run the processing loop to completion. Given the > > nature of the system (where stages and queue more work during the update > call), > > we cannot prove that this will ever run to completion, so it seems important > for > > there be a way to externally trigger a clean shutdown of the engine. > > Agreed. The threading model isn't complete. Acknowledged. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/engine.cc:338: Stage* stage = supply_backlog_.front(); On 2016/01/20 00:25:32, johngro wrote: > the comment above indicates that this is supposed to be a stack (LIFO), but the > code here is pushing to the back and popping from the front, which is a queue > (FIFO) not a stack. This still seems backwards, see updated comment in PS#5 around PushTo(Supply|Demand)BacklogUnsafe https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/engine.h (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/engine.h:33: Input input(uint32_t index); On 2016/01/25 23:29:37, dalesat wrote: > On 2016/01/25 17:47:29, dalesat wrote: > > On 2016/01/20 00:25:32, johngro wrote: > > > re: the accessors here which are making copies of Inputs, Outputs and Parts > > > > > > Is there an advantage to doing this? Why not just return a const& and let > the > > > caller make a copy using the default copy constructor if they need a mutable > > > copy? > > > > The intent is that Part, Input and Output are value types comparable to > > iterators. Part is pointer-sized and there's no benefit to passing it by > > reference. Input and Output are pointer + 32 bits, so your suggestion has some > > benefit for those. Regardless, I don't want to introduce any concerns about > > lifetime. > > Also, Input and Output are synthesized by these methods (from a stage pointer > and an input/output index). There's nothing to reference if we wanted to return > a reference. Acknowledged. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/lpcm_util.h (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/lpcm_util.h:30: uint64_t in_byte_count, On 2016/01/25 23:29:37, dalesat wrote: > On 2016/01/25 18:55:43, johngro wrote: > > in_byte_count seems redundant here. Don't we know everything we need to know > > (number of channels in a frame, number of bytes in a sample) from the stream > > type which was specified at creation time? It this to allow us to interleave > > only part of a non-interleaved packet? If so, how do we manage to interleave > > the rest of the data later on (we will be offset into the first "plane" and > > subsequent planes will be larger) > > The layout of non-interleaved samples we get from ffmpeg is such that the > payload is evenly divided into a buffer for each frame (so each buffer is > in_byte_count / frames_per_sample in size). The samples for a frame are > contiguous in the buffer starting at the beginning of the buffer, but they don't > necessarily fill the buffer. k, This is an important detail/assertion; perhaps a comment outlining this would be a good idea. Somewhere there needs to be a DCHECK((in_byte_count % bytes_per_frame()) == 0); DCHECK((in_byte_count > bytes_per_frame()) >= frame_count); Alternatively, you could keep things in units of frames and have something like total_frame_count and active_frame_count, in which case all you would need would be DCHECK(total_frame_count >= active_frame_count); https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... File services/media/framework/models/active_sink.h (right): https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/models/active_sink.h:27: virtual Allocator* allocator() = 0; On 2016/01/26 21:17:51, dalesat wrote: > On 2016/01/26 01:32:39, johngro wrote: > > Is it OK for a user of an ActiveSink to use something other than the default > > allocator if the ActiveSink instance returns nullptr? IOW - does nullptr > mean; > > "You really should use Allocator::GetDefault, but using something else is OK > > too." > > > > If not, then why not make this return an Allocator& and provide a default base > > class implementation which returns *Allocator::GetDefault(). > > > > If so, then why not keep the return type as an Allocator* and provide a base > > impl which returns Allocator::GetDefault(). Subclasses could override in > order > > to provide a specific allocator instance, or to indicate that they really > don't > > care (nullptr) which allocator is used. > > Allocator assignment is handled during Prepare by Stage, StageInput and > StageOutput instances. The only code that calls this method, for example, is in > ActiveSinkStage. Enlisting the default allocator is only done in a couple of > places, so there's no benefit to supplying it here. We get maximum flexibility > when 'parts' don't do anything they don't have to. I think your suggestion would > be the right way to go if allocator logic were more distributed, but it's not. Acknowledged. https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... File services/media/framework/models/lpcm_frames.h (right): https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/models/lpcm_frames.h:84: PacketPtr* reset_when_exhausted_; On 2016/01/26 21:17:51, dalesat wrote: > On 2016/01/26 01:32:39, johngro wrote: > > I'm unclear on why we need reset_when_exhausted_ here. > > > > From what I can tell, PacketPtr is a std::unique_ptr with a non-standard > > deleter. When a PacketPtr instance is reset (or has nullptr assigned to it), > > the non-standard deleter will be called, and presumably the packet memory will > > be returned to the allocator and the packet object itself perhaps placed on a > > free list somewhere. > > > > Why not just have a PacketPtr member which does this job, instead of a pointer > > to one? The LpcmFrames instance has ownership of the Packet, and when > > remaining_frame_count hits zero, you can just packet_ptr_.reset(). You get > the > > same behavior in the destructor for free. > > > > Doing it like this seems to allow the LpcmFrames object to manipulate the > > bookkeeping of its user which seems odd. It also introduces a lifetime > > management issue; how do we know that our pointer to our PacketPtr (IOW - our > > pointer to our user's bookkeeping) is going to have a lifetime <= the > PacketPtr > > instance itself? What prevents someone on the outside from resetting our > > PacketPtr (presumably freeing the memory which backs it) and leaving us with a > > stale buffer pointer (in the form of a non-null remaining_buffer_) > > This feature is used in LpcmStageInput, and the hazards you mention are avoided. > I take your point that this use of PacketPtr* constitutes an anti-pattern. I'll > address this by removing all references to PacketPtr here (making this class > more independent) and providing a callback mechanism so LpcmStageInput and other > callers can take action when the frames are exhausted as needed. Acknowledged. https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... File services/media/framework/ostream.h (right): https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/ostream.h:36: static std::string by; On 2016/01/26 21:17:51, dalesat wrote: > On 2016/01/26 01:32:39, johngro wrote: > > Indenter::by could be constexpr. At the very least, it should be const (which > > means that it should be named BY, instead of by). > > > > Re: level; this is not thread safe, but I assume that you know this. This > needs > > to be thread local, not static global, in order to be thread safe. I don't > know > > our policy on the C++11 threadlocal keyword and if it is permitted or not. > > > > A hack which would keep the counts consistent (by the indenting sometimes > > incorrect) would be to use std::atomic to manipulate the level in the > > constructor/destructors, instead of ++/-- > > > > Finally, level should probably be private with a static accessor, and suffixed > > with _. The only way to manipulate the value level should be via the > > constructor/destructor. > > This stuff is for debugging. I'd rather leave it out of the CL than spend a lot > of time getting it perfect, though copying it around from branch to branch would > be a hassle. I messed with it for awhile without great results. I switched to > using custom stream manipulators, which is probably a better pattern, but that > doesn't really address your concerns. So, I don't really understand xalloc/iword/pword super well (I'm not a fan of ostreams) and I needed to do some reading, but I think you are correct that this does not address my original concerns. If I am understanding correctly, xalloc will create a unique index which can be used with the iword member of any ostream instance to retrieve a long integer (initialized to zero) which is unique to that ostream instance. If I am understanding the base::log ostreams correctly, that means that we have implemented an indentation level per log stream. The thread safety concerns remain. Also, xalloc is apparently not thread safe until C++14 (and we are currently on C++11). I agree that this is probably a moot point, as I don't know what level of thread safety exists in the base or mojo log streams (we are likely to get either interleaved characters or lines anyway). That said, I checked with James regarding thread local storage. What he told me was... 1) Apple prohibits the thread_local keyword (introduced in C++11), but google and chrome toolchains support it. We allow its use provided your code does not need to run in an apple environment. For our purposed, this means that if you do not expect this code to be linked against Flutter, you can use thread_local. 2) If you want to be double sure, you can use base::ThreadLocalPointer; this should work on all platforms. So, your original code would be perfectly fine and thread safe (no atomics needed) by just saying static thread_local int level; instead (provided you didn't need to link against flutter). A small amount of more work would be needed to make the base::ThreadLocalPointer class work, but from glancing at the code, it seems like this would be pretty easy to manage either manually or with a small templated wrapper class (so you could say something like ThreadLocal<uint32_t> level(0)) It's fine if you want to leave it like this, just trying to give some options. https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... File services/media/framework/packet.h (right): https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/packet.h:37: bool end_of_stream, On 2016/01/26 21:17:51, dalesat wrote: > On 2016/01/26 01:32:39, johngro wrote: > > <design_point note="No code change requested> > > PTS, duration and EOS are all pieces of metadata for a Packet (payload, media > > access unit, whatever), but do not represent the sum total of per-packet > > metadata we may wish to express. Other pieces of metadata which can be useful > > for some (but not all) packets include things like... > > > > 1) Random-access-point, drop-able flags for codecs with interpacket > > dependencies. > > 2) Colorspaces, packing format (subsampling, plane construction, etc..) for > raw > > frames of video. > > 3) Mandated downmix matricies (or references to them) for decoded audio. > > 4) Language codes for subpicture packets. > > > > and so on. It may be good to consider decoupling the metadata from packet and > > having the packet just hold onto a reference to the metadata. > > </design_point> > > > > Stepping back from the design question, I could be convinced that the one > piece > > of universal metadata which is useful to all packets is the presentation > > timestamp. Even though it sometimes takes on the special value "Unknown", it > is > > so commonly accessed that I can understand hoisting it up to the class Packet > > level of things and using a sentinel value for "Unknown". In this case, why > are > > we accessing the PTS via a virtual function? Wouldn't it be better to make > the > > timestamp a private member of the base Packet class (accessible using an > inline > > public accessor) and require its definition by the subclass using an explicit > > protected constructor? > > Ack comments about more metadata. > > Are you concerned about performance issues with PTS? The entire thing is > abstract, because some implementations are backed by structures defined by e.g. > ffmpeg (AVFrame and AVPacket are examples). mildly; its one of those things which feel like a coin flip now, but later who knows. If the timestamp on a packet once created is immutable (a principal I think we both agree is a worthwhile goal), then it should be simple to have the protected constructor of Packet demand that a PTS be provided at construction time. I could be then accessed with a inline const accessor for pretty much no cost. If the underlying packet is going to undergo transformations which alter the PTS, and this is going to happen inside the context of something like an ffmpeg operation on an encapsulated ffmpeg structure, then this technique will not work without a bunch of manual labor which will be hard to maintain. Finally, if we are directly accessing the internal ffmpeg representation of an encapsulated PTS, then we are being implicitly forced to use the same sentinel value for no PTS that ffmpeg uses. If we are operating under the non-volatile PTS assumption given above, and choose to cache the PTS locally in this base class for inline access, then we can map from their value to our value as needed at construction time. If not, then we need to be sure that the values are the same. If they are already, then good. If not, then we can change ours to match theirs provided there are no design concerns about their choice (I would not use 0 or -1 as my sentinel; either 0x8000... or 0x7FFF... seem like better choices). Either way, we should have a static_assert somewhere in the FfmpegBackedPacketImpl code which asserts that their value matches our value (IIRC, they use a #def called NO_PTS) https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/packet.h:40: Allocator* allocator); On 2016/01/26 21:17:51, dalesat wrote: > On 2016/01/26 01:32:39, johngro wrote: > > Looking at the implementation (next file), it seems like we have 3 things here > > (size, payload and allocator) which all belong together. The Packet class is > > going to hold a pointer which came from an allocator and refers to a > particular > > size allocation. When it is time to release, the Packet is going to call the > > allocator's free equivalent. > > > > If any of these things are mismatched, Bad Things could happen. We don't want > > to return a payload to the incorrect allocator, we don't want the size to be > off > > when we do so, and so on. > > > > Would it be a good idea to introduce the concept of an Allocator::Buffer? All > > Allocator::Buffers would have a void* member which points to the buffer memory > > itself. For a default allocator (based on malloc/free or new/delete), this > > would be all that is needed. For other Allocator implementations, their > Buffer > > implementation could contain a reference to the allocator which generated the > > Buffer as well as any bookkeeping (like size) which would be needed by the > > allocator on a per-packet basis to properly free the buffer. > > > > Alternatively, if there is bookkeeping which is useful to external users of > the > > buffer (size, for example), it could be pushed down to the base Buffer > > implementation and accessed with an inline accessor (like payload). > > > > Finally, if allocators create std::unique_ptr<Allocator::Buffer>s, and > > Allocator::Buffer enforces move-semantics, then we have an extensible package > of > > all the things we need to return the buffer to its owner allocator as well as > > implicit lifetime management, and we can pass just the unique_ptr to the > buffer > > to this class instead of needing to pass all three members which need to stick > > together in order to be coherent. > > That's a worthy design idea. Can we save it for another CL? sure, if you are serious about implementing it, you may want to file a bug to track the idea so that we don't forget (this is what I ended up doing for all of Jeff's design change requests during the AudioServer code review) https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... File services/media/framework/engine.cc (right): https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/engine.cc:234: // LIFO These comments still seem backwards. Both Pop implementation Pop from the front of the deque. PushToSupply's comment says it is LIFO, but it is pushing to the back and pulling from the front (which is FIFO), and vice-versa for PushToDemand (push and pull to front is LIFO, comment says FIFO). Either the comment is wrong or the code is wrong here. Also, while it is great to have the comments (FIFO vs. LIFO) they are a a little vague. They merely assert whether these data structures are stacks or queues, but provide no explanation as to why the FIFO vs. LIFO-ness of the structures are important. This means I can notice the mismatch, but I cannot determine who is correct (the code or the comment). Please update the comment (or add one elsewhere in some more top-level theory-of-operation comment) which explains the reasoning behind the FIFO/LIFO decisions. Finally, its pretty easy to enforce this in code so that no mistakes can be made. Take a look at std::stack and std::queue. They are templateized adapters for std:: containers which support push/pop front/back (vector, deque, list; default it deque) which enforce LIFO and FIFO behavior respectively via their exposed API. This is a much stronger assertion than just having the comment, but an explanation for why to use one vs. the other somewhere in the code would still be appreciated. https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... File services/media/framework/lpcm_util.cc (right): https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/lpcm_util.cc:90: std::memset(buffer, 0, frame_count * stream_type_.channels()); memset operates in units of bytes, so this needs to be frame_count * stream_type_.channels() * sizeof(int16_t) and sizeof(int32_t) for the 32 bit version below. If you go the type_traits route, you can just say sizeof(T) https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/lpcm_util.cc:148: DCHECK(in_byte_count % channels == 0); sorry; I made a mistake in my last comment. This should not be % channels, it should be % bytes_per_frame. (See follow up comment in the header file)
https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/allocator.cc (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/allocator.cc:34: static DefaultAllocator default_allocator; On 2016/01/27 22:35:22, johngro wrote: > On 2016/01/25 23:29:37, dalesat wrote: > > On 2016/01/20 00:25:32, johngro wrote: > > > I think that static-scoped classes are supposed to be marked constexpr to > > insure > > > that they have no runtime initialization. Static and global classes with > > > runtime initialization are disallowed by style to prevent initialization > > > ordering bugs. > > > > I'm not sure what to do here in that case. I can't declare default_allocator > > constexpr, because GetDefault returns a non-const pointer. This is true even > > when I make the constructor constexpr. The compiler won't allow me to also > make > > the method implementations constexpr. I could lazy-create the default > allocator, > > but would require a lock. Suggestions? > > const_cast is your friend. I banged out a simple example below which follows > the pattern you are attempting to implement here. Change any of the #if 0s to > #if 1s to see why the constexpr-ness of the static declaration prevents runtime > side effects during construction/destruction, but still allows non-const member > functions. Also, note that you can still have member variables with > initializers and constructors which take parameters, provided that they are all > evaluate-able at compile time (eg, constexpr). > > uint32_t ThisFunctionIsNotConstexpr() { return 4; } > > class MyStaticBaseClass { > public: > static MyStaticBaseClass* GetDefault(); > virtual void Foo() = 0; > }; > > class MyStaticClass : public MyStaticBaseClass { > public: > #if 0 > constexpr MyStaticClass() { > foo_var_ = ThisFunctionIsNotConstexpr(); > } > #else > constexpr MyStaticClass() { } > #endif > > #if 0 > ~MyStaticClass() { } > #endif > > void Foo() override; > private: > #if 0 > uint32_t foo_var_ = ThisFunctionIsNotConstexpr(); > #else > uint32_t foo_var_ = 5; > #endif > }; > > static constexpr MyStaticClass default_static_class; > > MyStaticBaseClass* MyStaticBaseClass::GetDefault() { > return const_cast<MyStaticClass*>(&default_static_class); > } > > void MyStaticClass::Foo() { > LOG(INFO) << "I was called " << __PRETTY_FUNCTION__ > << " foo_var was " << foo_var_; > } > > int main(int argc, char** argv) { > MyStaticBaseClass::GetDefault()->Foo(); > return 0; > } Done. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/allocator.h (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/allocator.h:21: virtual void* AllocatePayloadBuffer(uint64_t size) = 0; On 2016/01/27 22:35:22, johngro wrote: > On 2016/01/20 00:25:32, johngro wrote: > > <nit> > > for in-proc non-mojom'ed stuff, we should use size_t instead of u64 here. > > </nit> > > Checking in; any thoughts on using size_t instead of uint64_t here? Added a TODO. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/conversion_pipeline_builder.cc (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/conversion_pipeline_builder.cc:192: NOTREACHED() << "conversion requires mixdown/up - not supported"; On 2016/01/27 22:35:22, johngro wrote: > On 2016/01/25 17:47:29, dalesat wrote: > > On 2016/01/20 00:25:32, johngro wrote: > > > why not just LOG(ERROR) or MOJO_LOG(ERROR) instead of NOTREACHED? is it > that > > > the not reached form ASSERTs in the process? > > > > The NOTREACHED will be removed as part of addressing the TODO. In the mean > time, > > I want the code to fail noisily if I try to make it do things that aren't > > implemented. > > Understood, but not really what I was asking. I wanted to know what the > behavior of the macro was, and did it assert/dcheck. > > I researched the answer to my own question; NOTREACHED is defined in base > logging. In a non-chromeOS build, it evaluates to DCHECK(false). DCHECK(false) > (in addition to dchecking) provides a lazy logstream at level DCHECK to eat the > ostream parameters. > > On chromeOS, a special function (LogErrorNotReached) is called, and the ostream > parameters are always discarded. > > TL;DR? Ack Acknowledged. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/engine.cc (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/engine.cc:16: DCHECK(stage_ != nullptr && index < stage_->input_count()); On 2016/01/20 00:25:32, johngro wrote: > FWIW, the 2 parameter form of the Input constructor already DCHECKs this (same > for output below) Acknowledged. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/engine.cc:234: stages_.push_back(std::unique_ptr<Stage>(stage)); On 2016/01/20 00:25:32, johngro wrote: > related to the question about unique pointers earler; why is this becoming > unique now? Isn't the proper pattern to make it unique at construction time and > then to follow move-only semantics everywhere? Using raw pointers here now. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/engine.cc:338: Stage* stage = supply_backlog_.front(); On 2016/01/27 22:35:22, johngro wrote: > On 2016/01/20 00:25:32, johngro wrote: > > the comment above indicates that this is supposed to be a stack (LIFO), but > the > > code here is pushing to the back and popping from the front, which is a queue > > (FIFO) not a stack. > > This still seems backwards, see updated comment in PS#5 around > PushTo(Supply|Demand)BacklogUnsafe Done. https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... File services/media/framework/lpcm_util.h (right): https://codereview.chromium.org/1577953002/diff/20001/services/media/framewor... services/media/framework/lpcm_util.h:30: uint64_t in_byte_count, On 2016/01/27 22:35:22, johngro wrote: > On 2016/01/25 23:29:37, dalesat wrote: > > On 2016/01/25 18:55:43, johngro wrote: > > > in_byte_count seems redundant here. Don't we know everything we need to > know > > > (number of channels in a frame, number of bytes in a sample) from the stream > > > type which was specified at creation time? It this to allow us to > interleave > > > only part of a non-interleaved packet? If so, how do we manage to > interleave > > > the rest of the data later on (we will be offset into the first "plane" and > > > subsequent planes will be larger) > > > > The layout of non-interleaved samples we get from ffmpeg is such that the > > payload is evenly divided into a buffer for each frame (so each buffer is > > in_byte_count / frames_per_sample in size). The samples for a frame are > > contiguous in the buffer starting at the beginning of the buffer, but they > don't > > necessarily fill the buffer. > > k, This is an important detail/assertion; perhaps a comment outlining this would > be a good idea. > > Somewhere there needs to be a > DCHECK((in_byte_count % bytes_per_frame()) == 0); > DCHECK((in_byte_count > bytes_per_frame()) >= frame_count); > > Alternatively, you could keep things in units of frames and have something like > total_frame_count and active_frame_count, in which case all you would need would > be > DCHECK(total_frame_count >= active_frame_count); Done. https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... File services/media/framework/ostream.h (right): https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/ostream.h:36: static std::string by; On 2016/01/27 22:35:22, johngro wrote: > On 2016/01/26 21:17:51, dalesat wrote: > > On 2016/01/26 01:32:39, johngro wrote: > > > Indenter::by could be constexpr. At the very least, it should be const > (which > > > means that it should be named BY, instead of by). > > > > > > Re: level; this is not thread safe, but I assume that you know this. This > > needs > > > to be thread local, not static global, in order to be thread safe. I don't > > know > > > our policy on the C++11 threadlocal keyword and if it is permitted or not. > > > > > > A hack which would keep the counts consistent (by the indenting sometimes > > > incorrect) would be to use std::atomic to manipulate the level in the > > > constructor/destructors, instead of ++/-- > > > > > > Finally, level should probably be private with a static accessor, and > suffixed > > > with _. The only way to manipulate the value level should be via the > > > constructor/destructor. > > > > This stuff is for debugging. I'd rather leave it out of the CL than spend a > lot > > of time getting it perfect, though copying it around from branch to branch > would > > be a hassle. I messed with it for awhile without great results. I switched to > > using custom stream manipulators, which is probably a better pattern, but that > > doesn't really address your concerns. > > So, I don't really understand xalloc/iword/pword super well (I'm not a fan of > ostreams) and I needed to do some reading, but I think you are correct that this > does not address my original concerns. If I am understanding correctly, xalloc > will create a unique index which can be used with the iword member of any > ostream instance to retrieve a long integer (initialized to zero) which is > unique to that ostream instance. > > If I am understanding the base::log ostreams correctly, that means that we have > implemented an indentation level per log stream. The thread safety concerns > remain. Also, xalloc is apparently not thread safe until C++14 (and we are > currently on C++11). > > I agree that this is probably a moot point, as I don't know what level of thread > safety exists in the base or mojo log streams (we are likely to get either > interleaved characters or lines anyway). > > That said, I checked with James regarding thread local storage. What he told me > was... > > 1) Apple prohibits the thread_local keyword (introduced in C++11), but google > and chrome toolchains support it. We allow its use provided your code does not > need to run in an apple environment. For our purposed, this means that if you > do not expect this code to be linked against Flutter, you can use thread_local. > 2) If you want to be double sure, you can use base::ThreadLocalPointer; this > should work on all platforms. > > So, your original code would be perfectly fine and thread safe (no atomics > needed) by just saying > > static thread_local int level; > > instead (provided you didn't need to link against flutter). A small amount of > more work would be needed to make the base::ThreadLocalPointer class work, but > from glancing at the code, it seems like this would be pretty easy to manage > either manually or with a small templated wrapper class (so you could say > something like ThreadLocal<uint32_t> level(0)) > > It's fine if you want to leave it like this, just trying to give some options. Thanks for digging in. https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... File services/media/framework/packet.h (right): https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/packet.h:37: bool end_of_stream, On 2016/01/27 22:35:22, johngro wrote: > On 2016/01/26 21:17:51, dalesat wrote: > > On 2016/01/26 01:32:39, johngro wrote: > > > <design_point note="No code change requested> > > > PTS, duration and EOS are all pieces of metadata for a Packet (payload, > media > > > access unit, whatever), but do not represent the sum total of per-packet > > > metadata we may wish to express. Other pieces of metadata which can be > useful > > > for some (but not all) packets include things like... > > > > > > 1) Random-access-point, drop-able flags for codecs with interpacket > > > dependencies. > > > 2) Colorspaces, packing format (subsampling, plane construction, etc..) for > > raw > > > frames of video. > > > 3) Mandated downmix matricies (or references to them) for decoded audio. > > > 4) Language codes for subpicture packets. > > > > > > and so on. It may be good to consider decoupling the metadata from packet > and > > > having the packet just hold onto a reference to the metadata. > > > </design_point> > > > > > > Stepping back from the design question, I could be convinced that the one > > piece > > > of universal metadata which is useful to all packets is the presentation > > > timestamp. Even though it sometimes takes on the special value "Unknown", > it > > is > > > so commonly accessed that I can understand hoisting it up to the class > Packet > > > level of things and using a sentinel value for "Unknown". In this case, why > > are > > > we accessing the PTS via a virtual function? Wouldn't it be better to make > > the > > > timestamp a private member of the base Packet class (accessible using an > > inline > > > public accessor) and require its definition by the subclass using an > explicit > > > protected constructor? > > > > Ack comments about more metadata. > > > > Are you concerned about performance issues with PTS? The entire thing is > > abstract, because some implementations are backed by structures defined by > e.g. > > ffmpeg (AVFrame and AVPacket are examples). > > mildly; its one of those things which feel like a coin flip now, but later who > knows. If the timestamp on a packet once created is immutable (a principal I > think we both agree is a worthwhile goal), then it should be simple to have the > protected constructor of Packet demand that a PTS be provided at construction > time. I could be then accessed with a inline const accessor for pretty much no > cost. > > If the underlying packet is going to undergo transformations which alter the > PTS, and this is going to happen inside the context of something like an ffmpeg > operation on an encapsulated ffmpeg structure, then this technique will not work > without a bunch of manual labor which will be hard to maintain. > > Finally, if we are directly accessing the internal ffmpeg representation of an > encapsulated PTS, then we are being implicitly forced to use the same sentinel > value for no PTS that ffmpeg uses. If we are operating under the non-volatile > PTS assumption given above, and choose to cache the PTS locally in this base > class for inline access, then we can map from their value to our value as needed > at construction time. If not, then we need to be sure that the values are the > same. If they are already, then good. If not, then we can change ours to match > theirs provided there are no design concerns about their choice (I would not use > 0 or -1 as my sentinel; either 0x8000... or 0x7FFF... seem like better choices). > Either way, we should have a static_assert somewhere in the > FfmpegBackedPacketImpl code which asserts that their value matches our value > (IIRC, they use a #def called NO_PTS) Looking at my ffmpeg code, it appears that demuxes are always giving us timestamps (so far, at least). The decoders don't, but they're using the vanilla Packet implementation. I add presentation time myself, in that case. https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/packet.h:40: Allocator* allocator); On 2016/01/27 22:35:22, johngro wrote: > On 2016/01/26 21:17:51, dalesat wrote: > > On 2016/01/26 01:32:39, johngro wrote: > > > Looking at the implementation (next file), it seems like we have 3 things > here > > > (size, payload and allocator) which all belong together. The Packet class > is > > > going to hold a pointer which came from an allocator and refers to a > > particular > > > size allocation. When it is time to release, the Packet is going to call > the > > > allocator's free equivalent. > > > > > > If any of these things are mismatched, Bad Things could happen. We don't > want > > > to return a payload to the incorrect allocator, we don't want the size to be > > off > > > when we do so, and so on. > > > > > > Would it be a good idea to introduce the concept of an Allocator::Buffer? > All > > > Allocator::Buffers would have a void* member which points to the buffer > memory > > > itself. For a default allocator (based on malloc/free or new/delete), this > > > would be all that is needed. For other Allocator implementations, their > > Buffer > > > implementation could contain a reference to the allocator which generated > the > > > Buffer as well as any bookkeeping (like size) which would be needed by the > > > allocator on a per-packet basis to properly free the buffer. > > > > > > Alternatively, if there is bookkeeping which is useful to external users of > > the > > > buffer (size, for example), it could be pushed down to the base Buffer > > > implementation and accessed with an inline accessor (like payload). > > > > > > Finally, if allocators create std::unique_ptr<Allocator::Buffer>s, and > > > Allocator::Buffer enforces move-semantics, then we have an extensible > package > > of > > > all the things we need to return the buffer to its owner allocator as well > as > > > implicit lifetime management, and we can pass just the unique_ptr to the > > buffer > > > to this class instead of needing to pass all three members which need to > stick > > > together in order to be coherent. > > > > That's a worthy design idea. Can we save it for another CL? > > sure, if you are serious about implementing it, you may want to file a bug to > track the idea so that we don't forget (this is what I ended up doing for all of > Jeff's design change requests during the AudioServer code review) Acknowledged. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/parts/file_reader.h (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/parts/file_reader.h:37: FILE* file_; On 2016/01/26 01:32:39, johngro wrote: > I don't think that we should be using stdio FILE structure here. If we are > going to continue to use them here, we should probably bury that fact using a > FileReader/FileReaderImpl pattern (where FileReaderImpl is declared in the .cc > file) > > In general, I think we really want to... > > 1) be designing our classes to use the underlying Mojo services for things like > files and network access. > 2) Design all of our I/O classes with an async IO model in mind, not a > synchronous one. > > There are multiple ways to fake out ffmpeg's transport interfaces if we go this > route. The simplest way would be to use a mojo service to get a data pipe and > then just use the data pipe in a synchronous. There are other ways (which > usually involve a dedicated thread) to fake out ffmpeg so it feels that it is > accessing data synchronously when it is actually async underneath at the > transport implementation level. Right...future CL? https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/parts/lpcm_reformatter.cc (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/parts/lpcm_reformatter.cc:14: switch (in_type.sample_format()) { On 2016/01/26 23:47:29, johngro wrote: > up to you, but you can reduce the amount of duplication of the inner switch > block here by templating this method. The idea would be... > > 1) Move the class definition of LpcmReformatterImpl into this transitional unit. > You can stuff it into an anon namespace if you want to really bury it. > 2) Make the impl class's constructor public. > 3) Make a static function templated on typename TIn. Have it implement the > inner switch to select the out type, and use the passed template parameter to > select the in type. > 4) Implement the outer switch in LpcmReformatter::New, use it to select the in > type and then call the templated method to select the out type. Acknowledged. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/parts/lpcm_reformatter.cc:128: inline void CopySample(uint8_t* dest, float* source) { On 2016/01/26 23:47:29, johngro wrote: > Source needs to be clamped to the range [-1.0, 1.0] here. This goes for all of > the instances of CopySample which take float as a source. During the demo, this > was causing some clicks and pops as the decoder spit out samples slightly above > and below [-1.0, 1.0] > > FWIW - the same can be said for the 24-in-32 source format as well since, like > the float, not all of the input value are valid for the encoding. Long term, I > suppose we need to decide whose job this ultimately is, but in the short term, > we have badly behaved codecs and no one fixing up their crap. > > In case its helpful, I just ended up defining an inline templated clamp function > and applying it to the formats with float sources. I skipped the 24-in-32 > formats as they were not relevant to the demo, but it should be simple enough to > use the same function. > > template <typename T> > inline constexpr T Clamp(T val, T min, T max) { > return (val > max) ? max : ((val < min) ? min : val); > } > > inline void CopySample(uint8_t* dest, float* source) { > *dest = static_cast<uint8_t>((Clamp(*source, -1.0f, 1.0f) * 0x7f) + 128); > } > > inline void CopySample(int16_t* dest, float* source) { > *dest = static_cast<int16_t>(Clamp(*source, -1.0f, 1.0f) * 0x7fff); > } > > inline void CopySample(int32_t* dest, float* source) { > *dest = static_cast<int32_t>(Clamp(*source, -1.0f, 1.0f) * 0x7fffff); > } > Done. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/parts/lpcm_reformatter.cc:255: if (mix) { On 2016/01/26 23:47:29, johngro wrote: > if you are going to put the mix decision in here, you should put it outside of > the loop, not inside. The compiler may attempt to make this optimization for > you, but this may be getting a bit complicated for it. > > Alternatively, if you make a private template of this method which takes a bool > DoMix parameter, you can call if from the public method where mix is decided at > runtime. Then, whether you put the if (DoMix) inside or outside of the loop the > compiler will kill the unused path in each of the generated copies of the > template expansion. The outer loop is per-channel. The test will happen approximately twice per call to TransformFrames, which I'm OK with. I think the best solution involves the mix template parameter, but I don't want to invest too heavily in optimization until we have the time to run performance tests. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/parts/lpcm_reformatter.h (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/parts/lpcm_reformatter.h:27: return LpcmReformatterPtr(NewImpl(in_type, out_type)); On 2016/01/26 23:47:29, johngro wrote: > What is the advantage of having an inline public new which just calls a private > NewImpl? Why not just a public New whose implementation lives in the .cc file? Done. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/parts/lpcm_reformatter.h:39: class LpcmReformatterImpl : public LpcmReformatter { On 2016/01/26 23:47:29, johngro wrote: > You can just hide this class in the .cc file. There is no need to expose it's > insides to the world. Done. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/parts/null_sink.cc (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/parts/null_sink.cc:24: demand_callback(Demand::kNeutral); On 2016/01/26 23:47:29, johngro wrote: > You are making a demand callback inside the RegisterDemand call for an > ActiveSink; is this OK? I thought that the model was that the initial demand > got expressed when the engine called Prime on all of the participants at > startup. > > Seems like the demand callback instance should be stored in the sink and called > once during prime. > > While we are on the subject, shouldn't ActiveSink have a protected > demand_callback_ member and implement RegisterDemandCallback itself? The method > could be made virtual if a derived class had custom work to do during the > registration/link-up phase, but it feels like this is really just bookkeeping, > and that any startup actions should take place during Prime. Good catch re Prime. I missed that when I added Prime to sinks in general. I accommodate calling the callback in RegisterDemandCallback, but perhaps I shouldn't. As for doing this in the base class, we don't know in general when demand should transition away from kNegative or whether it should transition to kNeutral or kPositive. If we did know that, I'd rather implement that default behavior in the stage and leave the model pure virtual. The models are the framework's contracts with 'part' developers, and the behavior of model implementations should be entirely the business of those developers. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/ptr.h (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/ptr.h:36: UniquePtr Clone() const { return *this ? this->get()->Clone() : UniquePtr(); } On 2016/01/26 23:47:29, johngro wrote: > Do we really want to sub-class std::unique_ptr to get this functionality? > Mostly, this just allows us to say.. > > UniquePtr<ClassA> obj = make_unique<ClassA>(...); > UniquePtr<ClassA> cloned = obj.Clone(); > > Instead of... > > UniquePtr<ClassA> cloned = obj->Clone(); > > It also provides an automatic null check and will produce a new NULL unique_ptr > if the cloned pointer is NULL. I'm not sure I see the value in this (why is > someone trying to clone an object which is not there?), but if you really want > this behavior, you should be able to say something like... > > template <typename T> > std::unique_ptr<T> SafeClone(const std::unique_ptr<T> obj) { > return obj ? obj->Clone() : nullptr; > } > > and the code above becomes > > std::unique_ptr<ClassA> cloned = SafeClone(obj); > > without needing to extend std::unique_ptr Yes, this is just to get rid of the null check. I've added a TODO to get rid of it. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/ptr.h:39: // shared_ptr with upcast to TBase. On 2016/01/26 23:47:29, johngro wrote: > Why do this? std::shared_ptr already has 3 static methods which handle static, > dynamic and const casting for you. To do what you are doing with the overloaded > operator below, you can just do things like... > > std::shared_ptr<MyDerived> derived = std::make_shared<MyDerived>(); > std::shared_ptr<MyBase> base = std::static_pointer_cast<MyBase>(derived); > GiveBaseToSomeoneElse(base); > > This is permitted for both up and down casting. Clearly you need to know what > you are doing when down casting, since we don't use RTTI and cannot use dynamic > cast. > > Also, if you are not going to give the pointer to someone else (who might retain > it), and you don't want to make a temp shared_ptr, you can just cast the > underlying pointer. Obviously, you would only ever need to do this in a > down-cast scenario > > void GiveBaseToSomeoneElse(const std::shared_ptr<MyBase>& base) { > // Danger, we had better know that base is actually a MyDerived > MyDerived* derived = static_cast<MyDerived*>(base.get()); > derived->MethodOnlyInMyDerived(...); > } I wasn't aware of static_pointer_cast. This class is really about making this Engine method work: template<typename T, typename TBase> Part Add(SharedPtr<T, TBase> t) { DCHECK(t); return Add(CreateStage(std::shared_ptr<TBase>(t))); } I tried the following, but the compiler couldn't infer TBase. template<typename T, typename TBase> Part Add(std::shared_ptr<T> t) { DCHECK(t); return Add(CreateStage(std::static_pointer_cast<TBase>(t))); } I've added a TODO for getting rid of this, if possible. I want the compiler to figure out TBase, because some code calls Engine::Add without knowing what TBase is. If that's possible, I'm fine with getting rid of this class. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/result.h (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/result.h:13: // Abstract base class for objects that allocate buffers for packets. On 2016/01/26 23:47:29, johngro wrote: > cut-n-paste error, this is not the comment you are looking for <jedi_wave/> Good eye, thanks! https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/result.h:14: enum class Result { On 2016/01/26 23:47:29, johngro wrote: > So, I did the same thing (defined a media result in .mojom) and got chewed out > by Trung and James. Trung has a vision for an error code system that he wants > us all to use. I had difficulty figuring out how it was going to solve my > problems, but they made me comment all of my codes with which one of Trung's > codes they would become someday. > > No action requested here, but we may want to consider defining these in .mojom > so they can be used over mojom interfaces. When we revisit, we should probably > take a second look Trung's proposal and make an effort to make use of it. I currently aspire to make the framework free of mojo dependencies. If we give up on that, I'm happy to use MojoResult or MediaResult. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/stages/active_sink_stage.h (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stages/active_sink_stage.h:17: class ActiveSinkStage : public SinkStage, public SingleInputStage { On 2016/01/26 23:47:29, johngro wrote: > see comments in stage.h about MI, this applies to most of the stage/* classes Acknowledged. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/stages/stage.h (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stages/stage.h:44: using UpdateCallback = std::function<void(Stage* stage)>; On 2016/01/26 23:47:29, johngro wrote: > public typedefs go at the top of the class. see > https://google.github.io/styleguide/cppguide.html#Declaration_Order OK. Someone should tell the mojo compiler. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stages/stage.h:68: class SourceStage : public virtual Stage { On 2016/01/26 23:47:29, johngro wrote: > so,this type of MI is pretty much banned by google3 style and *strongly* > discouraged by the chrome style (which is based on google3). > > https://google.github.io/styleguide/cppguide.html#Multiple_Inheritance > > Is there another way that this could be decomposed to avoid the need for MI with > diamond inheritance? Perhaps Jeff has some ideas? The goal here was to make stage authoring a bit easier by providing some default behavior that can be called out via inheritance. Probably the best way to get rid of the MI is just to copy the implementations of these things...or use macros. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/stages/stage_input.h (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stages/stage_input.h:33: void connect(Stage* upstream_stage, uint32_t output_index) { On 2016/01/26 23:47:30, johngro wrote: > It would be good to DCHECK that we are not already connected, and that the > connection we are making is valid (eg, not nullptr). Done. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stages/stage_input.h:55: return must_allocate_; On 2016/01/26 23:47:30, johngro wrote: > must_allocate_ is only really defined after Prepare gets called. It would be > good to be able to assert that the StageInput had been properly prepared before > people start to call the allocator or must_allocate accessors. > > It would be convenient to be able to use allocator_'s not-null-ness to test for > this, but currently, allocator_ is permitted to be nullptr, so that won't work. > One idea (mentioned in my feedback in allocator.h) would be to disallow a > nullptr allocator. This would require callers of Prepare to (at least) provide > the default allocator. > > If this is not workable, a state could be introduced (either a bool flag, or a > state enum) to note that the system had been properly prepared before these > calls got made. Since this serves to purpose aside from debugging, it would > need to be put under a debug build #def conditional, which is something of a > PITA. My thinking regarding allocator() was that it could return a preferred (but not required) allocator when must_allocate() returns false. If we don't think we'll ever need this, we can get rid of must_allocate() and return an allocator if and only if the connected output is required to use it. I've added a DCHECK so this isn't called when the input's stage isn't prepared. That's not exactly the same as the input being prepared, but it's pretty close. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stages/stage_input.h:74: Stage* upstream_stage_; On 2016/01/26 23:47:30, johngro wrote: > FWIW, you can initialize PoD members at declaration time in C++11 now. This is > particularly useful when you have multiple constructors (not the case here), but > it can help readability as well, IMO. Thanks for pointing that out...I wasn't aware. This is something I really missed from C#. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/stages/stage_output.cc (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stages/stage_output.cc:50: if (demand_ == Demand::kNegative || mate().packet_from_upstream()) { On 2016/01/26 23:47:30, johngro wrote: > this check (demand == negative) is redundant. It will fall out naturally from > the return demand_ below if the mate does not have a packet. I did this to avoid the second conditional when the first succeeds. Adding a comment. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/stages/stage_output.h (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stages/stage_output.h:56: void supply_packet(PacketPtr packet, Engine* engine) const; On 2016/01/26 23:47:30, johngro wrote: > this method is not a simple accessor, it takes action. It should probably be > named SupplyPacket. > > Same for update_demand_internal below. Also, since it is in the public scope, > it should probably be just UpdateDemand, not UpdateDemandInternal. Done. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stages/stage_output.h:66: void supply_packet_internal(PacketPtr packet, Engine* engine) const; On 2016/01/26 23:47:30, johngro wrote: > SupplyPacketInternal Done. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/stream_type.h (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stream_type.h:6: #define SERVICES_MEDIA_FRAMEWORK_STREAM_TYPE_H_ On 2016/01/26 23:47:30, johngro wrote: > top level comment. > > This whole file feels like a custom, in-proc-centric C++ implementation of the > media types which you spec'ed in mojom way back when. > > Why are we not using the mojom generated classes here? How are we going to > manage this when we have to start to inter-operate with media services > (potentially implemented in other languages) across mojo transport boundaries? > > If we choose to do anything at all about this in the short term, it should > probably be handled with a follow-up change, but I suspect that this will start > to be an issue in the short-to-mid term. I did this for four reasons: 1) to avoid mojo dependencies 2) because these classes are easier to use than the mojo ones 3) I can implement useful code in these classes 4) to allow internal support for things that aren't supported in mojo. This last item was important when I was getting the interleaved flag out of the mojo defs. I've since removed it from here as well. If we want internal support for e.g. kSigned16In32, reason 4 will become valid again. If none of these reasons bear out, I will do the work to get rid of this. I get that there's significant maintenance cost. If this becomes a problem for you, let me know, and I will do the work. Interop is a non-issue as the things that get moved across mojo are the mojo-generated things. To the degree that those are extensible, these must be as well (e.g. when we switch to using strings instead of enums for some values). https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stream_type.h:33: StreamTypes(size_t size) : std::vector<StreamTypePtr>(size) {} On 2016/01/26 23:47:30, johngro wrote: > if we are imposing move semantics on this class, this constructor should be > private, and the copy constructor should be removed. I think that the Mojo > framework already has a handy macro for this, but I'm not sure. > > If we are not imposing move semantics on this class, then we should probably > just say... > > using StreamTypes = std::vector<StreamTypePtr>; > > and leave it at that. This seems to apply to all of the extensions of > std::vector below. They all seem to add New and Clone to the mix to force > unique_ptr and move patterns, but they all expose a direct constructor as well. I'm not enforcing move semantics, but I do support them. There are plenty of cases in which classes have StreamType members that are only exposed as const StreamType&. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stream_type.h:47: static BytesPtr New(uint8_t* data, size_t size) { On 2016/01/26 23:47:30, johngro wrote: > DCHECK(data || !size); > > memcpy's behavior when passing nullptr for the source is not super consistent > across std C library implementations, but its probably pretty safe to call it > with nullptr, provided that size is zero. Done. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stream_type.h:53: Bytes(size_t size) : std::vector<uint8_t>(size) {} On 2016/01/26 23:47:30, johngro wrote: > explicit Bytes(size_t size) ... > > also, this should probably be private. Same for the copy constructor below. Leaving public, otherwise done. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stream_type.h:57: const uint8_t* data() const { return &(*this)[0]; } On 2016/01/26 23:47:30, johngro wrote: > Why is the base class's implementation of data (here and below) not good enough > for this job? Wasn't aware that vector had that. Done. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stream_type.h:109: Range(T min_param, T max_param) : min(min_param), max(max_param) {} On 2016/01/26 23:47:30, johngro wrote: > DCHECK(min <= max); Done. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stream_type.h:114: bool contains(T t) const { On 2016/01/26 23:47:30, johngro wrote: > inline, constexpr > > Also, this is probably only for primitive data types, by you might want to spec > this as const T& instead of passing T by value. When the method gets inlined, > the compiler will "do the right thing" for PoDs. Done. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stream_type.h:149: ~StreamTypeSet(); On 2016/01/26 23:47:30, johngro wrote: > virtual destructor. I'm pretty sure that style mandates that you always have a > virtual destructor if you have virtual members. > > Also in the derived classes. Done. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stream_type.h:155: virtual const MultiplexedStreamTypeSet* multiplexed() const; On 2016/01/26 23:47:30, johngro wrote: > If only we allowed dynamic_cast, you would not need to do this. Acknowledged. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stream_type.h:447: class VideoStreamType : public StreamType { On 2016/01/26 23:47:30, johngro wrote: > I think that we will need to revisit this at some point in time. I'm not all > that sure that using enums in header files is a long term scale-able solution to > communicating this information (this really starts to show as you start to > enumerate all of the profiles of H.264 below, but have not mixed in their > concept of levels as well) > > The pixel packing section has related issues. In addition to needing to > enumerate all of the combinations in the world (ARGB, ABGR, BGRA, etc...), just > saying that a frame is YUYV can miss hardware imposed line stride requirements > (I420 as issues with both line and plane stride, etc...) > > No change requested now, but I think we need to come back to this and sit down > with the graphics team at some point and figure out a final solution. It may > end up being enums, but I'm not convinced that this will be the proper thing to > do in the long run. I agree. I dropped these into the mojom in an attempt to get an ffmpeg video decoder working isolated from the ffmpeg demux. That effort was sidelined to get the framework ready for the mixer. Many of these should be strings for extensibility. If we can do without some, that would be great. The bottom line is that I need to be able to reconstruct an ffmpeg decoder context across mojo using (the mojom version of) this representation. We could make some aspects opaque in the mojom definitions. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stream_type.h:521: VideoStreamType( On 2016/01/26 23:47:30, johngro wrote: > again, something to revisit later on, but I am not convinced that these are > necessarily properties of the video stream. Things like the colorspace, the > width/height, the active video region, the pixel aspect ratio and so on can > actually change on a per-frame basis. > > Whether or not we support this at the per-frame level or we support this by > tearing down and rebuilding video sub-streams in a seamless fashion (or through > some completely other means) is something I am not clear on, but it is something > I have been burnt on when attempting to use other media frameworks to create > broadcast video devices in the past. > > I tossed out some starting points for how this might be approached in the > video-pipe strawman doc I wrote for James forever ago, but I'm not certain that > anyone ever took the time to read through it. Acknowledged. https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... File services/media/framework/engine.cc (right): https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/engine.cc:234: // LIFO On 2016/01/27 22:35:22, johngro wrote: > These comments still seem backwards. Both Pop implementation Pop from the front > of the deque. PushToSupply's comment says it is LIFO, but it is pushing to the > back and pulling from the front (which is FIFO), and vice-versa for PushToDemand > (push and pull to front is LIFO, comment says FIFO). > > Either the comment is wrong or the code is wrong here. Also, while it is great > to have the comments (FIFO vs. LIFO) they are a a little vague. They merely > assert whether these data structures are stacks or queues, but provide no > explanation as to why the FIFO vs. LIFO-ness of the structures are important. > This means I can notice the mismatch, but I cannot determine who is correct (the > code or the comment). Please update the comment (or add one elsewhere in some > more top-level theory-of-operation comment) which explains the reasoning behind > the FIFO/LIFO decisions. > > Finally, its pretty easy to enforce this in code so that no mistakes can be > made. Take a look at std::stack and std::queue. They are templateized adapters > for std:: containers which support push/pop front/back (vector, deque, list; > default it deque) which enforce LIFO and FIFO behavior respectively via their > exposed API. This is a much stronger assertion than just having the comment, > but an explanation for why to use one vs. the other somewhere in the code would > still be appreciated. Thanks for the tip on queue/stack. They don't have clear(), but are otherwise just the ticket. I've added a comment as to why these are queue and stack - it basically says that I guessed. There's also a TODO to determine what ordering is optimal and implement that. It's possible that it doesn't matter. I also see a potential issue with having two backlogs. One may be adequate. https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... File services/media/framework/lpcm_util.cc (right): https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/lpcm_util.cc:90: std::memset(buffer, 0, frame_count * stream_type_.channels()); On 2016/01/27 22:35:22, johngro wrote: > memset operates in units of bytes, so this needs to be > > frame_count * stream_type_.channels() * sizeof(int16_t) > > and sizeof(int32_t) for the 32 bit version below. > > If you go the type_traits route, you can just say sizeof(T) stream_type_ has bytes_per_frame(). Using that. Thanks! https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/lpcm_util.cc:148: DCHECK(in_byte_count % channels == 0); On 2016/01/27 22:35:22, johngro wrote: > sorry; I made a mistake in my last comment. This should not be % channels, it > should be % bytes_per_frame. (See follow up comment in the header file) Hmmm. I guess this is true because the samples are guaranteed to be on sample boundaries and there are 'channels' buffers.
first round of feedback about thread safety modifications. TL;DR? I have concerns about the locking granularity (one master lock) and the scope with which the lock is held. A writeup describing the syncronization domains, assumptions being made and how those assumptions are enforced (currently and moving forward) would go a long way towards easing my concerns. https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... File services/media/framework/allocator.cc (right): https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/allocator.cc:49: DCHECK(buffer); Given that allocate is NOTREACHED, shouldn't this be NOTREACHED as well? Also, in order to assert that size > 0 here, we should do so as well in AllocatePayloadBuffer. That said, since we never give out a buffer anyway, is there any real reason to assert anything about the size of the failed allocation which is (for some reason) being released? Finally, since the Null allocator never gives out an actual buffer (it always returns nullptr), this should probably be DCHECK(!buffer) or DCHECK(buffer == nullptr). It would be bit odd, but not unthinkable that someone is attempting to return the failed allocation to the Null allocator; but if they attempt to do so, the pointer they are "returning" had better be nullptr. https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/allocator.cc:53: static NullAllocator null_allocator; quick reminder, remember to make the Null allocator constexpr as well when you take care of the Default allocator. https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... File services/media/framework/allocator.h (right): https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/allocator.h:19: // Gets the default allocator, which allocates vanilla memory from the heap. cut-n-paste comment. This needs to be updated https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... File services/media/framework/engine.cc (right): https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/engine.cc:208: lock_.Release(); so, you made a copy of the sinks_ list which is holding raw pointers to the stages which are classified as sinks, then you release the lock and call prime on each sink. What prevents another thread from calling Remove on a stage which exists in the sink collection (or from Resetting the entire Engine) and deleting the sink stage out from under you while you are doing this? https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/engine.cc:353: stage->Prepare(update_function_); Following comment applies to UpdateUnsafe as well So, at this point you are holding your master lock, and calling into another function implemented by the stage (which will most likely execute logic implemented by the part contained within the stage). This raises a number of concerns. How does the stage/part call back into the engine to queue new work and not re-enter the lock. If the stage/part acquires an internal lock, what it to prevent an A/B, B/A lock ordering issue in the interaction between the stage/part and the engine. IOW, There seems to be a significant danger that we engine.lock then part.lock during update, and something asynchronous happens inside of the part (interrupt fires or something) and the part code attempts to part.lock then engine.do_something_which_requires_the_lock, resulting in a deadlock hazard. I'm not going to be dogmatic and say that locks should never be held when calling other functions, but if we are going to do so, then we need to be very certain of our assumptions about things like lock domains, lock ordering and exactly what the other code will do while holding our lock. Whenever possible, we should do our best to break down the scope of where locks are held, both to increase parallelism and decrease the risk of deadlock. This particular pattern of holding a master engine lock while calling out into the modular stage/part world makes me very nervous. Is there any way that we can decompose this so that we do not have to hold the lock while we call into what amounts to external code? https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... File services/media/framework/engine.h (right): https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/engine.h:213: void PushToDemandBacklogUnsafe(Stage* stage); The unsafe methods are the methods which assume that the lock is held, yes? If so, then shouldn't they all need to be private? https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/engine.h:266: mutable base::Lock lock_; Now that you are adding thread safety and multi threading capabilities to the Engine concept, it would be really good to have a comment (probably at the top of the class) which gives a brief theory of operation and outlines the threading assumptions. Questions I have that I'd like to have answers spelled out to are... + Is there an assumption that the execution of the jobs pending in the backlog queues assumed to execute in some form of serialized context (eg; sort of single threaded, but perhaps pumped by multiple threads, only 1 at a time) + What does the lock protect? Is it just the backlog queues, or is it the entire update cycle? + If the engine is holding the lock over the entire update cycle, how does a stage safely queue new work or update demand during an update cycle?
https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... File services/media/framework/allocator.cc (right): https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/allocator.cc:49: DCHECK(buffer); On 2016/01/28 19:14:54, johngro wrote: > Given that allocate is NOTREACHED, shouldn't this be NOTREACHED as well? > > Also, in order to assert that size > 0 here, we should do so as well in > AllocatePayloadBuffer. That said, since we never give out a buffer anyway, is > there any real reason to assert anything about the size of the failed allocation > which is (for some reason) being released? > > Finally, since the Null allocator never gives out an actual buffer (it always > returns nullptr), this should probably be DCHECK(!buffer) or DCHECK(buffer == > nullptr). It would be bit odd, but not unthinkable that someone is attempting > to return the failed allocation to the Null allocator; but if they attempt to do > so, the pointer they are "returning" had better be nullptr. NullAllocator is used when the packet packet payload didn't actually come from an allocator. No one should call AllocatePayloadBuffer and ReleasePayloadBuffer should do nothing. In retrospect, it would be simpler if Packet was OK not having an allocator. I'll get rid of this. https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/allocator.cc:53: static NullAllocator null_allocator; On 2016/01/28 19:14:55, johngro wrote: > quick reminder, remember to make the Null allocator constexpr as well when you > take care of the Default allocator. Done. https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... File services/media/framework/allocator.h (right): https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/allocator.h:19: // Gets the default allocator, which allocates vanilla memory from the heap. On 2016/01/28 19:14:55, johngro wrote: > cut-n-paste comment. This needs to be updated Done. https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... File services/media/framework/engine.cc (right): https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/engine.cc:208: lock_.Release(); On 2016/01/28 19:14:55, johngro wrote: > so, you made a copy of the sinks_ list which is holding raw pointers to the > stages which are classified as sinks, then you release the lock and call prime > on each sink. > > What prevents another thread from calling Remove on a stage which exists in the > sink collection (or from Resetting the entire Engine) and deleting the sink > stage out from under you while you are doing this? At this point, we are depending on the kindness of the caller. Adding a TODO. https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/engine.cc:353: stage->Prepare(update_function_); On 2016/01/28 19:14:55, johngro wrote: > Following comment applies to UpdateUnsafe as well > > So, at this point you are holding your master lock, and calling into another > function implemented by the stage (which will most likely execute logic > implemented by the part contained within the stage). > > This raises a number of concerns. > How does the stage/part call back into the engine to queue new work and not > re-enter the lock. > > If the stage/part acquires an internal lock, what it to prevent an A/B, B/A lock > ordering issue in the interaction between the stage/part and the engine. > IOW, There seems to be a significant danger that we engine.lock then part.lock > during update, and something asynchronous happens inside of the part (interrupt > fires or something) and the part code attempts to part.lock then > engine.do_something_which_requires_the_lock, resulting in a deadlock hazard. > > I'm not going to be dogmatic and say that locks should never be held when > calling other functions, but if we are going to do so, then we need to be very > certain of our assumptions about things like lock domains, lock ordering and > exactly what the other code will do while holding our lock. Whenever possible, > we should do our best to break down the scope of where locks are held, both to > increase parallelism and decrease the risk of deadlock. > > This particular pattern of holding a master engine lock while calling out into > the modular stage/part world makes me very nervous. Is there any way that we > can decompose this so that we do not have to hold the lock while we call into > what amounts to external code? The threading model will change dramatically as time goes on. This CL is not the place to redesign it. I've added a lengthy comment to engine.h that talks about the current constraints. It will be fine for now. In general, let's keep comments to the effect of "You haven't done everything that will ever need to be done concerning issue X in this CL" to a minimum. We should make sure issues are captured and move ahead. https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... File services/media/framework/engine.h (right): https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/engine.h:213: void PushToDemandBacklogUnsafe(Stage* stage); On 2016/01/28 19:14:55, johngro wrote: > The unsafe methods are the methods which assume that the lock is held, yes? If > so, then shouldn't they all need to be private? These are called by inputs and outputs, which are part of the Engine/Stage/StageInput/StageOutput system. I'll make these private and have the input and output classes listed as friends. https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/engine.h:266: mutable base::Lock lock_; On 2016/01/28 19:14:55, johngro wrote: > Now that you are adding thread safety and multi threading capabilities to the > Engine concept, it would be really good to have a comment (probably at the top > of the class) which gives a brief theory of operation and outlines the threading > assumptions. Questions I have that I'd like to have answers spelled out to > are... > > + Is there an assumption that the execution of the jobs pending in the backlog > queues assumed to execute in some form of serialized context (eg; sort of single > threaded, but perhaps pumped by multiple threads, only 1 at a time) > + What does the lock protect? Is it just the backlog queues, or is it the > entire update cycle? > + If the engine is holding the lock over the entire update cycle, how does a > stage safely queue new work or update demand during an update cycle? Done.
LGTM minor comments remaining (spelling, requests for tracking issues, etc) I just checked, Jeff is actively looking at this too. Please give him a bit of time to get his view in on all of this before pulling the trigger. https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... File services/media/framework/packet.h (right): https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/packet.h:37: bool end_of_stream, On 2016/01/28 18:49:15, dalesat wrote: > On 2016/01/27 22:35:22, johngro wrote: > > On 2016/01/26 21:17:51, dalesat wrote: > > > On 2016/01/26 01:32:39, johngro wrote: > > > > <design_point note="No code change requested> > > > > PTS, duration and EOS are all pieces of metadata for a Packet (payload, > > media > > > > access unit, whatever), but do not represent the sum total of per-packet > > > > metadata we may wish to express. Other pieces of metadata which can be > > useful > > > > for some (but not all) packets include things like... > > > > > > > > 1) Random-access-point, drop-able flags for codecs with interpacket > > > > dependencies. > > > > 2) Colorspaces, packing format (subsampling, plane construction, etc..) > for > > > raw > > > > frames of video. > > > > 3) Mandated downmix matricies (or references to them) for decoded audio. > > > > 4) Language codes for subpicture packets. > > > > > > > > and so on. It may be good to consider decoupling the metadata from packet > > and > > > > having the packet just hold onto a reference to the metadata. > > > > </design_point> > > > > > > > > Stepping back from the design question, I could be convinced that the one > > > piece > > > > of universal metadata which is useful to all packets is the presentation > > > > timestamp. Even though it sometimes takes on the special value "Unknown", > > it > > > is > > > > so commonly accessed that I can understand hoisting it up to the class > > Packet > > > > level of things and using a sentinel value for "Unknown". In this case, > why > > > are > > > > we accessing the PTS via a virtual function? Wouldn't it be better to > make > > > the > > > > timestamp a private member of the base Packet class (accessible using an > > > inline > > > > public accessor) and require its definition by the subclass using an > > explicit > > > > protected constructor? > > > > > > Ack comments about more metadata. > > > > > > Are you concerned about performance issues with PTS? The entire thing is > > > abstract, because some implementations are backed by structures defined by > > e.g. > > > ffmpeg (AVFrame and AVPacket are examples). > > > > mildly; its one of those things which feel like a coin flip now, but later who > > knows. If the timestamp on a packet once created is immutable (a principal I > > think we both agree is a worthwhile goal), then it should be simple to have > the > > protected constructor of Packet demand that a PTS be provided at construction > > time. I could be then accessed with a inline const accessor for pretty much > no > > cost. > > > > If the underlying packet is going to undergo transformations which alter the > > PTS, and this is going to happen inside the context of something like an > ffmpeg > > operation on an encapsulated ffmpeg structure, then this technique will not > work > > without a bunch of manual labor which will be hard to maintain. > > > > Finally, if we are directly accessing the internal ffmpeg representation of an > > encapsulated PTS, then we are being implicitly forced to use the same sentinel > > value for no PTS that ffmpeg uses. If we are operating under the non-volatile > > PTS assumption given above, and choose to cache the PTS locally in this base > > class for inline access, then we can map from their value to our value as > needed > > at construction time. If not, then we need to be sure that the values are the > > same. If they are already, then good. If not, then we can change ours to > match > > theirs provided there are no design concerns about their choice (I would not > use > > 0 or -1 as my sentinel; either 0x8000... or 0x7FFF... seem like better > choices). > > Either way, we should have a static_assert somewhere in the > > FfmpegBackedPacketImpl code which asserts that their value matches our value > > (IIRC, they use a #def called NO_PTS) > > Looking at my ffmpeg code, it appears that demuxes are always giving us > timestamps (so far, at least). The decoders don't, but they're using the vanilla > Packet implementation. I add presentation time myself, in that case. so I went ahead and checked in the current ffmpeg source. The constant is called AV_NOPTS_VALUE. It is currently set to 0x80000.... (the most negative int64_t). We have our version defined in media_pipe.mojom as 0x7ffff... (the most positive int64_t). I still feel that if we are going to provide direct access to the value that we should make certain to have a static assert that both of these line up. FWIW - an example of a demuxer which routinely produces this value for its packets is the MPEG-2 PS/PES demuxer. Both the DTS and the PTS are initialized to NOPTS; if there is no timestamps at the PS/PES level (common for non-random access points in MPEG-2 encapsulated content), these will remain this way. libavformat/avformat.h goes into some detail about the use of the sentinel value (which fields it may be used for, what it means when it is used that way, etc...) I'm fine with leaving this as a TODO or a tracking bug for now. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/parts/file_reader.h (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/parts/file_reader.h:37: FILE* file_; On 2016/01/28 18:49:15, dalesat wrote: > On 2016/01/26 01:32:39, johngro wrote: > > I don't think that we should be using stdio FILE structure here. If we are > > going to continue to use them here, we should probably bury that fact using a > > FileReader/FileReaderImpl pattern (where FileReaderImpl is declared in the .cc > > file) > > > > In general, I think we really want to... > > > > 1) be designing our classes to use the underlying Mojo services for things > like > > files and network access. > > 2) Design all of our I/O classes with an async IO model in mind, not a > > synchronous one. > > > > There are multiple ways to fake out ffmpeg's transport interfaces if we go > this > > route. The simplest way would be to use a mojo service to get a data pipe and > > then just use the data pipe in a synchronous. There are other ways (which > > usually involve a dedicated thread) to fake out ffmpeg so it feels that it is > > accessing data synchronously when it is actually async underneath at the > > transport implementation level. > > Right...future CL? Acknowledged. Sure, please file a tracking issue or something so that it does not become forgotten. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/parts/lpcm_reformatter.cc (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/parts/lpcm_reformatter.cc:255: if (mix) { On 2016/01/28 18:49:16, dalesat wrote: > On 2016/01/26 23:47:29, johngro wrote: > > if you are going to put the mix decision in here, you should put it outside of > > the loop, not inside. The compiler may attempt to make this optimization for > > you, but this may be getting a bit complicated for it. > > > > Alternatively, if you make a private template of this method which takes a > bool > > DoMix parameter, you can call if from the public method where mix is decided > at > > runtime. Then, whether you put the if (DoMix) inside or outside of the loop > the > > compiler will kill the unused path in each of the generated copies of the > > template expansion. > > The outer loop is per-channel. The test will happen approximately twice per call > to TransformFrames, which I'm OK with. I think the best solution involves the > mix template parameter, but I don't want to invest too heavily in optimization > until we have the time to run performance tests. Acknowledged. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/parts/null_sink.cc (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/parts/null_sink.cc:24: demand_callback(Demand::kNeutral); On 2016/01/28 18:49:16, dalesat wrote: > On 2016/01/26 23:47:29, johngro wrote: > > You are making a demand callback inside the RegisterDemand call for an > > ActiveSink; is this OK? I thought that the model was that the initial demand > > got expressed when the engine called Prime on all of the participants at > > startup. > > > > Seems like the demand callback instance should be stored in the sink and > called > > once during prime. > > > > While we are on the subject, shouldn't ActiveSink have a protected > > demand_callback_ member and implement RegisterDemandCallback itself? The > method > > could be made virtual if a derived class had custom work to do during the > > registration/link-up phase, but it feels like this is really just bookkeeping, > > and that any startup actions should take place during Prime. > > Good catch re Prime. I missed that when I added Prime to sinks in general. I > accommodate calling the callback in RegisterDemandCallback, but perhaps I > shouldn't. > > As for doing this in the base class, we don't know in general when demand should > transition away from kNegative or whether it should transition to kNeutral or > kPositive. If we did know that, I'd rather implement that default behavior in > the stage and leave the model pure virtual. The models are the framework's > contracts with 'part' developers, and the behavior of model implementations > should be entirely the business of those developers. Acknowledged. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/ptr.h (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/ptr.h:39: // shared_ptr with upcast to TBase. On 2016/01/28 18:49:16, dalesat wrote: > On 2016/01/26 23:47:29, johngro wrote: > > Why do this? std::shared_ptr already has 3 static methods which handle static, > > dynamic and const casting for you. To do what you are doing with the > overloaded > > operator below, you can just do things like... > > > > std::shared_ptr<MyDerived> derived = std::make_shared<MyDerived>(); > > std::shared_ptr<MyBase> base = std::static_pointer_cast<MyBase>(derived); > > GiveBaseToSomeoneElse(base); > > > > This is permitted for both up and down casting. Clearly you need to know what > > you are doing when down casting, since we don't use RTTI and cannot use > dynamic > > cast. > > > > Also, if you are not going to give the pointer to someone else (who might > retain > > it), and you don't want to make a temp shared_ptr, you can just cast the > > underlying pointer. Obviously, you would only ever need to do this in a > > down-cast scenario > > > > void GiveBaseToSomeoneElse(const std::shared_ptr<MyBase>& base) { > > // Danger, we had better know that base is actually a MyDerived > > MyDerived* derived = static_cast<MyDerived*>(base.get()); > > derived->MethodOnlyInMyDerived(...); > > } > > I wasn't aware of static_pointer_cast. This class is really about making this > Engine method work: > > template<typename T, typename TBase> > Part Add(SharedPtr<T, TBase> t) { > DCHECK(t); > return Add(CreateStage(std::shared_ptr<TBase>(t))); > } > > I tried the following, but the compiler couldn't infer TBase. > > template<typename T, typename TBase> > Part Add(std::shared_ptr<T> t) { > DCHECK(t); > return Add(CreateStage(std::static_pointer_cast<TBase>(t))); > } > > I've added a TODO for getting rid of this, if possible. I want the compiler to > figure out TBase, because some code calls Engine::Add without knowing what TBase > is. If that's possible, I'm fine with getting rid of this class. Acknowledged. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/result.h (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/result.h:14: enum class Result { On 2016/01/28 18:49:16, dalesat wrote: > On 2016/01/26 23:47:29, johngro wrote: > > So, I did the same thing (defined a media result in .mojom) and got chewed out > > by Trung and James. Trung has a vision for an error code system that he wants > > us all to use. I had difficulty figuring out how it was going to solve my > > problems, but they made me comment all of my codes with which one of Trung's > > codes they would become someday. > > > > No action requested here, but we may want to consider defining these in .mojom > > so they can be used over mojom interfaces. When we revisit, we should > probably > > take a second look Trung's proposal and make an effort to make use of it. > > I currently aspire to make the framework free of mojo dependencies. If we give > up on that, I'm happy to use MojoResult or MediaResult. Ack I'm not certain that it is possible or desirable to make the framework completely independent of mojo. In particular, it seems (to me at least) like that Turquoise is supposed to be fundamentally a service oriented architecture with all inter-service communications built out of mojo. Because of this, all of the components in the AV architecture must, first and foremost, be defined using mojo interfaces and primitives. If we end up implementing most or all of our service implementations using this framework architecture, it seems advantageous to have it understand, at a deep level, how the edges of the service will communicate with other services (read; Mojo, message loops, mojom defined interfaces, etc...) Regardless, this is a deep design discussion; not something to do during this review. Let's have this discussion somewhere else. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/stages/stage.h (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stages/stage.h:44: using UpdateCallback = std::function<void(Stage* stage)>; On 2016/01/28 18:49:16, dalesat wrote: > On 2016/01/26 23:47:29, johngro wrote: > > public typedefs go at the top of the class. see > > https://google.github.io/styleguide/cppguide.html#Declaration_Order > > OK. Someone should tell the mojo compiler. agreed; I'll mention it to Mitch. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stages/stage.h:68: class SourceStage : public virtual Stage { On 2016/01/28 18:49:16, dalesat wrote: > On 2016/01/26 23:47:29, johngro wrote: > > so,this type of MI is pretty much banned by google3 style and *strongly* > > discouraged by the chrome style (which is based on google3). > > > > https://google.github.io/styleguide/cppguide.html#Multiple_Inheritance > > > > Is there another way that this could be decomposed to avoid the need for MI > with > > diamond inheritance? Perhaps Jeff has some ideas? > > The goal here was to make stage authoring a bit easier by providing some default > behavior that can be called out via inheritance. Probably the best way to get > rid of the MI is just to copy the implementations of these things...or use > macros. Acknowledged. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/stream_type.h (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stream_type.h:6: #define SERVICES_MEDIA_FRAMEWORK_STREAM_TYPE_H_ On 2016/01/28 18:49:17, dalesat wrote: > On 2016/01/26 23:47:30, johngro wrote: > > top level comment. > > > > This whole file feels like a custom, in-proc-centric C++ implementation of the > > media types which you spec'ed in mojom way back when. > > > > Why are we not using the mojom generated classes here? How are we going to > > manage this when we have to start to inter-operate with media services > > (potentially implemented in other languages) across mojo transport boundaries? > > > > If we choose to do anything at all about this in the short term, it should > > probably be handled with a follow-up change, but I suspect that this will > start > > to be an issue in the short-to-mid term. > > I did this for four reasons: > > 1) to avoid mojo dependencies > 2) because these classes are easier to use than the mojo ones > 3) I can implement useful code in these classes > 4) to allow internal support for things that aren't supported in mojo. > > This last item was important when I was getting the interleaved flag out of the > mojo defs. I've since removed it from here as well. If we want internal support > for e.g. kSigned16In32, reason 4 will become valid again. If none of these > reasons bear out, I will do the work to get rid of this. > > I get that there's significant maintenance cost. If this becomes a problem for > you, let me know, and I will do the work. Interop is a non-issue as the things > that get moved across mojo are the mojo-generated things. To the degree that > those are extensible, these must be as well (e.g. when we switch to using > strings instead of enums for some values). Acknowledged. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/stream_type.h:33: StreamTypes(size_t size) : std::vector<StreamTypePtr>(size) {} On 2016/01/28 18:49:17, dalesat wrote: > On 2016/01/26 23:47:30, johngro wrote: > > if we are imposing move semantics on this class, this constructor should be > > private, and the copy constructor should be removed. I think that the Mojo > > framework already has a handy macro for this, but I'm not sure. > > > > If we are not imposing move semantics on this class, then we should probably > > just say... > > > > using StreamTypes = std::vector<StreamTypePtr>; > > > > and leave it at that. This seems to apply to all of the extensions of > > std::vector below. They all seem to add New and Clone to the mix to force > > unique_ptr and move patterns, but they all expose a direct constructor as > well. > > I'm not enforcing move semantics, but I do support them. There are plenty of > cases in which classes have StreamType members that are only exposed as const > StreamType&. Acknowledged. https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... File services/media/framework/allocator.cc (right): https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/allocator.cc:49: DCHECK(buffer); On 2016/01/29 01:08:29, dalesat wrote: > On 2016/01/28 19:14:54, johngro wrote: > > Given that allocate is NOTREACHED, shouldn't this be NOTREACHED as well? > > > > Also, in order to assert that size > 0 here, we should do so as well in > > AllocatePayloadBuffer. That said, since we never give out a buffer anyway, is > > there any real reason to assert anything about the size of the failed > allocation > > which is (for some reason) being released? > > > > Finally, since the Null allocator never gives out an actual buffer (it always > > returns nullptr), this should probably be DCHECK(!buffer) or DCHECK(buffer == > > nullptr). It would be bit odd, but not unthinkable that someone is attempting > > to return the failed allocation to the Null allocator; but if they attempt to > do > > so, the pointer they are "returning" had better be nullptr. > > NullAllocator is used when the packet packet payload didn't actually come from > an allocator. No one should call AllocatePayloadBuffer and ReleasePayloadBuffer > should do nothing. In retrospect, it would be simpler if Packet was OK not > having an allocator. I'll get rid of this. Acknowledged. https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... File services/media/framework/engine.cc (right): https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/engine.cc:208: lock_.Release(); On 2016/01/29 01:08:29, dalesat wrote: > On 2016/01/28 19:14:55, johngro wrote: > > so, you made a copy of the sinks_ list which is holding raw pointers to the > > stages which are classified as sinks, then you release the lock and call prime > > on each sink. > > > > What prevents another thread from calling Remove on a stage which exists in > the > > sink collection (or from Resetting the entire Engine) and deleting the sink > > stage out from under you while you are doing this? > > At this point, we are depending on the kindness of the caller. Adding a TODO. Acknowledged. https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/engine.cc:234: // LIFO On 2016/01/28 18:49:17, dalesat wrote: > On 2016/01/27 22:35:22, johngro wrote: > > These comments still seem backwards. Both Pop implementation Pop from the > front > > of the deque. PushToSupply's comment says it is LIFO, but it is pushing to > the > > back and pulling from the front (which is FIFO), and vice-versa for > PushToDemand > > (push and pull to front is LIFO, comment says FIFO). > > > > Either the comment is wrong or the code is wrong here. Also, while it is > great > > to have the comments (FIFO vs. LIFO) they are a a little vague. They merely > > assert whether these data structures are stacks or queues, but provide no > > explanation as to why the FIFO vs. LIFO-ness of the structures are important. > > This means I can notice the mismatch, but I cannot determine who is correct > (the > > code or the comment). Please update the comment (or add one elsewhere in some > > more top-level theory-of-operation comment) which explains the reasoning > behind > > the FIFO/LIFO decisions. > > > > Finally, its pretty easy to enforce this in code so that no mistakes can be > > made. Take a look at std::stack and std::queue. They are templateized > adapters > > for std:: containers which support push/pop front/back (vector, deque, list; > > default it deque) which enforce LIFO and FIFO behavior respectively via their > > exposed API. This is a much stronger assertion than just having the comment, > > but an explanation for why to use one vs. the other somewhere in the code > would > > still be appreciated. > > Thanks for the tip on queue/stack. They don't have clear(), but are otherwise > just the ticket. > > I've added a comment as to why these are queue and stack - it basically says > that I guessed. There's also a TODO to determine what ordering is optimal and > implement that. It's possible that it doesn't matter. I also see a potential > issue with having two backlogs. One may be adequate. Acknowledged. Re: ordering, this basically boils down to a priority issue (which work is the most important?) I'm sure we will figure it out moving forward. If we want to move to a 1-backlog model, it may be appropriate to have a priority for each job and stuff them into a multiset (or set, if there is a good way to unique-ify the priorities). Time will tell.. Re: clear() missing; how odd. Perhaps its because they didn't want to significantly increase the complexity of the requirements for the underlying container? That said, C++11 supplies a constant order swap method. I'm not sure how you implement such a thing without requiring that the underlying container has a constant order swap and/or assignment operator (which they do not require). That said, the most efficient way to implement clear for one of these is probably... using MyQueue = std::queue<MyStuff, MyContainer<MyStuff>>; { MyQueue tmp; clear_this_queue_.swap(tmp); } // tmp goes out of scope, clearing the underlying container in the process. https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/engine.cc:353: stage->Prepare(update_function_); On 2016/01/29 01:08:30, dalesat wrote: > On 2016/01/28 19:14:55, johngro wrote: > > Following comment applies to UpdateUnsafe as well > > > > So, at this point you are holding your master lock, and calling into another > > function implemented by the stage (which will most likely execute logic > > implemented by the part contained within the stage). > > > > This raises a number of concerns. > > How does the stage/part call back into the engine to queue new work and not > > re-enter the lock. > > > > If the stage/part acquires an internal lock, what it to prevent an A/B, B/A > lock > > ordering issue in the interaction between the stage/part and the engine. > > IOW, There seems to be a significant danger that we engine.lock then part.lock > > during update, and something asynchronous happens inside of the part > (interrupt > > fires or something) and the part code attempts to part.lock then > > engine.do_something_which_requires_the_lock, resulting in a deadlock hazard. > > > > I'm not going to be dogmatic and say that locks should never be held when > > calling other functions, but if we are going to do so, then we need to be very > > certain of our assumptions about things like lock domains, lock ordering and > > exactly what the other code will do while holding our lock. Whenever > possible, > > we should do our best to break down the scope of where locks are held, both to > > increase parallelism and decrease the risk of deadlock. > > > > This particular pattern of holding a master engine lock while calling out into > > the modular stage/part world makes me very nervous. Is there any way that we > > can decompose this so that we do not have to hold the lock while we call into > > what amounts to external code? > > The threading model will change dramatically as time goes on. This CL is not the > place to redesign it. I've added a lengthy comment to engine.h that talks about > the current constraints. It will be fine for now. > > In general, let's keep comments to the effect of "You haven't done everything > that will ever need to be done concerning issue X in this CL" to a minimum. We > should make sure issues are captured and move ahead. Acknowledged. https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... File services/media/framework/engine.h (right): https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/engine.h:213: void PushToDemandBacklogUnsafe(Stage* stage); On 2016/01/29 01:08:30, dalesat wrote: > On 2016/01/28 19:14:55, johngro wrote: > > The unsafe methods are the methods which assume that the lock is held, yes? > If > > so, then shouldn't they all need to be private? > > These are called by inputs and outputs, which are part of the > Engine/Stage/StageInput/StageOutput system. I'll make these private and have the > input and output classes listed as friends. Acknowledged. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/engine.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/engine.h:111: // its dwmand through an input, the upstream stage is added to the backlog. s/dwmand/demand
https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... File services/media/framework/packet.h (right): https://codereview.chromium.org/1577953002/diff/40001/services/media/framewor... services/media/framework/packet.h:37: bool end_of_stream, On 2016/02/01 22:38:16, johngro wrote: > On 2016/01/28 18:49:15, dalesat wrote: > > On 2016/01/27 22:35:22, johngro wrote: > > > On 2016/01/26 21:17:51, dalesat wrote: > > > > On 2016/01/26 01:32:39, johngro wrote: > > > > > <design_point note="No code change requested> > > > > > PTS, duration and EOS are all pieces of metadata for a Packet (payload, > > > media > > > > > access unit, whatever), but do not represent the sum total of per-packet > > > > > metadata we may wish to express. Other pieces of metadata which can be > > > useful > > > > > for some (but not all) packets include things like... > > > > > > > > > > 1) Random-access-point, drop-able flags for codecs with interpacket > > > > > dependencies. > > > > > 2) Colorspaces, packing format (subsampling, plane construction, etc..) > > for > > > > raw > > > > > frames of video. > > > > > 3) Mandated downmix matricies (or references to them) for decoded audio. > > > > > 4) Language codes for subpicture packets. > > > > > > > > > > and so on. It may be good to consider decoupling the metadata from > packet > > > and > > > > > having the packet just hold onto a reference to the metadata. > > > > > </design_point> > > > > > > > > > > Stepping back from the design question, I could be convinced that the > one > > > > piece > > > > > of universal metadata which is useful to all packets is the presentation > > > > > timestamp. Even though it sometimes takes on the special value > "Unknown", > > > it > > > > is > > > > > so commonly accessed that I can understand hoisting it up to the class > > > Packet > > > > > level of things and using a sentinel value for "Unknown". In this case, > > why > > > > are > > > > > we accessing the PTS via a virtual function? Wouldn't it be better to > > make > > > > the > > > > > timestamp a private member of the base Packet class (accessible using an > > > > inline > > > > > public accessor) and require its definition by the subclass using an > > > explicit > > > > > protected constructor? > > > > > > > > Ack comments about more metadata. > > > > > > > > Are you concerned about performance issues with PTS? The entire thing is > > > > abstract, because some implementations are backed by structures defined by > > > e.g. > > > > ffmpeg (AVFrame and AVPacket are examples). > > > > > > mildly; its one of those things which feel like a coin flip now, but later > who > > > knows. If the timestamp on a packet once created is immutable (a principal > I > > > think we both agree is a worthwhile goal), then it should be simple to have > > the > > > protected constructor of Packet demand that a PTS be provided at > construction > > > time. I could be then accessed with a inline const accessor for pretty much > > no > > > cost. > > > > > > If the underlying packet is going to undergo transformations which alter the > > > PTS, and this is going to happen inside the context of something like an > > ffmpeg > > > operation on an encapsulated ffmpeg structure, then this technique will not > > work > > > without a bunch of manual labor which will be hard to maintain. > > > > > > Finally, if we are directly accessing the internal ffmpeg representation of > an > > > encapsulated PTS, then we are being implicitly forced to use the same > sentinel > > > value for no PTS that ffmpeg uses. If we are operating under the > non-volatile > > > PTS assumption given above, and choose to cache the PTS locally in this base > > > class for inline access, then we can map from their value to our value as > > needed > > > at construction time. If not, then we need to be sure that the values are > the > > > same. If they are already, then good. If not, then we can change ours to > > match > > > theirs provided there are no design concerns about their choice (I would not > > use > > > 0 or -1 as my sentinel; either 0x8000... or 0x7FFF... seem like better > > choices). > > > Either way, we should have a static_assert somewhere in the > > > FfmpegBackedPacketImpl code which asserts that their value matches our value > > > (IIRC, they use a #def called NO_PTS) > > > > Looking at my ffmpeg code, it appears that demuxes are always giving us > > timestamps (so far, at least). The decoders don't, but they're using the > vanilla > > Packet implementation. I add presentation time myself, in that case. > > so I went ahead and checked in the current ffmpeg source. The constant is > called AV_NOPTS_VALUE. It is currently set to 0x80000.... (the most negative > int64_t). We have our version defined in media_pipe.mojom as 0x7ffff... (the > most positive int64_t). I still feel that if we are going to provide direct > access to the value that we should make certain to have a static assert that > both of these line up. > > FWIW - an example of a demuxer which routinely produces this value for its > packets is the MPEG-2 PS/PES demuxer. Both the DTS and the PTS are initialized > to NOPTS; if there is no timestamps at the PS/PES level (common for non-random > access points in MPEG-2 encapsulated content), these will remain this way. > libavformat/avformat.h goes into some detail about the use of the sentinel value > (which fields it may be used for, what it means when it is used that way, > etc...) > > I'm fine with leaving this as a TODO or a tracking bug for now. Acknowledged. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/parts/file_reader.h (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/parts/file_reader.h:37: FILE* file_; On 2016/02/01 22:38:16, johngro wrote: > On 2016/01/28 18:49:15, dalesat wrote: > > On 2016/01/26 01:32:39, johngro wrote: > > > I don't think that we should be using stdio FILE structure here. If we are > > > going to continue to use them here, we should probably bury that fact using > a > > > FileReader/FileReaderImpl pattern (where FileReaderImpl is declared in the > .cc > > > file) > > > > > > In general, I think we really want to... > > > > > > 1) be designing our classes to use the underlying Mojo services for things > > like > > > files and network access. > > > 2) Design all of our I/O classes with an async IO model in mind, not a > > > synchronous one. > > > > > > There are multiple ways to fake out ffmpeg's transport interfaces if we go > > this > > > route. The simplest way would be to use a mojo service to get a data pipe > and > > > then just use the data pipe in a synchronous. There are other ways (which > > > usually involve a dedicated thread) to fake out ffmpeg so it feels that it > is > > > accessing data synchronously when it is actually async underneath at the > > > transport implementation level. > > > > Right...future CL? > > Acknowledged. > > Sure, please file a tracking issue or something so that it does not become > forgotten. Acknowledged. https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... File services/media/framework/result.h (right): https://codereview.chromium.org/1577953002/diff/60001/services/media/framewor... services/media/framework/result.h:14: enum class Result { On 2016/02/01 22:38:16, johngro wrote: > On 2016/01/28 18:49:16, dalesat wrote: > > On 2016/01/26 23:47:29, johngro wrote: > > > So, I did the same thing (defined a media result in .mojom) and got chewed > out > > > by Trung and James. Trung has a vision for an error code system that he > wants > > > us all to use. I had difficulty figuring out how it was going to solve my > > > problems, but they made me comment all of my codes with which one of Trung's > > > codes they would become someday. > > > > > > No action requested here, but we may want to consider defining these in > .mojom > > > so they can be used over mojom interfaces. When we revisit, we should > > probably > > > take a second look Trung's proposal and make an effort to make use of it. > > > > I currently aspire to make the framework free of mojo dependencies. If we give > > up on that, I'm happy to use MojoResult or MediaResult. > > Ack > > I'm not certain that it is possible or desirable to make the framework > completely independent of mojo. In particular, it seems (to me at least) like > that Turquoise is supposed to be fundamentally a service oriented architecture > with all inter-service communications built out of mojo. Because of this, all > of the components in the AV architecture must, first and foremost, be defined > using mojo interfaces and primitives. If we end up implementing most or all of > our service implementations using this framework architecture, it seems > advantageous to have it understand, at a deep level, how the edges of the > service will communicate with other services (read; Mojo, message loops, mojom > defined interfaces, etc...) > > Regardless, this is a deep design discussion; not something to do during this > review. Let's have this discussion somewhere else. Acknowledged. I'll soon be uploading a CL that has all the mojo transport stuff. https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... File services/media/framework/engine.cc (right): https://codereview.chromium.org/1577953002/diff/80001/services/media/framewor... services/media/framework/engine.cc:234: // LIFO On 2016/02/01 22:38:17, johngro wrote: > On 2016/01/28 18:49:17, dalesat wrote: > > On 2016/01/27 22:35:22, johngro wrote: > > > These comments still seem backwards. Both Pop implementation Pop from the > > front > > > of the deque. PushToSupply's comment says it is LIFO, but it is pushing to > > the > > > back and pulling from the front (which is FIFO), and vice-versa for > > PushToDemand > > > (push and pull to front is LIFO, comment says FIFO). > > > > > > Either the comment is wrong or the code is wrong here. Also, while it is > > great > > > to have the comments (FIFO vs. LIFO) they are a a little vague. They merely > > > assert whether these data structures are stacks or queues, but provide no > > > explanation as to why the FIFO vs. LIFO-ness of the structures are > important. > > > This means I can notice the mismatch, but I cannot determine who is correct > > (the > > > code or the comment). Please update the comment (or add one elsewhere in > some > > > more top-level theory-of-operation comment) which explains the reasoning > > behind > > > the FIFO/LIFO decisions. > > > > > > Finally, its pretty easy to enforce this in code so that no mistakes can be > > > made. Take a look at std::stack and std::queue. They are templateized > > adapters > > > for std:: containers which support push/pop front/back (vector, deque, list; > > > default it deque) which enforce LIFO and FIFO behavior respectively via > their > > > exposed API. This is a much stronger assertion than just having the > comment, > > > but an explanation for why to use one vs. the other somewhere in the code > > would > > > still be appreciated. > > > > Thanks for the tip on queue/stack. They don't have clear(), but are otherwise > > just the ticket. > > > > I've added a comment as to why these are queue and stack - it basically says > > that I guessed. There's also a TODO to determine what ordering is optimal and > > implement that. It's possible that it doesn't matter. I also see a potential > > issue with having two backlogs. One may be adequate. > > Acknowledged. > > Re: ordering, this basically boils down to a priority issue (which work is the > most important?) I'm sure we will figure it out moving forward. If we want to > move to a 1-backlog model, it may be appropriate to have a priority for each job > and stuff them into a multiset (or set, if there is a good way to unique-ify the > priorities). Time will tell.. > > Re: clear() missing; how odd. Perhaps its because they didn't want to > significantly increase the complexity of the requirements for the underlying > container? That said, C++11 supplies a constant order swap method. I'm not > sure how you implement such a thing without requiring that the underlying > container has a constant order swap and/or assignment operator (which they do > not require). That said, the most efficient way to implement clear for one of > these is probably... > > using MyQueue = std::queue<MyStuff, MyContainer<MyStuff>>; > { > MyQueue tmp; > clear_this_queue_.swap(tmp); > } // tmp goes out of scope, clearing the underlying container in the process. Nice trick, thanks! https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/engine.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/engine.h:111: // its dwmand through an input, the upstream stage is added to the backlog. On 2016/02/01 22:38:17, johngro wrote: > s/dwmand/demand Done.
My comments are mostly advisory. These are things you should think about in future changes as you iterate. A few common themes: 1. Don't subclass C++ standard library types just to add your own conveniences. 2. The *Ptr typedefs really obscure object ownership semantics. 3. I think there's a missing link to explain how packets and allocators and stages relate. 4. I think the framework is overreaching a bit. Some parts of this functionality such as URL to content resolution should be handled by other Mojo services. In fact, I'm somewhat concerned that the centrally managed synchronous pipeline will experience a large impedance mismatch with Mojo based components that might need to plug into various stages. 5. For security reasons, you should be assuming that media pipelines will be *heavily* compartmentalized. That is to say, you're unlikely to have code running in-process doing coding, decoding, mixing, and other transformations. At the very least, each codec is probably going to live in its own isolated sandbox. The implication here is that the pipeline must be a good fit for constructs accessed via IPC. 6. I would actually recommend that you tackle the external communication issues sooner rather than later since I expect it to have a large impact on your design. 7. Watch out for missing "explicit" keywords on one-arg constructors. I didn't look too closely at the implementation itself. However I find that the logic of the pipeline is getting somewhat obfuscated by bookkeeping which suggests that further refactoring will be needed to elucidate the essential underlying concepts. Overall there's probably too much subclassing as well as type tests (eg. is mate LPCM?) happening, which is to say that the responsibilities are perhaps a bit scattered. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... File services/media/framework/allocator.cc (right): https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/allocator.cc:17: constexpr DefaultAllocator() {} constexpr isn't appropriate here https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/allocator.cc:26: DCHECK(size > 0); Why can't the size be zero? malloc allows this, though I suppose it's silly. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/allocator.cc:36: static constexpr DefaultAllocator default_allocator; I don't think you want constexpr here since this isn't a constant expression. If you mean to say that the value won't change after initialization then put const on the end, say if it were a pointer. eg. static DefaultAllocator* const default_allocator That said, static initializers are banned in our tree. You'll need someone to call a function to set this up during startup. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... File services/media/framework/allocator.h (right): https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/allocator.h:14: class Allocator { The class name is very generic but its use is more specific. Odd that this doesn't refer to the Packet structure or contain PacketPayload in the name. It's not clear to me how this class would handle scoping. Say if packets were allocating from a pool and the pool was being destroyed. I might advise that you delete this until you have a use for the polymorphism. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/allocator.h:15: public: missing virtual destructor https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/allocator.h:22: virtual void* AllocatePayloadBuffer(uint64_t size) = 0; use size_t https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/allocator.h:25: virtual void ReleasePayloadBuffer(uint64_t size, void* buffer) = 0; void*, size_t https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... File services/media/framework/conversion_pipeline_builder.cc (right): https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/conversion_pipeline_builder.cc:24: // in_type is incompatible with out_type_set. I'm generally skeptical of baking policies like this into a framework, particularly if the code runs in-process where it can't be updated. Can't we make it the application's responsibility to build pipelines? Alternately this could be a Mojo service. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/conversion_pipeline_builder.cc:26: const LpcmStreamType& in_type, Formatting looks wrong here. Use clang-format. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/conversion_pipeline_builder.cc:78: const StreamTypeSetsPtr& out_type_sets) { It's really hard to follow this code when you have typedefs for everything. I assume this is mojo::Array<StreamTypeSet> or something like that. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/conversion_pipeline_builder.cc:81: // Array has no const iterator, hence the for(;;). array->storage().cbegin()... https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... File services/media/framework/conversion_pipeline_builder.h (right): https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/conversion_pipeline_builder.h:16: // type compatible with out_type_sets. If it succeeds, returns true, updates I feel like there's a parameter missing here to provide a resolver for the type converters, unless that's hardcoded or something. Would that state reasonably be part of the Engine's context? (Should this method live on Engine?) https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/conversion_pipeline_builder.h:21: const StreamTypeSetsPtr& out_type_sets, So this captures one-to-many demux cases but not mux cases. Does that matter? https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... File services/media/framework/engine.h (right): https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/engine.h:28: // references to parts and their inputs and outputs. Engine provides a variety Consider adding a suffix to the Part, Input, and Output types to make it clearer that they are references of some kind. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/engine.h:35: // for operation, and the PrimeSinks method tells the sinks in the graph to Should PrimeSinks be folded into Prepare? https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/engine.h:87: // upstream. StageInputs recieve media from StageOutputs in the form of packets "receive" https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/engine.h:144: // 4) Parts cannot rely on being called back on the same thread on which they It might make sense to reorganize this around a work-queue instead of invoking all callbacks synchronously. All the engine cares about is that it can drive the parts in the pipeline to satisfy demand but that could be done asynchronously. ie. Signal the part (or the stage that owns it) that something is needed rather than waiting for that work to complete. If done correctly, you should be able to have parts scheduled to run on any number of threads. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/engine.h:292: Part Connect(Part upstream_part, Part downstream_part); Is this just a convenience method? https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/engine.h:311: void RemoveAll(Part part); I'd recommend encoding some type information into the method name rather than overloading. eg. AddPart, RemovePart, DisconnectOutput, etc. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/engine.h:364: static Stage* CreateStage(std::shared_ptr<T> t); It's not clear to me why these need to be defined on Engine, especially if each type of source needs some special code of its own. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/lpcm_util.cc (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/lpcm_util.cc:152: const uint8_t* in_channel = reinterpret_cast<const uint8_t*>(in); Given that the stride is always an exact multiple of the sample size, these pointers could be expressed as T* which would be a bit simpler. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/lpcm_util.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/lpcm_util.h:16: class LpcmUtil { Avoid "Util" or "Helper" types. Perhaps call this LpcmFormat or something. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/lpcm_util.h:18: static std::unique_ptr<LpcmUtil> Create(const LpcmStreamType& stream_type); Can instances be shared? Consider GetInstance perhaps if the number of types is finite. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/lpcm_util.h:28: // Mixes samples. The description is a bit unclear. Is this summing? What's the gain factor? https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/lpcm_util.h:38: uint64_t in_byte_count, Mixing bytes and frames could be error prone. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/metadata.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/metadata.h:19: typedef UniquePtr<Metadata> MetadataPtr; These typedefs make the code much harder to follow. I recommend you never define typedefs for unique_ptr or shared_ptr backed types. The mojom *Ptr types are a special case given their ubiquity. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/metadata.h:21: class Metadata { Docs? https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/metadata.h:23: static MetadataPtr New( We usually call these methods "Create". https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/metadata.h:25: const std::string& title, Don't hardcode this stuff. It'll explode. Consider using a map. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/models/active_sink.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/models/active_sink.h:17: // Sink that consumes packets asynchronously. In general it's a little unclear what subclasses must do to implement this contract. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/models/active_sink.h:20: using DemandCallback = std::function<void(Demand demand)>; Should this be defined next to Demand? https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/models/active_sink.h:25: virtual bool must_allocate() const = 0; Should be called MustAllocate because this method is virtual so doesn't meet the special rule regarding property accessors. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/models/active_sink.h:32: virtual void RegisterDemandCallback(DemandCallback demand_callback) = 0; Can there only be one callback? If so, consider calling this SetDemandCallback. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/models/active_sink.h:41: typedef std::shared_ptr<ActiveSink> ActiveSinkPtr; These typedefs are making it very hard to understand ownership semantics at point of use. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/models/active_source.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/models/active_source.h:25: virtual bool can_accept_allocator() const = 0; Same comments here for style, etc. Likely for the other model types too. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/models/demand.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/models/demand.h:13: // Ordered such that (kNegative < kNeutral < kPositive). If you intend for these values to be ordered, assign them values. eg. -1, 0, 1. Also, please add some comments to explain them. This is a central concept in your design. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/models/lpcm_frames.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/models/lpcm_frames.h:17: class LpcmFrames { LpcmFrameBuffer? LpcmBuffer? https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/models/lpcm_frames.h:44: void reset() { These methods aren't simple accessors. Call them Reset, Set, Advance https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/models/lpcm_transform.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/models/lpcm_transform.h:29: LpcmFrames* source, const? https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/models/lpcm_transform.h:31: bool mix) = 0; Consider eliminating the bool and adding another method instead or a different type. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/models/packet_transform.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/models/packet_transform.h:30: PacketPtr* output) = 0; So is this allocating a new packet each time? https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/ostream.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/ostream.h:6: #define SERVICES_MEDIA_FRAMEWORK_OSTREAM_H_ I've started calling files like this "formatting.h" https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/ostream.h:34: inline int ostream_indent_index() { Don't need inline keyword. And in particular, this probably should go into the implementation file since it has static storage. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/ostream.h:39: template<class charT, class traits> std::basic_ostream<charT, traits>& I think this can be specialized to ostream. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/ostream.h:82: // The following overloads add newlines. likely to get confusing... https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/packet.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/packet.h:31: // 1) We probably need an extensible way to add metadata to packets. Worth asking whether the metadata is part of the packet or the stream, or whether the packet's relationship to a stream should be expressed, and whether the allocation policy for packets should be located someplace else perhaps on a stream. In general it's going to get confusing to have packets floating around with varying allocation models unless they're anchored in some way, such as by saying that Packets all come from a stream of some kind which then becomes a natural place to describe allocation / reuse policies. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/packet.h:58: virtual int64_t presentation_time() const = 0; Virtual methods can't use accessor notation. So this must be GetPresentationTime(). That said, perhaps these shouldn't be virtual in the first place... https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/packet.h:71: virtual void release() = 0; Release() https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/parts/decoder.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/parts/decoder.h:32: virtual Result Init(const StreamTypePtr& stream_type) = 0; Who will call this method? Does Create do it? It already has the stream type... https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/parts/demux.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/parts/demux.h:60: virtual MetadataPtr metadata() const = 0; same style nits here for accessors Is this data immutable? If not, then how does the state evolve? Do we need to query it by time or what? https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/parts/demux.h:63: virtual const std::vector<DemuxStream*>& streams() const = 0; Ok, I think you're overusing virtual throughout this code. Consider how your base classes can be more useful. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/parts/file_reader.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/parts/file_reader.h:13: class FileReader : public Reader { Docs? https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/parts/file_reader.h:15: static ReaderPtr New() { Create https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/parts/lpcm_reformatter.cc (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/parts/lpcm_reformatter.cc:44: result = new LpcmReformatterImpl<uint8_t, uint8_t>( Do you think these allocations could become a bottleneck? Should they be singletons? https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/parts/reader.cc (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/parts/reader.cc:22: // TODO(dalesat): More schemes. Honestly this code doesn't belong in your framework at all. Resolving content by URL is something that should be done via a service. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/parts/reader.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/parts/reader.h:21: // This model is synchronous, because that's how ffmpeg works. Is ffmpeg going to be the only client of this interface? I sort of feel like synchronous I/O is going to be actively harmful to the pipeline. So although individual codecs might expect to be able to pull content using some kind of synchronous API, ultimately the input to that codec should be driven asynchronously through a suitable adaptation layer. As you're building this, remember that everything is going to be coming from some other process over IPC. There probably won't be a local filesystem or anything. To make sure you're ready for that, I'd recommend ripping most of this out in the near future and using the network service to open up URLs and get data pipes and whatever else you need. In other words, I don't see this Reader class as embodying a truly essential concern of your pipeline. Maybe it's more of a helper for shimming synchronous input over asynchronous channels but I would hope that that is kind of rare... https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/parts/reader.h:34: virtual int Read(uint8* buffer, int bytes_to_read) = 0; Use size_t or ssize_t for buffer sizes, etc. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/parts/reader.h:37: virtual int64 GetPosition() const = 0; Don't you mean int64_t? https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/parts/reader.h:44: virtual int64 GetSize() const = 0; What if the size isn't known? https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/ptr.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/ptr.h:14: // TODO(dalesat): Remove in favor of unique_ptr and a Clone template function. +1, get rid of this thing entirely https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/ptr.h:20: UniquePtr(std::nullptr_t) : std::unique_ptr<T, Deleter>() {} All one-arg constructors should be explicit. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/ptr.h:56: operator std::shared_ptr<TBase>() const { Avoid conversion operators. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/stages/active_sink_stage.cc (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/active_sink_stage.cc:24: ActiveSinkStage::~ActiveSinkStage() {} Do you need to remove your callback from the sink? (Can't tell if that's a unique or shared pointer here.) https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/stages/active_source_stage.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/active_source_stage.h:32: bool Prepare(UpdateCallback update_callback) override; Probably want const UpdateCallback& https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/stages/lpcm_stage_input.cc (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/lpcm_stage_input.cc:116: StageInput::SupplyPacketFromOutput(std::move(packet)); Do you need to consume the result of this function? https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/lpcm_stage_input.cc:121: // The upstream output is LPCM, so the packet should be a wrapper for the Does the format matter? https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/lpcm_stage_input.cc:155: return CopyOrMixFrames(); Reading the code in this function, I get the sense that there's something about the flow of packets which should be expressed through some other means, perhaps as a stream or something. The stage seems to be doing a lot of bookkeeping and optimization which could get hairy as the number of special formats grows. Or rather, the core logic of the stage seems to be getting obscured. I was expecting this class to be about applying some function to LPCM data but it's got a bunch of other things going on. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/lpcm_stage_input.cc:213: demand_buffer_ = std::malloc(frame_count * lpcm_supply_.bytes_per_frame()); Use a unique pointer or vector? https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/stages/lpcm_stage_input.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/lpcm_stage_input.h:32: // Updates mate's demand. I have no idea what this is really doing. Strange to be passing the Engine around too. Isn't the stage strongly tied to a particular Engine? https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/stages/lpcm_stage_output.cc (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/lpcm_stage_output.cc:64: // isn't LPCM. Having stages need to know about their peers is getting a little convoluted. I think there's a missing concept here. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/lpcm_stage_output.cc:71: allocator == nullptr ? Allocator::GetDefault() : allocator; I don't see a shared_ptr so the lifetime of the allocator is unclear to me here. Does Prepare() create an allocator? Who owns it? I don't see anyone freeing it... https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/stages/stage.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/stage.h:31: virtual uint32_t input_count() const = 0; Why virtual? Seems like something the base class could deal with so that implementations don't have to duplicate all bookkeeping. Also note the style guide suggests we should call this GetInputCount(), if virtual. I wonder whether Stage should be subclassed at all or whether it should call into something else that is supply the rule for that stage. In any case, having the connectivity responsibilities mixed in with the actual processing algorithms is getting confusing. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/stages/stage_input.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/stage_input.h:55: bool must_allocate() const; Or else what? Some arbitrary allocator can be used? Is this telling the sender of a packet whether packets can be passed by reference or must be copied, or what? The more I read, the more I think this allocator thing needs to be wrapped up into a stream abstraction of some kind that can handle concerns related to forwarding or duplicating packets as needed. I get the sense that you're trying to ensure that whenever a copy is made that it's being made in-place into the optimal location but it's sort of obscured by the structure of this code. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/stage_input.h:66: virtual bool SupplyPacketFromOutput(PacketPtr packet); Should this type really be subclassed? I wonder. I mean at some level, this thing is just a means for a Stage to get stuff from another Stage so maybe these should bottom out into method calls on the Engine which is managing the pipeline and the overall dataflow. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/stage_input.h:70: virtual LpcmStageInput* get_lpcm(); nit: GetLpcmStageInput(). But something about this doesn't smell right... https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/stages/stage_output.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/stage_output.h:71: Allocator* copy_allocator_; Avoid protected fields. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/stream_type.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stream_type.h:28: class StreamTypes : public std::vector<StreamTypePtr> { Please don't subclass collections like this. It doesn't add anything and makes the code more obscure. At least, I would call this StreamTypeVector or something but even so a global function Clone(vector<StreamType>) or something would avoid this entirely. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stream_type.h:34: StreamTypes(size_t size) : std::vector<StreamTypePtr>(size) {} explicit https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stream_type.h:68: enum class Scheme { Scheme is pretty overloaded and is usually associated with URLs. This looks like the beginnings of a mime type to me. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stream_type.h:89: StreamType(Scheme scheme); explicit https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stream_type.h:97: virtual const MultiplexedStreamType* multiplexed() const; This looks wrong. It's just a disguised dynamic_cast<> as far as I can tell. Given that the set of types is fixed here, you might almost be better off using a tagged union, possibly with pointers to type-specific information. That would make the virtual functions and Clone() methods go away at least. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stream_type.h:117: inline constexpr bool contains(const T& t) const { You really don't need inline on functions anymore. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stream_type.h:132: class StreamTypeSets : public std::vector<StreamTypeSetPtr> { Don't subclass C++ standard library types like this. It just makes everything harder to follow. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stream_type.h:452: enum class VideoEncoding { I suspect this representation is going to break down rapidly. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stream_type.h:590: class VideoStreamTypeSet : public StreamTypeSet { So this is a std::vector which also has extra constraints? Aggregate, don't subclass.
https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... File services/media/framework/allocator.cc (right): https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/allocator.cc:17: constexpr DefaultAllocator() {} On 2016/02/02 05:35:45, jeffbrown wrote: > constexpr isn't appropriate here constexpr is required here to satisfy style. Later on, he has a static global instance of the default allocator class (to hand out during calls to Allocator::GetDefault()). Style's opinion on global statics of class type currently says... """ Variables of class type with static storage duration are forbidden: they cause hard-to-find bugs due to indeterminate order of construction and destruction. However, such variables are allowed if they are constexpr: they have no dynamic initialization or destruction. """ Declaring the instance of the class as constexpr requires that the class have a constexpr constructor, hence the constexpr keyword here. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/allocator.cc:36: static constexpr DefaultAllocator default_allocator; On 2016/02/02 05:35:46, jeffbrown wrote: > I don't think you want constexpr here since this isn't a constant expression. > > If you mean to say that the value won't change after initialization then put > const on the end, say if it were a pointer. > > eg. static DefaultAllocator* const default_allocator > > That said, static initializers are banned in our tree. You'll need someone to > call a function to set this up during startup. See comment above. This is mandated by style, and the reason is to prevent the need to have a ctor style initializer.
Responding to general points: >> 1. Don't subclass C++ standard library types just to add your own conveniences. Understood. >> 2. The *Ptr typedefs really obscure object ownership semantics. Agree. Will remove. >> 3. I think there's a missing link to explain how packets and allocators and stages relate. Let's discuss. Across the board, much more documentation is required. >> 4. I think the framework is overreaching a bit. Some parts of this functionality such as URL to content resolution should be handled by other Mojo services. In fact, I'm somewhat concerned that the centrally managed synchronous pipeline will experience a large impedance mismatch with Mojo based components that might need to plug into various stages. I don't think this is the case. You're only seeing part of the picture, and none of it will be visible in IPC. Also, I wouldn't characterize the framework as synchronous. It accommodates synchronous and asynchronous components while avoiding the rattletrap, overweight system that results from forcing everything into an asynchronous model. >> 5. For security reasons, you should be assuming that media pipelines will be *heavily* compartmentalized. That is to say, you're unlikely to have code running in-process doing coding, decoding, mixing, and other transformations. At the very least, each codec is probably going to live in its own isolated sandbox. The implication here is that the pipeline must be a good fit for constructs accessed via IPC. I think I've addressed this in code you haven't seen. >> 6. I would actually recommend that you tackle the external communication issues sooner rather than later since I expect it to have a large impact on your design. I would love to shift focus, but we seem to be focused on the system mixer at the moment. >> 7. Watch out for missing "explicit" keywords on one-arg constructors. Right. Still regaining my c++ legs. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... File services/media/framework/allocator.cc (right): https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/allocator.cc:17: constexpr DefaultAllocator() {} On 2016/02/02 17:21:37, johngro wrote: > On 2016/02/02 05:35:45, jeffbrown wrote: > > constexpr isn't appropriate here > > constexpr is required here to satisfy style. Later on, he has a static global > instance of the default allocator class (to hand out during calls to > Allocator::GetDefault()). Style's opinion on global statics of class type > currently says... > > """ > Variables of class type with static storage duration are forbidden: they cause > hard-to-find bugs due to indeterminate order of construction and destruction. > However, such variables are allowed if they are constexpr: they have no dynamic > initialization or destruction. > """ > > Declaring the instance of the class as constexpr requires that the class have a > constexpr constructor, hence the constexpr keyword here. Acknowledged. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/allocator.cc:26: DCHECK(size > 0); On 2016/02/02 05:35:45, jeffbrown wrote: > Why can't the size be zero? malloc allows this, though I suppose it's silly. I'd be OK with it, but I'd want to return nullptr. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/allocator.cc:36: static constexpr DefaultAllocator default_allocator; On 2016/02/02 17:21:37, johngro wrote: > On 2016/02/02 05:35:46, jeffbrown wrote: > > I don't think you want constexpr here since this isn't a constant expression. > > > > If you mean to say that the value won't change after initialization then put > > const on the end, say if it were a pointer. > > > > eg. static DefaultAllocator* const default_allocator > > > > That said, static initializers are banned in our tree. You'll need someone to > > call a function to set this up during startup. > > See comment above. This is mandated by style, and the reason is to prevent the > need to have a ctor style initializer. Acknowledged. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... File services/media/framework/allocator.h (right): https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/allocator.h:14: class Allocator { On 2016/02/02 05:35:46, jeffbrown wrote: > The class name is very generic but its use is more specific. Odd that this > doesn't refer to the Packet structure or contain PacketPayload in the name. > > It's not clear to me how this class would handle scoping. Say if packets were > allocating from a pool and the pool was being destroyed. I might advise that > you delete this until you have a use for the polymorphism. With regard to referencing Packet, my tendency would be to avoid that unless it's needed. Allocators can be used for purposes other than creating Packet payloads. The LPCM input/output stuff currently uses Packet, but there's no reason why it has to (and previous versions have not). Flushing the graph is part of orderly shutdown. All allocations are released during flush (not implemented here). I have multiple implementations of this already. For example, there's a special one for allocating from a shared buffer for mojo transport. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/allocator.h:15: public: On 2016/02/02 05:35:46, jeffbrown wrote: > missing virtual destructor Adding this blows up the constexpr stuff: ../../services/media/framework/allocator.cc:17:13: error: constexpr constructor never produces a constant expression [-Winvalid-constexpr] constexpr DefaultAllocator() {} https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/allocator.h:22: virtual void* AllocatePayloadBuffer(uint64_t size) = 0; On 2016/02/02 05:35:46, jeffbrown wrote: > use size_t John pointed this out. I have this teed up in a future CL. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/allocator.h:25: virtual void ReleasePayloadBuffer(uint64_t size, void* buffer) = 0; On 2016/02/02 05:35:46, jeffbrown wrote: > void*, size_t Acknowledged. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... File services/media/framework/conversion_pipeline_builder.cc (right): https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/conversion_pipeline_builder.cc:24: // in_type is incompatible with out_type_set. On 2016/02/02 05:35:46, jeffbrown wrote: > I'm generally skeptical of baking policies like this into a framework, > particularly if the code runs in-process where it can't be updated. Can't we > make it the application's responsibility to build pipelines? Alternately this > could be a Mojo service. From the framework perspective, there's no baking in, because anyone can do the graph building. From the mojo perspective, the interesting stuff will be happening at the IPC level. This just needs to meet our in-proc needs where extensibility is not a priority. This is a helper function, not mojo policy. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/conversion_pipeline_builder.cc:26: const LpcmStreamType& in_type, On 2016/02/02 05:35:46, jeffbrown wrote: > Formatting looks wrong here. Use clang-format. Acknowledged. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/conversion_pipeline_builder.cc:78: const StreamTypeSetsPtr& out_type_sets) { On 2016/02/02 05:35:46, jeffbrown wrote: > It's really hard to follow this code when you have typedefs for everything. I > assume this is mojo::Array<StreamTypeSet> or something like that. Acknowledged. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/conversion_pipeline_builder.cc:81: // Array has no const iterator, hence the for(;;). On 2016/02/02 05:35:46, jeffbrown wrote: > array->storage().cbegin()... Comment predates transition from MediaType to StreamType. Fixed. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... File services/media/framework/conversion_pipeline_builder.h (right): https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/conversion_pipeline_builder.h:16: // type compatible with out_type_sets. If it succeeds, returns true, updates On 2016/02/02 05:35:46, jeffbrown wrote: > I feel like there's a parameter missing here to provide a resolver for the type > converters, unless that's hardcoded or something. Would that state reasonably > be part of the Engine's context? (Should this method live on Engine?) Yes, this can be expanded to accommodate more scenarios, including ones in which the caller adds their own sauce. It would be a cool improvement on the framework, though I doubt our own needs will motivate that. More likely, we'll focus on the third-party extensibility that happens when someone adds e.g. a demux or decoder service. That pipeline building will be happening at the mojo ipc level, not here in the bowels of service implementation. I deliberately kept this separate from the engine. The engine has no sense of media type, and I think this is a good separation of concerns. With this approach, a completely different pipeline builder can be employed. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/conversion_pipeline_builder.h:21: const StreamTypeSetsPtr& out_type_sets, On 2016/02/02 05:35:46, jeffbrown wrote: > So this captures one-to-many demux cases but not mux cases. Does that matter? That's currently out of scope for this function. It wasn't my intention that this function do all the graph building. The MediaSource service, for example, uses this function to convert a stream to a requested type. It understands demuxes and doesn't need help with those. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... File services/media/framework/engine.h (right): https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/engine.h:28: // references to parts and their inputs and outputs. Engine provides a variety On 2016/02/02 05:35:46, jeffbrown wrote: > Consider adding a suffix to the Part, Input, and Output types to make it clearer > that they are references of some kind. Added a TODO. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/engine.h:35: // for operation, and the PrimeSinks method tells the sinks in the graph to On 2016/02/02 05:35:46, jeffbrown wrote: > Should PrimeSinks be folded into Prepare? That might be the right thing to do. Prepare traverses the graph from sink to source. Prime needs to be done after that's complete and only involves the sinks. Add a TODO. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/engine.h:87: // upstream. StageInputs recieve media from StageOutputs in the form of packets On 2016/02/02 05:35:46, jeffbrown wrote: > "receive" Done. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/engine.h:144: // 4) Parts cannot rely on being called back on the same thread on which they On 2016/02/02 05:35:46, jeffbrown wrote: > It might make sense to reorganize this around a work-queue instead of invoking > all callbacks synchronously. > > All the engine cares about is that it can drive the parts in the pipeline to > satisfy demand but that could be done asynchronously. ie. Signal the part (or > the stage that owns it) that something is needed rather than waiting for that > work to complete. > > If done correctly, you should be able to have parts scheduled to run on any > number of threads. Yes, as discussed below, the plan is to update the threading model. The ground rules will change as well. Note that the framework is built to accommodate asynchronous 'parts' as well as synchronous ones. In keeping with the principle of making the parts easy to implement, the engine and stages contend with the threading/locking issues on behalf of synchronous parts. Asynchronous parts will have their own locks and aren't restricted with respect to how they use threads except when they interact with their respective stages. In addition to enhancing the engine to allow it employ multiple threads, we can enhance the stages for async parts to do things such as transitioning to a better thread when callbacks happen. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/engine.h:292: Part Connect(Part upstream_part, Part downstream_part); On 2016/02/02 05:35:46, jeffbrown wrote: > Is this just a convenience method? Yes, as are quite a few of these. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/engine.h:311: void RemoveAll(Part part); On 2016/02/02 05:35:46, jeffbrown wrote: > I'd recommend encoding some type information into the method name rather than > overloading. > > eg. AddPart, RemovePart, DisconnectOutput, etc. Done. https://codereview.chromium.org/1577953002/diff/120001/services/media/framewo... services/media/framework/engine.h:364: static Stage* CreateStage(std::shared_ptr<T> t); On 2016/02/02 05:35:46, jeffbrown wrote: > It's not clear to me why these need to be defined on Engine, especially if each > type of source needs some special code of its own. Not sure what you mean by "each type of source needs some special code of its own", but, yes, these don't need to be member functions. Actually, I get "unused function" errors when I make them non-members. Am I missing some c++ magic? https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/lpcm_util.cc (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/lpcm_util.cc:152: const uint8_t* in_channel = reinterpret_cast<const uint8_t*>(in); On 2016/02/02 05:35:46, jeffbrown wrote: > Given that the stride is always an exact multiple of the sample size, these > pointers could be expressed as T* which would be a bit simpler. Done. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/metadata.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/metadata.h:19: typedef UniquePtr<Metadata> MetadataPtr; On 2016/02/02 05:35:46, jeffbrown wrote: > These typedefs make the code much harder to follow. I recommend you never > define typedefs for unique_ptr or shared_ptr backed types. > > The mojom *Ptr types are a special case given their ubiquity. OK. The guidance I've received is to the contrary. Adding a comment. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/metadata.h:21: class Metadata { On 2016/02/02 05:35:47, jeffbrown wrote: > Docs? Done. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/metadata.h:23: static MetadataPtr New( On 2016/02/02 05:35:47, jeffbrown wrote: > We usually call these methods "Create". Added TODO. I was taking a cue from mojo. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/metadata.h:25: const std::string& title, On 2016/02/02 05:35:47, jeffbrown wrote: > Don't hardcode this stuff. It'll explode. Consider using a map. Yes, this was just to get the demo done. I've added a TODO. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/models/active_sink.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/models/active_sink.h:17: // Sink that consumes packets asynchronously. On 2016/02/02 05:35:47, jeffbrown wrote: > In general it's a little unclear what subclasses must do to implement this > contract. Yes, we need to provide more guidance for source/sink/transform developers generally. I'm not prepared to write the manual at this point, given that we'll probably change our minds about a ew things. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/models/active_sink.h:20: using DemandCallback = std::function<void(Demand demand)>; On 2016/02/02 05:35:47, jeffbrown wrote: > Should this be defined next to Demand? I don't think so. Its exact signature is a function of what's needed here. It's a coincidence that we're just passing Demand and nothing else...that could easily change in the future. I don't think I would, in general, define an XCallback for a type X adjacent to the definition of X. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/models/active_sink.h:25: virtual bool must_allocate() const = 0; On 2016/02/02 05:35:47, jeffbrown wrote: > Should be called MustAllocate because this method is virtual so doesn't meet the > special rule regarding property accessors. In that case, I don't understand the rules. From Google C++ style: "Functions that are very cheap to call may instead follow the style for variable names (all lower-case, with underscores between words). The rule of thumb is that such a function should be so cheap that you normally wouldn't bother caching its return value when calling it in a loop. A canonical example is an inline method that just returns one of the class's member variables." Is the concern that it's a virtual function? https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/models/active_sink.h:32: virtual void RegisterDemandCallback(DemandCallback demand_callback) = 0; On 2016/02/02 05:35:47, jeffbrown wrote: > Can there only be one callback? If so, consider calling this SetDemandCallback. Done. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/models/active_sink.h:41: typedef std::shared_ptr<ActiveSink> ActiveSinkPtr; On 2016/02/02 05:35:47, jeffbrown wrote: > These typedefs are making it very hard to understand ownership semantics at > point of use. Yes, I've added a TODO elsewhere. I received feedback to the contrary. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/models/active_source.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/models/active_source.h:25: virtual bool can_accept_allocator() const = 0; On 2016/02/02 05:35:47, jeffbrown wrote: > Same comments here for style, etc. Likely for the other model types too. Addressed elsewhere. I'll take care of this when I understand the criteria better. I think it would have been better if the style guide picked one style or the other. My 2 cents. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/models/demand.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/models/demand.h:13: // Ordered such that (kNegative < kNeutral < kPositive). On 2016/02/02 05:35:47, jeffbrown wrote: > If you intend for these values to be ordered, assign them values. eg. -1, 0, 1. > > Also, please add some comments to explain them. This is a central concept in > your design. Done. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/models/lpcm_frames.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/models/lpcm_frames.h:17: class LpcmFrames { On 2016/02/02 05:35:47, jeffbrown wrote: > LpcmFrameBuffer? LpcmBuffer? LpcmFrameBuffer. Done. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/models/lpcm_frames.h:44: void reset() { On 2016/02/02 05:35:47, jeffbrown wrote: > These methods aren't simple accessors. Call them Reset, Set, Advance Done. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/models/lpcm_transform.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/models/lpcm_transform.h:29: LpcmFrames* source, On 2016/02/02 05:35:47, jeffbrown wrote: > const? No, because the function calls source->Advance, reflecting the consumption of the frames. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/models/lpcm_transform.h:31: bool mix) = 0; On 2016/02/02 05:35:47, jeffbrown wrote: > Consider eliminating the bool and adding another method instead or a different > type. Acknowledged. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/models/packet_transform.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/models/packet_transform.h:30: PacketPtr* output) = 0; On 2016/02/02 05:35:47, jeffbrown wrote: > So is this allocating a new packet each time? Yes, whenever the transform wants to produce a packet. My intention is to optimize Packet allocation using a lookaside. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/ostream.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/ostream.h:6: #define SERVICES_MEDIA_FRAMEWORK_OSTREAM_H_ On 2016/02/02 05:35:47, jeffbrown wrote: > I've started calling files like this "formatting.h" Done. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/ostream.h:34: inline int ostream_indent_index() { On 2016/02/02 05:35:47, jeffbrown wrote: > Don't need inline keyword. And in particular, this probably should go into the > implementation file since it has static storage. Done. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/ostream.h:39: template<class charT, class traits> std::basic_ostream<charT, traits>& On 2016/02/02 05:35:47, jeffbrown wrote: > I think this can be specialized to ostream. Done. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/ostream.h:82: // The following overloads add newlines. On 2016/02/02 05:35:47, jeffbrown wrote: > likely to get confusing... Acknowledged. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/packet.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/packet.h:31: // 1) We probably need an extensible way to add metadata to packets. On 2016/02/02 05:35:47, jeffbrown wrote: > Worth asking whether the metadata is part of the packet or the stream, or > whether the packet's relationship to a stream should be expressed, and whether > the allocation policy for packets should be located someplace else perhaps on a > stream. > > In general it's going to get confusing to have packets floating around with > varying allocation models unless they're anchored in some way, such as by saying > that Packets all come from a stream of some kind which then becomes a natural > place to describe allocation / reuse policies. Acknowledged. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/packet.h:58: virtual int64_t presentation_time() const = 0; On 2016/02/02 05:35:47, jeffbrown wrote: > Virtual methods can't use accessor notation. So this must be > GetPresentationTime(). > > That said, perhaps these shouldn't be virtual in the first place... Acknowledged. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/packet.h:71: virtual void release() = 0; On 2016/02/02 05:35:47, jeffbrown wrote: > Release() Done. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/parts/decoder.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/parts/decoder.h:32: virtual Result Init(const StreamTypePtr& stream_type) = 0; On 2016/02/02 05:35:47, jeffbrown wrote: > Who will call this method? Does Create do it? It already has the stream > type... Yes. Added a comment. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/parts/demux.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/parts/demux.h:60: virtual MetadataPtr metadata() const = 0; On 2016/02/02 05:35:48, jeffbrown wrote: > same style nits here for accessors > > Is this data immutable? If not, then how does the state evolve? Do we need to > query it by time or what? Currently, there's no support for mutability. That, and many other things, will need to be added. The pattern I would prefer here is a MetadataChanged callback. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/parts/demux.h:63: virtual const std::vector<DemuxStream*>& streams() const = 0; On 2016/02/02 05:35:48, jeffbrown wrote: > Ok, I think you're overusing virtual throughout this code. Consider how your > base classes can be more useful. Given that 'parts' such as these are always hosted by Stages, I would rather put common implementation in the stage. I think it's confusing for the part developer to have some of the commonality coming from the stage and some from the base class. That said, there may be some opportunities to do some simple, well-documented stuff in the base class. In particular, methods that return a reference to something (such as this one) are obvious candidates. I'll keep an eye out and address this in future CLs. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/parts/file_reader.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/parts/file_reader.h:13: class FileReader : public Reader { On 2016/02/02 05:35:48, jeffbrown wrote: > Docs? Done. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/parts/file_reader.h:15: static ReaderPtr New() { On 2016/02/02 05:35:48, jeffbrown wrote: > Create Done. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/parts/lpcm_reformatter.cc (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/parts/lpcm_reformatter.cc:44: result = new LpcmReformatterImpl<uint8_t, uint8_t>( On 2016/02/02 05:35:48, jeffbrown wrote: > Do you think these allocations could become a bottleneck? Should they be > singletons? This is done once per LpcmReformatter, which means at most once per graph, typically. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/parts/reader.cc (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/parts/reader.cc:22: // TODO(dalesat): More schemes. On 2016/02/02 05:35:48, jeffbrown wrote: > Honestly this code doesn't belong in your framework at all. Resolving content > by URL is something that should be done via a service. I agree that *most* of the resolution should be done via a service. Are file I/O and network I/O liable to use the same interface? If not, we'll need to make that distinction at least. The rubber has to hit the road somewhere. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/parts/reader.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/parts/reader.h:21: // This model is synchronous, because that's how ffmpeg works. On 2016/02/02 05:35:48, jeffbrown wrote: > Is ffmpeg going to be the only client of this interface? > > I sort of feel like synchronous I/O is going to be actively harmful to the > pipeline. So although individual codecs might expect to be able to pull content > using some kind of synchronous API, ultimately the input to that codec should be > driven asynchronously through a suitable adaptation layer. > > As you're building this, remember that everything is going to be coming from > some other process over IPC. There probably won't be a local filesystem or > anything. To make sure you're ready for that, I'd recommend ripping most of > this out in the near future and using the network service to open up URLs and > get data pipes and whatever else you need. > > In other words, I don't see this Reader class as embodying a truly essential > concern of your pipeline. Maybe it's more of a helper for shimming synchronous > input over asynchronous channels but I would hope that that is kind of rare... I agree. There are comments to this effect elsewhere and TODOs to resolve it. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/parts/reader.h:34: virtual int Read(uint8* buffer, int bytes_to_read) = 0; On 2016/02/02 05:35:48, jeffbrown wrote: > Use size_t or ssize_t for buffer sizes, etc. Done in another CL. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/parts/reader.h:37: virtual int64 GetPosition() const = 0; On 2016/02/02 05:35:48, jeffbrown wrote: > Don't you mean int64_t? Yes. Done. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/parts/reader.h:44: virtual int64 GetSize() const = 0; On 2016/02/02 05:35:48, jeffbrown wrote: > What if the size isn't known? Added to comment. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/ptr.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/ptr.h:14: // TODO(dalesat): Remove in favor of unique_ptr and a Clone template function. On 2016/02/02 05:35:48, jeffbrown wrote: > +1, get rid of this thing entirely Acknowledged. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/ptr.h:20: UniquePtr(std::nullptr_t) : std::unique_ptr<T, Deleter>() {} On 2016/02/02 05:35:48, jeffbrown wrote: > All one-arg constructors should be explicit. True for std::nullptr_t as well? What I have here is consistent with std::unique_ptr. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/ptr.h:56: operator std::shared_ptr<TBase>() const { On 2016/02/02 05:35:48, jeffbrown wrote: > Avoid conversion operators. This is used to solve a specific problem, namely how to engine->Add(part) and have the right stage created. I think there may be some template magic involving std::enable_if that will achieve this. The TODO above is there for this purpose. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/stages/active_sink_stage.cc (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/active_sink_stage.cc:24: ActiveSinkStage::~ActiveSinkStage() {} On 2016/02/02 05:35:48, jeffbrown wrote: > Do you need to remove your callback from the sink? (Can't tell if that's a > unique or shared pointer here.) Yes, the framework currently lacks Unprepare, which is needed for lots of things. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/stages/active_source_stage.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/active_source_stage.h:32: bool Prepare(UpdateCallback update_callback) override; On 2016/02/02 05:35:48, jeffbrown wrote: > Probably want const UpdateCallback& Will investigate. Added a TODO. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/stages/lpcm_stage_input.cc (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/lpcm_stage_input.cc:116: StageInput::SupplyPacketFromOutput(std::move(packet)); On 2016/02/02 05:35:48, jeffbrown wrote: > Do you need to consume the result of this function? No. The StageInput version of this method always returns true. We return false here if the packet doesn't satisfy our demands. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/lpcm_stage_input.cc:121: // The upstream output is LPCM, so the packet should be a wrapper for the On 2016/02/02 05:35:48, jeffbrown wrote: > Does the format matter? Yes, but it's someone else's problem. Format is out of scope where the engine is concerned. I don't currently require stages to expose their input and output types. I might consider adding this for graph building (which will be done by Graph in the future), but the engine should presume that this has been taken care of. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/lpcm_stage_input.cc:155: return CopyOrMixFrames(); On 2016/02/02 05:35:48, jeffbrown wrote: > Reading the code in this function, I get the sense that there's something about > the flow of packets which should be expressed through some other means, perhaps > as a stream or something. The stage seems to be doing a lot of bookkeeping and > optimization which could get hairy as the number of special formats grows. > > Or rather, the core logic of the stage seems to be getting obscured. I was > expecting this class to be about applying some function to LPCM data but it's > got a bunch of other things going on. First off, I don't anticipate that we'll do any more of these things (other supply/demand protocols). The idea is that LpcmStageInput/Output are compatible with StageOutput/Input but provide strong advantages when connected to LpcmStageOutput/Input. The downside is that there's added complexity to implement the compatibility. There's additional complexity, because we get some advantages when a stage doesn't care about what specific buffer is used or what size it is. I iterated a few times on this, and I'm still not thrilled with it. One of the basic issues is that these objects have to do the job regardless of which type of mate they're connected to. I might get better results if I have a Connection object that sits between the input and output. That way, we can have the StageOutput->StageInput connection object, the LpcmStageOutput->LpcmStageInput connection object, the LpcmStageOutput->StageInput connection object etc. Having four of them isn't great, but at least each would get to stick to its knitting and not be jack-of-all-trades. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/lpcm_stage_input.cc:213: demand_buffer_ = std::malloc(frame_count * lpcm_supply_.bytes_per_frame()); On 2016/02/02 05:35:48, jeffbrown wrote: > Use a unique pointer or vector? Done. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/stages/lpcm_stage_input.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/lpcm_stage_input.h:32: // Updates mate's demand. On 2016/02/02 05:35:48, jeffbrown wrote: > I have no idea what this is really doing. Strange to be passing the Engine > around too. Isn't the stage strongly tied to a particular Engine? There's only one engine for a given graph. My thinking around multiproc is that there will still be one engine (there are other ways to go). The engine is just for context - we'll need to add the upstream stage to the demand backlog. In general, I'd rather not give Stage/StageInput/StageOutput a pointer to Engine long-term given that they could make various calls at the wrong time. Persistent pointers of this type take the form of callbacks, which are appropriate specific and are prepared for an asynchronous call. I provide a pointer to Engine in particular contexts in which it's appropriate for the code in question to use that pointer. In my view, this particular code is too liberal. Engine should really have some restricted interface (e.g. UpdateContext) that only exposes the methods that are appropriate to call in this context. FYI, for a future CL, I've already factored the graph parts of Engine into their own class (Graph). Engine is just about prepare/update/flush/unprepare. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/stages/lpcm_stage_output.cc (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/lpcm_stage_output.cc:64: // isn't LPCM. On 2016/02/02 05:35:48, jeffbrown wrote: > Having stages need to know about their peers is getting a little convoluted. I > think there's a missing concept here. The way I think of this is that Engine/Stage/StageInput/StageOutput are part of a container-like thing just like list/list_node. The Connection concept, mentioned elsewhere, might help here. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/lpcm_stage_output.cc:71: allocator == nullptr ? Allocator::GetDefault() : allocator; On 2016/02/02 05:35:48, jeffbrown wrote: > I don't see a shared_ptr so the lifetime of the allocator is unclear to me here. > Does Prepare() create an allocator? Who owns it? I don't see anyone freeing > it... Allocators are generally static (the default) or integral to some part (e.g. MojoConsumer, which owns an allocator that allocates space in a shared buffer). At this point, I don't need to manage their lifetimes separately. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/stages/stage.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/stage.h:31: virtual uint32_t input_count() const = 0; On 2016/02/02 05:35:48, jeffbrown wrote: > Why virtual? Seems like something the base class could deal with so that > implementations don't have to duplicate all bookkeeping. > > Also note the style guide suggests we should call this GetInputCount(), if > virtual. > > I wonder whether Stage should be subclassed at all or whether it should call > into something else that is supply the rule for that stage. In any case, having > the connectivity responsibilities mixed in with the actual processing algorithms > is getting confusing. We should discuss. The reason I make this virtual is that many stages need 0 or 1 inputs, so forcing them all to have a vector is wasteful. I'll deal with the renaming in a future CL. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/stages/stage_input.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/stage_input.h:55: bool must_allocate() const; On 2016/02/02 05:35:49, jeffbrown wrote: > Or else what? Some arbitrary allocator can be used? Is this telling the sender > of a packet whether packets can be passed by reference or must be copied, or > what? > > The more I read, the more I think this allocator thing needs to be wrapped up > into a stream abstraction of some kind that can handle concerns related to > forwarding or duplicating packets as needed. > > I get the sense that you're trying to ensure that whenever a copy is made that > it's being made in-place into the optimal location but it's sort of obscured by > the structure of this code. We should discuss. This isn't a stream attribute, if I understand what you mean. The idea is to minimize copies. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/stage_input.h:66: virtual bool SupplyPacketFromOutput(PacketPtr packet); On 2016/02/02 05:35:48, jeffbrown wrote: > Should this type really be subclassed? I wonder. I mean at some level, this > thing is just a means for a Stage to get stuff from another Stage so maybe these > should bottom out into method calls on the Engine which is managing the pipeline > and the overall dataflow. > This is virtual so that the LPCM version can run more code when this happens. Getting stuff from stage to stage is done differently for LPCM. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/stage_input.h:70: virtual LpcmStageInput* get_lpcm(); On 2016/02/02 05:35:49, jeffbrown wrote: > nit: GetLpcmStageInput(). > > But something about this doesn't smell right... This is a downcast. I'd be happy to do it differently, if you can make a suggestion. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/stages/stage_output.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stages/stage_output.h:71: Allocator* copy_allocator_; On 2016/02/02 05:35:49, jeffbrown wrote: > Avoid protected fields. Done. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/stream_type.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stream_type.h:28: class StreamTypes : public std::vector<StreamTypePtr> { On 2016/02/02 05:35:49, jeffbrown wrote: > Please don't subclass collections like this. It doesn't add anything and makes > the code more obscure. At least, I would call this StreamTypeVector or > something but even so a global function Clone(vector<StreamType>) or something > would avoid this entirely. Added TODO. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stream_type.h:34: StreamTypes(size_t size) : std::vector<StreamTypePtr>(size) {} On 2016/02/02 05:35:49, jeffbrown wrote: > explicit Done. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stream_type.h:68: enum class Scheme { On 2016/02/02 05:35:49, jeffbrown wrote: > Scheme is pretty overloaded and is usually associated with URLs. This looks > like the beginnings of a mime type to me. I'm currently thinking in terms of switching to a major type (audio, video, etc) and an encoding (lpcm, theora, ...). https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stream_type.h:89: StreamType(Scheme scheme); On 2016/02/02 05:35:49, jeffbrown wrote: > explicit Done. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stream_type.h:97: virtual const MultiplexedStreamType* multiplexed() const; On 2016/02/02 05:35:49, jeffbrown wrote: > This looks wrong. It's just a disguised dynamic_cast<> as far as I can tell. > > Given that the set of types is fixed here, you might almost be better off using > a tagged union, possibly with pointers to type-specific information. That would > make the virtual functions and Clone() methods go away at least. Acknowledged. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stream_type.h:117: inline constexpr bool contains(const T& t) const { On 2016/02/02 05:35:49, jeffbrown wrote: > You really don't need inline on functions anymore. Done. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stream_type.h:132: class StreamTypeSets : public std::vector<StreamTypeSetPtr> { On 2016/02/02 05:35:49, jeffbrown wrote: > Don't subclass C++ standard library types like this. It just makes everything > harder to follow. Added TODO. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stream_type.h:452: enum class VideoEncoding { On 2016/02/02 05:35:49, jeffbrown wrote: > I suspect this representation is going to break down rapidly. I agree. I threw this together in attempt to get ffmpeg demux and decode to work independently. https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/stream_type.h:590: class VideoStreamTypeSet : public StreamTypeSet { On 2016/02/02 05:35:49, jeffbrown wrote: > So this is a std::vector which also has extra constraints? > > Aggregate, don't subclass. Understood, but this isn't one of those cases.
Description was changed from ========== Motown in-proc streaming framework used to implement media services. Includes the engine that hosts the various 'parts' that make up in-proc streaming graphs, as well as the parts that aren't dependendent on ffmpeg or mojo. framework_create is where ffmpeg and other technologies will be integrated. For now, the 'Create' functions simply fail. ========== to ========== Motown in-proc streaming framework used to implement media services. Includes the engine that hosts the various 'parts' that make up in-proc streaming graphs, as well as the parts that aren't dependendent on ffmpeg or mojo. framework_create is where ffmpeg and other technologies will be integrated. For now, the 'Create' functions simply fail. R=johngro@google.com Committed: https://chromium.googlesource.com/external/mojo/+/bd2a17d65fabcdb7785c67b4376... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) manually as bd2a17d65fabcdb7785c67b43769e5a52cf6f8aa (presubmit successful).
Message was sent while issue was closed.
https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... File services/media/framework/ptr.h (right): https://codereview.chromium.org/1577953002/diff/140001/services/media/framewo... services/media/framework/ptr.h:20: UniquePtr(std::nullptr_t) : std::unique_ptr<T, Deleter>() {} On 2016/02/02 21:46:40, dalesat wrote: > On 2016/02/02 05:35:48, jeffbrown wrote: > > All one-arg constructors should be explicit. > > True for std::nullptr_t as well? What I have here is consistent with > std::unique_ptr. Ahh, unique_ptr may be a special case to allow some nice syntax for smart pointers. Anyhow, you're going to delete this so it's moot.
Message was sent while issue was closed.
This doesn't compile on android: FAILED: /b/build/goma/gomacc ../../third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-g++ -MMD -MF obj/services/media/framework/framework.stream_type.o.d -DV8_DEPRECATION_WARNINGS -DCLD_VERSION=1 -DDONT_EMBED_BUILD_METADATA -DUSE_OPENSSL=1 -DUSE_OPENSSL_CERTS=1 -DNO_TCMALLOC -DDISABLE_NACL -DCHROMIUM_BUILD -D_FILE_OFFSET_BITS=64 -DANDROID -DHAVE_SYS_UIO_H -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D_FORTIFY_SOURCE=2 -D__GNU_SOURCE=1 -DNDEBUG -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -I../.. -Igen -fno-strict-aliasing -march=armv7-a -mfloat-abi=softfp -mtune=generic-armv7-a -mthumb -mthumb-interwork -fno-tree-sra -fno-caller-saves -funwind-tables -fPIC -pipe -ffunction-sections -funwind-tables -fno-short-enums -finline-limit=64 -mfpu=vfpv3-d16 -Wall -Wsign-compare -Wendif-labels -Werror -Wno-missing-field-initializers -Wno-unused-parameter -Wno-psabi -Wno-extra -Wno-ignored-qualifiers -Wno-type-limits -Wno-unused-local-typedefs -isystem../../third_party/android_tools/ndk/sources/cxx-stl/llvm-libc++/libcxx/include -isystem../../third_party/android_tools/ndk/sources/cxx-stl/llvm-libc++abi/libcxxabi/include -isystem../../third_party/android_tools/ndk/sources/android/support/include -fvisibility=hidden --sysroot=/b/build/slave/mojo/build/src/third_party/android_tools/ndk/platforms/android-16/arch-arm -fno-ident -fdata-sections -ffunction-sections -fomit-frame-pointer -Os -g2 -fno-threadsafe-statics -fvisibility-inlines-hidden -std=c++11 -Wno-narrowing -Wno-literal-suffix -Wno-error=c++0x-compat -Wno-non-virtual-dtor -Wno-sign-promo -fno-rtti -fno-exceptions -c ../../services/media/framework/stream_type.cc -o obj/services/media/framework/framework.stream_type.o ../../services/media/framework/stream_type.cc: In static member function 'static uint32_t mojo::media::LpcmStreamType::SampleSizeFromFormat(mojo::media::LpcmStreamType::SampleFormat)': ../../services/media/framework/stream_type.cc:164:1: error: control reaches end of non-void function [-Werror=return-type] } ^ where are the tryjob results? |
