Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1406393004:
Add two demos which exercise the audio server. (Closed)
DescriptionAdd two demos which exercise the audio server.
Add two simple demos which show how AudioTracks can be used to render audio.
One synthesizes and plays a sine wave at a specific frequency. The other
fetches a WAV file given a URL and plays the WAV file using an AudioTrack.
R=jeffbrown@google.com, jamesr@chromium.org
BUG=
Committed: https://chromium.googlesource.com/external/mojo/+/906af5ccdb8bfd9a1062c45f294b41c534fc6dcf
Patch Set 1 #Patch Set 2 : #
Total comments: 46
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : fix android build #
Messages
Total messages: 7 (2 generated)
Description was changed from ========== Add two demos which exercise the audio server. Add two simple demos which show how AudioTracks can be used to render audio. One synthesizes and plays a sine wave at a specific frequency. The other fetches a WAV file given a URL and plays the WAV file using an AudioTrack. R=jamesr@chromium.org, jeffbrown@google.com BUG= ========== to ========== Add two demos which exercise the audio server. Add two simple demos which show how AudioTracks can be used to render audio. One synthesizes and plays a sine wave at a specific frequency. The other fetches a WAV file given a URL and plays the WAV file using an AudioTrack. R=jamesr@chromium.org, jeffbrown@google.com BUG= ==========
johngro@google.com changed reviewers: + dalesat@chromium.org
Mostly comments on how better pipelining and asynchronous error handling strategies could make the API easier to use. There should be relatively few points where a client needs to wait for a response before it can use an interface. Ideally just one -- at creation time -- when we need to exchange buffers or tokens or something. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... File examples/audio_play_test/play_tone.cc (right): https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:32: + (4 * CHUNK_USEC); Is there anything significant about the fact the the constant 4 appears in both of these definitions? https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:33: static constexpr uint32_t FRAME_BYTES = 2; sizeof(uint16_t)? https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:65: do { This construct with early breaks is a little hard to follow. Might be clearer to refactor this block out to its own function. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:89: if (!audio_track_.WaitForIncomingResponse()) { The pattern you're looking for here is to do two things: 1. Pass a callback for a function to continue the work after the describe function completes. 2. Set a connection error handler to clean up in case the pipe gets closed. No clean way to handle timeouts which is a bit annoying. See https://github.com/domokit/mojo/issues/485 https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:100: if (!audio_track_.WaitForIncomingResponse()) { This is our second WaitForIncomingResponse. I think in practice we can avoid a lot of this by designing the API to be friendlier for pipelining. In this case there's actually no reason to wait since we can already start stuffing the pipe with data. It's not clear what errors we care about here. (Why should retrieving this interface ever fail?) Notice that trying to handle errors synchronously is very painful in Mojo. Embracing asynchronous error reporting (like connection error handlers) will make it much simpler. (Get rid of those MojoResults as much as you can!) https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:133: if (!audio_track_.WaitForIncomingResponse()) { We already have the pipe so we can start using it immediately. No need to wait for the callback response. If the request fails, then the audio service can simply close the pipe on us. So for error handling it's just a matter of attaching a connection error handler to the pipe (which we'll need anyhow). https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:149: pipe_->SetSignalCallback( Oddly enough base::Bind is cleaner for stuff like this. But if you're going to use lambdas, I would say you should use them consistently throughout the file. So this is fine. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:150: [this](MediaResult res) -> bool { Note the inherent assumption that "this" will outlive the pipe. That's true because the pipe is being stored as a member. So you're good here. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:171: if (res != MediaResult::OK) { As we discussed before, it would be more Mojo-like to just close the pipe. Then you will only need the connection error handler to handle all of these errors. Note that you *still* need a connection error handler here (but don't have one) because the pipe could be closed on you and you will have to clean up. So adopting a "shoot the pipe in the head" mentality leads to more consistent error handling logic on the client side. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:173: goto error; Not sure what the style guide has to say about gotos but I imagine it's nothing good. You could just as easily return false and have the caller invoke Cleanup() when that happens. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:180: if (res != MediaResult::OK) { Why would this ever fail? Is it DCHECK worthy? https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:186: mapped_pkt.packet()->pts = media_time_; Avoid abbreviations. Prefer "presentation_time". I know it's annoying... Also, should mapped_pkt even expose the underlying packet structure? I see we have accessors for data() and length() below so we could simply offer a set_presentation_time() method too then hide the packet pointer itself. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:189: int16_t* data = reinterpret_cast<int16_t*>(mapped_pkt.data(i)); Would it make sense to embed the data type into MappedPacket itself as a convenient? When we create the pipe adapter thingy, we already know what kind of data we're going to be transferring through these buffers. So we could parameterize the type with this knowledge. If the data is heterogeneous then the client could use uint8_t and reinterpret_cast as seen here. But frequently the data will be homogeneous. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:191: if (!data) continue; swap the above two lines https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:201: res = pipe_->SendMediaPacket(&mapped_pkt); Is this blocking behind the scenes? Kind of annoying to have to check all of these things. If the error is bad arguments (malformed packet) then we could DCHECK. If the error is that the remote end didn't accept the packet then it can just close the pipe and our connection error handler will deal with it. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... File examples/audio_play_test/play_wav.cc (right): https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_wav.cc:35: "http://www.thesoundarchive.com/starwars/swvader04.wav"; Is this copyrighted material? https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_wav.cc:73: // TODO(johngro): as mentioned before... endianness! Mentioned where? https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_wav.cc:188: Wait(payload_.get(), Isn't there a non-blocking way to do this? https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_wav.cc:217: RunLoop::current()->Quit(); This is telling me that Mojo needs a better pattern for how to terminate applications... :) https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_wav.cc:266: if ((wav_info_.channel_count != 1) && (wav_info_.channel_count != 2)) { duplicate code https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_wav.cc:295: RunLoop::current()->Quit(); FWIW, you might be better off making a single Validate function which checks these things, logs errors, and returns true / false. It'll be tidier than all of these repeated quit blocks. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_wav.cc:379: this->OnAudioConfigured(res); Why can't we just send the rate control request immediately here? We don't need to wait for the callback. Be sure to watch for connection errors on the audio_track_. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_wav.cc:393: this->OnHasRateControl(res); Likewise, no need to wait for the callback before we can start using the pipe.
https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... File examples/audio_play_test/play_tone.cc (right): https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:32: + (4 * CHUNK_USEC); On 2015/11/04 19:34:43, jeffbrown wrote: > Is there anything significant about the fact the the constant 4 appears in both > of these definitions? not really. There needs to be some distance between the high and low water mark, and there should be some distance between high water and full, particularly if we want to move in the direction of a feed-forward timing driven system for maintaining buffer levels instead of a full/empty water-mark driven system. When I was using this code to exercise the server during initial development, I just picked some numbers which were not 1 or 2 so I would get some bursty behavior when it came to waking up and re-buffering. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:33: static constexpr uint32_t FRAME_BYTES = 2; On 2015/11/04 19:34:43, jeffbrown wrote: > sizeof(uint16_t)? sure; but <pedantic> sizeof(int16_t) since this is the actual type of the sample format being used. </pedantic> More seriously, however. A better (or at least) more formal version of this would not have the frame size as a constexpr. Instead, it would use the LpcmSampleFormat enum from media_types.mojom to spec what sample types should be produced. Issues like how to generate the tone for the given sample type parameter, and what the resulting frame size would be should flow from this parameter (as well as the channel count, also assumed to be 1 in this example code). This would then allow the example to be converted from hard coded parameters to something command line driven so that different formats could be easily switched between for the purposes of exercising the mixer various different ways. This was just a quick example, however, so there is a lot of hardcodin' going on. I'll switch this to sizeof for now; depending on how much I use this particular demo for exercising the mixer, I may come back later and give it a more formal treatment. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:65: do { On 2015/11/04 19:34:43, jeffbrown wrote: > This construct with early breaks is a little hard to follow. Might be clearer > to refactor this block out to its own function. yeah... Classically, I would use gotos for this pattern, but they tend to be frowned upon which is why I messed around with the do/while(false) pattern. The do/while(false) pattern also avoids the silly issue of the compiler warning which gets generated when you do a single line initialization. <rant> Seriously... what is up with this? I have never figured it out. Two examples... int foo() { if (bar()) { goto bailout; } int my_param = baz(); foobar(my_param); return 0; bailout: return -1; } And int foo() { if (bar()) { goto bailout; } int my_param; my_param = baz(); foobar(my_param); return 0; bailout: return -1; } In the first case, the compiler will give you an error that the goto crosses the initialization of my_param. Specifically, clang (gcc/g++ will do the same, different wording) will say... "error: cannot jump from this goto statement to its label" then "note: jump bypasses variable initialization" I think this is pretty silly, because the compiler's live-ness analysis knows goddamn well that my_param is a dead variable once you reach bailout. Even so, I might be able to understand provided that there were some consistency. Enter case #2. Exactly the same as #1 in terms of the code to be produced, except that I have declared the variable, then initialized it in a separate statement. No error for this, not even a warning. Why? Because the compiler knows that the variable is dead and has no destructor, so it does not matter! Extra credit; change the bailout to return my_param. Now clang gives you an uninitialized variable error, but does not tell you the reason why (because the code was reached via the jump). </rant> Gak... With that out of my system... Breaking this into functions introduces some issues around managing local variables (not insurmountable ones, but still things which need to be dealt with), but does not really address the fundamental issue; it just minimizes the size of the do/while block. If I fail to fetch the sink descriptor, I still need to execute the cleanup steps before exiting. So either end up with something like what I had or, something more like... if (!DoCriticalTask1()) { Cleanup(); return ERROR; } ... if (!DoCriticalTaskN()) { Cleanup(); return ERROR; } return SUCCESS; The issue here is the cut-n-paste repetition of the cleanup code. In this case, I am able to wrap this all up in destructor side effects and the Cleanup function, so its not the end of the world, but its still annoying from a pattern perspective. try/catch/finally is the classic purist's approach to these pattern problems, but we don't do exceptions in our world (a decision which I support FWIW, but feel the most when I write code like this). Formally, given all of the icky calls to Wait, this should really be broken into an async pattern with callbacks which are methods in the class. In addition to making the code harder to follow as a sequence of critical steps (IMO), this can have alternate negative side effect of forcing you to carry your temporary state around as class members (although I suppose that in this case, I may be able to bind them all to lamdas, I have not really though too much about it). Finally, I can take you suggestion and put the entire do/while block into a function. This will work, but has the strange feeling of wrapping the important sequence of steps up into a a separate function just to centralize the cleanup code. In addition, if destructor side effects were not handling a lot of the cleanup (they are in this case, but not in all code confronted with this pattern), then you still have to manage cleaning up your locals/temps. TL;DR? The proper pattern here is exception handling. Were it not banned, it would be what I used here. All of the other options can work, but they all have their individual strangeness-es when it comes to no-cut-and-paste cleanup code (not all of it relevant in this instance, but in other real world cases). I'm going to go with the if (!DoImportantStep()) { Cleanup; return; } pattern and eliminate the do/while construct for now as the compromise. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:89: if (!audio_track_.WaitForIncomingResponse()) { On 2015/11/04 19:34:43, jeffbrown wrote: > The pattern you're looking for here is to do two things: > > 1. Pass a callback for a function to continue the work after the describe > function completes. > 2. Set a connection error handler to clean up in case the pipe gets closed. > > No clean way to handle timeouts which is a bit annoying. See > https://github.com/domokit/mojo/issues/485 Acknowledged. We should talk sometime about this. In addition to this, I feel that this pattern is something which can become burdensome to developers in general. While a fully async programming model can force developers to explicitly recognize points in code where blocking can happen and yield back to a central framework, it does not force them to handle errors properly from all states. I think the place where this shows up the most is patterns like this, where developers need to break things up into a sequence of states punctuated with yield points, every one of which has an edge to the next state, as well as to an error state. Basically... A->B->C->...->Z->Success where all of the letters also have an edge to Failure. Forcing developers to explicitly recognize their yield points is good in principal, but introduces other hazards and readability issues as well (not going into detail here, I'm ranting way too much already). It would be fantastic if we could provide some C/C++ specific patterns which would allow users to easily follow the model, but also program in a sequential style even when the code is actually reduced to a co-operative multitasking pattern under the hood. This (IMO) is a generally difficult problem. While I have some ideas of things which could be done, I'm not sure that they are good ideas or that they should be done. TL;DR I agree, we should talk about ways to make this nicer for people. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:100: if (!audio_track_.WaitForIncomingResponse()) { On 2015/11/04 19:34:43, jeffbrown wrote: > This is our second WaitForIncomingResponse. > > I think in practice we can avoid a lot of this by designing the API to be > friendlier for pipelining. In this case there's actually no reason to wait > since we can already start stuffing the pipe with data. > > It's not clear what errors we care about here. (Why should retrieving this > interface ever fail?) Notice that trying to handle errors synchronously is very > painful in Mojo. Embracing asynchronous error reporting (like connection error > handlers) will make it much simpler. > > (Get rid of those MojoResults as much as you can!) Ack. Re: "why should retrieving this interface every fail?". Faulty (or malicious) implementation. IMO - if you exit the scope of a service's method which has a client's interface request, and you have not bound the interface to something, that should be fatal for your connection. Right now, you end up with a dangling, half formed connection. Calls made by the client on the interface will hang forever; they will not receive a connection failure and cannot timeout because of the current nature of the framework. Currently, there is nothing I can do about this. I return the error code because there may be a case where the track has not been configured to the point where it is able to give back a functioning rate control interface (I'm not even sure if this is the case in the current implementation anymore) and returning an un-bound interface, or adding a bunch of statefullness to the rate control interface would be a bad thing as well. Even so, the error code does not mitigate against the poorly written/malicious app which returns OK in the callback, but fails to bind the interface. Really, we need what you and I have already discussed. The ability to return some form of diagnostic with a connection closed event, and then just hang up the phone. In addition, I think (at the moment at least) we should make it an automatic connection close event if a caller requests an interface and the service does not immediately bind the request to something. I'm not sure I see the advantage to allowing very-delayed-binding, and there are a number of issues which lead to nasty timeout situations which would need to be dealt with if we continue to allow this behavior. For now, I'm leaving this, but I'm adding it to my growing list of design/cleanup tasks that we need to address. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:133: if (!audio_track_.WaitForIncomingResponse()) { On 2015/11/04 19:34:43, jeffbrown wrote: > We already have the pipe so we can start using it immediately. No need to wait > for the callback response. > > If the request fails, then the audio service can simply close the pipe on us. > So for error handling it's just a matter of attaching a connection error handler > to the pipe (which we'll need anyhow). See above. I will switch to this pattern, and re-write all of the interfaces here, just as soon as I can provide diagnostic information as to why the connection was closed. I want to land this chain of CLs first. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:149: pipe_->SetSignalCallback( On 2015/11/04 19:34:43, jeffbrown wrote: > Oddly enough base::Bind is cleaner for stuff like this. But if you're going to > use lambdas, I would say you should use them consistently throughout the file. > So this is fine. Acknowledged. base::Bind has its own issues. Without going into too many of them the important one here is that if this is code which is supposed to be published as an example of how a 3rd party should use our media interfaces, there should be no dependencies on base::. James has already made this clear to me. In light of that, I'm going to remove my dependencies on base from both of these demos, and switch to using the MOJO_ versions of logging and DCHECK. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:150: [this](MediaResult res) -> bool { On 2015/11/04 19:34:43, jeffbrown wrote: > Note the inherent assumption that "this" will outlive the pipe. That's true > because the pipe is being stored as a member. So you're good here. Indeed. This is one of the other issues I have with base::Callbacks. It really encourages you to make all of your callback target objects ref-counted, then fire off all sorts of callbacks which hold strong references to the object itself. IMO, this is a lifecycle nightmare. I would much rather have a weak_ptr model which auto nerfs the callback if its target object has gone away. base:: provides this, but warns you in no uncertain terms that their weak reference implementation is not threadsafe (unlike std::weak_ptr), which just presents a whole new set of invariants which need to be demonstrated in your code. Finally, base::Bind and base::Callback provide a way to bind to the naked this pointer, but make you feel like a Bad Person (IMO) for doing so when you do it (you need to pass base::Unretained(this) instead of passing a scoped_refptr reference to this). Complexity like this is just an inherent side effect of using a highly asynchronous callback pattern with a non-garbage collected language like C++. As you note, encapsulation should get me out of this situation (although technically, cannot be demonstrated... a bad framework implementation could technically fire my callback even after the pipe has been destroyed; the proof that this cannot happen is non-obvious, at least to me) https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:171: if (res != MediaResult::OK) { On 2015/11/04 19:34:43, jeffbrown wrote: > As we discussed before, it would be more Mojo-like to just close the pipe. Then > you will only need the connection error handler to handle all of these errors. > > Note that you *still* need a connection error handler here (but don't have one) > because the pipe could be closed on you and you will have to clean up. So > adopting a "shoot the pipe in the head" mentality leads to more consistent error > handling logic on the client side. As we discussed before, I will clean all of this up with a smile on my face as soon as the framework provides a proper channel for diagnostic/debugging error propagation. A proper timeout mechanism is a nice to have as well. For now, its going on my TODO list. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:173: goto error; On 2015/11/04 19:34:43, jeffbrown wrote: > Not sure what the style guide has to say about gotos but I imagine it's nothing > good. You could just as easily return false and have the caller invoke > Cleanup() when that happens. The external style guide actually sidesteps the issue completely. It talks about goto in only one place where it says... """ Style rules should pull their weight The benefit of a style rule must be large enough to justify asking all of our engineers to remember it. The benefit is measured relative to the codebase we would get without the rule, so a rule against a very harmful practice may still have a small benefit if people are unlikely to do it anyway. This principle mostly explains the rules we don’t have, rather than the rules we do: for example, goto contravenes many of the following principles, but is already vanishingly rare, so the Style Guide doesn’t discuss it. """ IMO - they go out of their way to slam goto and dismiss it as something that "no one does", but provide no good alternatives to the pattern. See my *extensive* rant about issues surrounding the sequential init/unwind pattern given above. Also, if they think that goto is unusual, they should spend more time in low level system/kernel code where the pattern is common (and not universally incorrect, as is a common snarky response from folks who do not do a lot of this style of programming). I'm going to respond with the conclusion of that rant. The real answer to this pattern is try/catch/finally, but this pattern is banned (for other good reasons). I will resolve the issue the same way that I did before which is to duplicate the cleanup/return false handler at the end. All of the non-exception solutions suck, this one just prefers cutting-and-pasting hazards to avoidance of a useful keyword which people have an irrational fear of because of the harm it could do (but does not in this application). https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:180: if (res != MediaResult::OK) { On 2015/11/04 19:34:43, jeffbrown wrote: > Why would this ever fail? Is it DCHECK worthy? Please refer to the documentation present for this method in circular_buffer_media_pipe_adapter.h. Seriously... I provide 28 lines of documentation including an exhaustive list of the error codes which may be returned as well as the reasons why they might be returned, in a doxygen compatible format. If the change had been checked in already (this is patchset 7/7 in the chain, the code which introduced the method is 1/7 in the chain), and we had automatic documentation generation as part of the build, I would refer you to the automatically generated docs as to the possible reasons this method might fail. re: should we DCHECK? Perhaps. From a style standpoint, I am a generally fan of an orderly shutdown whenever reasonable, particularly when interacting with the behavior of someone else's code (which, from the perspective of this demo code, the adapter's utility code is). DCHECKs should generally be only for things which are truly invariants of the code sitting in front of you, and then usually when the invariant is relatively simple to show. In this case... Given the currently documented ways for this code to return an error, I do not expect this to fail, which is why I log an error and attempt a clean shutdown. This code exists outside of this project, however, and could change in the future. If future behavior introduces a new way this might legitimately fail, I defend my decision to have this code log an error and cleanly shutdown instead of shooting itself for a perceived violation of an invariant that it cannot (or should not) actually assert. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:186: mapped_pkt.packet()->pts = media_time_; On 2015/11/04 19:34:43, jeffbrown wrote: > Avoid abbreviations. Prefer "presentation_time". I know it's annoying... > > Also, should mapped_pkt even expose the underlying packet structure? I see we > have accessors for data() and length() below so we could simply offer a > set_presentation_time() method too then hide the packet pointer itself. I responded to this already, 6 patch sets ago. As I noted back then, PTS is a term which is... 1) common in this context. 1a) I can haul out multiple industry standard which use this term all over the place. 1b) I can also also point to popular media APIs which use this term as well. 2) Defined in the top level documentation for the media APIs (we have a glossary section which defines this term in the Motown docs). FWIW - so does the docs/specs that I would bring out for 1a and 1b. 3) Defined by the comments around this code. 4) Not made intrinsically more understandable by the expansion to the phrase presentation_time or even presentation_timestamp. If you don't know what a pts is, you are unlikely to know what a presentation_time is either. You should probably be starting with the top level documentation in either case if you are not familiar with the term. I can make the same argument for the abbreviation VBLANK in a video compositor, or R146/C192 in an electrical schematic. If you don't already know, you are going to need more than just an expansion of the abbreviation to be comfortable with the material. Finally, the place for suggestions (such as implementing a class/accessory pattern instead of a struct style pattern) is in the CL which introduces the structure, not 6 changes down the line where the code is being used. This is doubly true when that patch set is still out for review (which it is). If you really want to push for this, please do it in the appropriate patch set, not here. Making the comment here is akin to someone checking in code which fills out the pts field an FFMpeg structure and telling them that FFMpeg should have named that field something else. Perhaps, but this is not the place to make that argument. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:189: int16_t* data = reinterpret_cast<int16_t*>(mapped_pkt.data(i)); On 2015/11/04 19:34:43, jeffbrown wrote: > Would it make sense to embed the data type into MappedPacket itself as a > convenient? > > When we create the pipe adapter thingy, we already know what kind of data we're > going to be transferring through these buffers. So we could parameterize the > type with this knowledge. If the data is heterogeneous then the client could > use uint8_t and reinterpret_cast as seen here. But frequently the data will be > homogeneous. no, I don't think so. If it would make you feel better, I could templatize data so you have to say data<uint8_t>, or data<int16_t>, but I don't think that I see the advantage. Regardless, if we decide that this is the proper way to go, the comment belongs either as a bug/issue, or as a comment for the CL which defines the structure, not the one which uses is. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:191: if (!data) continue; On 2015/11/04 19:34:43, jeffbrown wrote: > swap the above two lines Done. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_tone.cc:201: res = pipe_->SendMediaPacket(&mapped_pkt); On 2015/11/04 19:34:43, jeffbrown wrote: > Is this blocking behind the scenes? > > Kind of annoying to have to check all of these things. If the error is bad > arguments (malformed packet) then we could DCHECK. If the error is that the > remote end didn't accept the packet then it can just close the pipe and our > connection error handler will deal with it. no; this is interacting with a client side helper class, not the actual mojo interface, and it will not block. If you want to get the result of the SendPacket operation on the underlying pipe interface itself, you can pass a callback along with the call to send. The local error code is about local state. I suppose this is another request for the framework. If I (the client) decide to close the pipe myself due to some error... 1) My connection closed callback should still fire. 2) I should be able to provide the diagnostic reason for closing the connection which is supplied to the connection closed callback I have registered. Right now... 1) The closed callback will not fire until I take my next action and discover that my pipe has been closed... either by myself, or some other piece of code which has the ability to close my connection (perhaps some library code, or some framework code). 2) There is no ability to provide any diagnostic info, regardless of which side closes the connection. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... File examples/audio_play_test/play_wav.cc (right): https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_wav.cc:35: "http://www.thesoundarchive.com/starwars/swvader04.wav"; On 2015/11/04 19:34:44, jeffbrown wrote: > Is this copyrighted material? The URL? I don't think that you can copyright a URL. The use of the content by the server which responds to this URL is not my business IMO. I suspect that it would falls under "fair use", but since I am not a lawyer, who knows? I certainly don't have any idea, nor would it be proper for either me or you to even give the impression that we have any idea. I do know that I can find this URL using many popular search engines (like Bing(tm), for example), and that I can access this content using many popular "Web Browsers" (like Apple(tm) Safari(tm), for example) and I do not think that anyone doing so is violating any copyrights. Like I said before, however, I am not a lawyer and clearly unqualified to have any opinion about anything, in particular anything involving legal matters. I will remove the string from this CL. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_wav.cc:73: // TODO(johngro): as mentioned before... endianness! On 2015/11/04 19:34:44, jeffbrown wrote: > Mentioned where? line 55. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_wav.cc:188: Wait(payload_.get(), On 2015/11/04 19:34:44, jeffbrown wrote: > Isn't there a non-blocking way to do this? yes; if I understand correctly it involves creating an instance of a mojo::AsyncWaiter and passing it the handle, the wait conditions, and a callback. Whenever the handle meets the signalling conditions, it will invoke the callback on the main message thread. From what I can tell, There is no way to alter the waiter after construction (no way to change the conditions, no way to mask the callback), so if the handle meets the conditions, but you want to temporarily halt the callbacks, you need to destroy your waiter and allocate a new one the next time you want to be called back. Basically, in this application, the waiter needs to be.. 1) A member of the class (it cannot be bound via closure to the callback because the callback needs to be bound and passed to the waiter at construction time). 2) Heap allocated/destroyed every time you want to start/stop waiting. This is lazy example code. If you want, I will formally restructure this so that there are absolutely no blocking operations. If I do, I will go ahead and eliminate all of the calls to wait on interfaces during setup as well. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_wav.cc:217: RunLoop::current()->Quit(); On 2015/11/04 19:34:44, jeffbrown wrote: > This is telling me that Mojo needs a better pattern for how to terminate > applications... :) Acknowledged. There is some discussion on the subject here... https://docs.google.com/document/d/1tKZw3cnnGEuoOD1gYLmPQtqYHNR0swLQLLhm7fV7j... I don't think that anyone has said anything about this since early July. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_wav.cc:266: if ((wav_info_.channel_count != 1) && (wav_info_.channel_count != 2)) { On 2015/11/04 19:34:44, jeffbrown wrote: > duplicate code Done. good catch. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_wav.cc:295: RunLoop::current()->Quit(); On 2015/11/04 19:34:44, jeffbrown wrote: > FWIW, you might be better off making a single Validate function which checks > these things, logs errors, and returns true / false. It'll be tidier than all > of these repeated quit blocks. Agreed, Done. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_wav.cc:379: this->OnAudioConfigured(res); On 2015/11/04 19:34:44, jeffbrown wrote: > Why can't we just send the rate control request immediately here? We don't need > to wait for the callback. > > Be sure to watch for connection errors on the audio_track_. will change, I'm going to finish responses to initial feedback and push a new CL before coming back and restructuring this. Watching for connections errors is something which should be done no matter what, but since I do not close the pipe from the other end (and will not do until there is a good way to propagate diagnostics), its not something which should ever fire. https://codereview.chromium.org/1406393004/diff/20001/examples/audio_play_tes... examples/audio_play_test/play_wav.cc:393: this->OnHasRateControl(res); On 2015/11/04 19:34:44, jeffbrown wrote: > Likewise, no need to wait for the callback before we can start using the pipe. Acknowledged.
lgtm
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 906af5ccdb8bfd9a1062c45f294b41c534fc6dcf (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
