Motown: Add examples/media_test, a command line media player app.

Motown: Add examples/media_test, a command line media player app.

R=johngro@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/6d10d6bbfecec30019b2fe5c06e28a809f0d9753

[dalesat, 2016-03-16 23:00:48]

dalesat@chromium.org changed reviewers:
+ jeffbrown@google.com, johngro@google.com

[dalesat, 2016-03-16 23:01:02]

Please take a look. Thanks!

[johngro, 2016-03-21 22:10:47]

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/keystro...
File examples/media_test/keystroke.cc (right):

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/keystro...
examples/media_test/keystroke.cc:16: bool upped_already = false;
This should either be hidden as private static members of a class, or made
static at the file scope level so that they do not leak out into the program's
namespace.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/keystro...
examples/media_test/keystroke.cc:21: fcntl(0, F_SETFL, fcntl(0, F_GETFL) |
O_NONBLOCK);
you can say STDIN_FILENO instead of 0 here.  You will need to include unistd.h

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/keystro...
examples/media_test/keystroke.cc:24: if (!upped_already) {
so, this is terminal specific and is probably not going to work everywhere...

As a slightly more portable solution, I have some code I can give you which you
can use to put stdin into non-buffered, non-echoing, non-blocking mode.  Then
you can just call getchar.  The nasty part, however, is that you need to be sure
to hook various signals (ctrl-c for example) and put stuff back the way it was. 
Otherwise, you will need to reset your terminal to get character echoing back if
your program terminates uncleanly.

A better longer term TODO might be to use Mozart to get your keyboard input.  I
don't think that this is an option as of today, sadly, because Mozart assumes
that you are using base:: message loop type things, and will not play well with
code which expects you to be using the mojo:: message loop stuff.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/keystro...
examples/media_test/keystroke.cc:25: std::cerr << kUp << std::flush;
Why cerr and not cout?

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
File examples/media_test/media_test.cc (right):

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test.cc:61: mojo::media::LinearTransform transform(
transform_ does not appear to be getting used for much of anything else at the
moment.  As a minor optimization, consider making transform_ a LinearTransform
which gets reset during HandleStatusUpdate.  This will remove the need to reduce
the rate fraction every time position_ns gets called.

Also, if the LinearTransform instance gets initialized to 0 + 0/1 + 0, you don't
need to special case not having received a HandleStatusUpdate callback yet.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test.cc:73: return position;
if you have an idea of what the asset's duration is, you should cap the reported
position to be <= the asset duration.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
File examples/media_test/media_test.h (right):

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test.h:14: namespace examples {
if you want to save the need to say mojo::media:: all over the place, you could
put this project into ::mojo::media::examples instead of just ::examples.

If you prefer the explicit nature of calling out the media subsystem as you use
it, you could also put this in ::mojo::examples (meaning you should be able to
say media::MyThing instead of mojo::media::MyThing)

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test.h:54: uint64_t version = 0,
sanity check, 0 is a special value for version which will never be reported in a
status update by a mojo::media::MedaiPlayer, correct?

If so, it might be good to define this in the .mojom interface and then use the
constant's symbol instead of using 0.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
File examples/media_test/media_test_app.cc (right):

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test_app.cc:79: const double ns_per_second =
1000000000.0;
static constexpr for these.  Don't forget to declare storage 

Same comment as was made in the key input helper function, this is going to be
brittle from a portability perspective.  If this was the path we were going to
pursue moving forward, I would ask that these magic values get wrapped up as
static constexprs in some form of input class.

That said, I think the way forward is to integrate with Mozart, so re-organizing
this is probably moot.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test_app.cc:87: for (std::string arg : args) {
probably should be const std::string& or const auto&, but see below.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test_app.cc:97: input_file_names_.pop_front();
instead of doing this, why not just

for (size_t i = 1; i < args.size(); ++i) {
   const std::string& arg = args[i]
   // do good things
}

This also eliminates the need for the args.size() check above, and ensures that
input_file_names_iter_ (below) will always be properly initialized.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test_app.cc:99: input_file_names_iter_ =
input_file_names_.begin();
see above; this variable will not be initialized by this method if args.size()
is <= 1.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test_app.cc:104: void CreateNewMediaTest() {
DCHECK(input_file_names_iter_ != input_file_names.end());

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test_app.cc:108:
media_test_->RegisterUpdateCallback([this]() {
I really think that this lambda belongs in its own method (which the lambda
simply thunks to).

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test_app.cc:112: play_next_ = true;
consider posting a message to yourself in order to advance the track instead of
relying on Poll.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test_app.cc:123: << kUp << kUp << kUp<< kUp << kUp <<
kUp << kUp << kUp << kUp;
This is a bit nasty.  While I understand the lack of Mozart, and the example-ish
nature of this code, it would still be good to put a comment here explaining
what is going on (and why there are 9 kUps delivered, not 8, 10 or some other
number)

Also, ' ' after the 3rd kUp

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test_app.cc:171: return "FAULT";
perhaps put this case at the bottom, grouped with a default: for extra
insurance.

That, or everyone who calls state_string() needs to be on their guard for
possibly getting nullptr

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test_app.cc:265: }
right now, Poll seems to be a necessary evil in order to make the keyboard go,
but tasks like moving to the next track and could be handled just by posting
messages (and avoid the 0-100mSec processing delay).

Given how difficult it is to take back a message after it has been queued (even
if it immediately becomes no longer relevant because of, say, they very next
keystroke waiting in the input queue) it may be the right idea to keep the
Poll/advance-state-machine style expressed here.  Still, actions like track
advance and quit could just post Poll callback.

Someday, when the switch to Mozart can be made, all of the Poll-ing can be
eliminated.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test_app.cc:267: if (quit_) {
this quit_ check should probably be first.  We should not be HandleKeystroke-ing
when it is time to quit.

[dalesat, 2016-03-21 22:22:52]

dalesat@chromium.org changed reviewers:
+ kulakowski@chromium.org

[dalesat, 2016-03-21 23:58:35]

PTAL

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/keystro...
File examples/media_test/keystroke.cc (right):

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/keystro...
examples/media_test/keystroke.cc:16: bool upped_already = false;
On 2016/03/21 22:10:46, johngro wrote:
> This should either be hidden as private static members of a class, or made
> static at the file scope level so that they do not leak out into the program's
> namespace.

Done.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/keystro...
examples/media_test/keystroke.cc:21: fcntl(0, F_SETFL, fcntl(0, F_GETFL) |
O_NONBLOCK);
On 2016/03/21 22:10:46, johngro wrote:
> you can say STDIN_FILENO instead of 0 here.  You will need to include unistd.h

Done.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/keystro...
examples/media_test/keystroke.cc:24: if (!upped_already) {
On 2016/03/21 22:10:46, johngro wrote:
> so, this is terminal specific and is probably not going to work everywhere...
> 
> As a slightly more portable solution, I have some code I can give you which
you
> can use to put stdin into non-buffered, non-echoing, non-blocking mode.  Then
> you can just call getchar.  The nasty part, however, is that you need to be
sure
> to hook various signals (ctrl-c for example) and put stuff back the way it
was. 
> Otherwise, you will need to reset your terminal to get character echoing back
if
> your program terminates uncleanly.
> 
> A better longer term TODO might be to use Mozart to get your keyboard input. 
I
> don't think that this is an option as of today, sadly, because Mozart assumes
> that you are using base:: message loop type things, and will not play well
with
> code which expects you to be using the mojo:: message loop stuff.

Acknowledged.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/keystro...
examples/media_test/keystroke.cc:25: std::cerr << kUp << std::flush;
On 2016/03/21 22:10:46, johngro wrote:
> Why cerr and not cout?

Force of habit. Fixed.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
File examples/media_test/media_test.cc (right):

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test.cc:61: mojo::media::LinearTransform transform(
On 2016/03/21 22:10:46, johngro wrote:
> transform_ does not appear to be getting used for much of anything else at the
> moment.  As a minor optimization, consider making transform_ a LinearTransform
> which gets reset during HandleStatusUpdate.  This will remove the need to
reduce
> the rate fraction every time position_ns gets called.
> 
> Also, if the LinearTransform instance gets initialized to 0 + 0/1 + 0, you
don't
> need to special case not having received a HandleStatusUpdate callback yet.

Done.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test.cc:73: return position;
On 2016/03/21 22:10:46, johngro wrote:
> if you have an idea of what the asset's duration is, you should cap the
reported
> position to be <= the asset duration.

Done.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
File examples/media_test/media_test.h (right):

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test.h:14: namespace examples {
On 2016/03/21 22:10:47, johngro wrote:
> if you want to save the need to say mojo::media:: all over the place, you
could
> put this project into ::mojo::media::examples instead of just ::examples.
> 
> If you prefer the explicit nature of calling out the media subsystem as you
use
> it, you could also put this in ::mojo::examples (meaning you should be able to
> say media::MyThing instead of mojo::media::MyThing)

Done.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test.h:54: uint64_t version = 0,
On 2016/03/21 22:10:46, johngro wrote:
> sanity check, 0 is a special value for version which will never be reported in
a
> status update by a mojo::media::MedaiPlayer, correct?
> 
> If so, it might be good to define this in the .mojom interface and then use
the
> constant's symbol instead of using 0.

Excellent point. I've added this to three mojom files.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
File examples/media_test/media_test_app.cc (right):

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test_app.cc:79: const double ns_per_second =
1000000000.0;
On 2016/03/21 22:10:47, johngro wrote:
> static constexpr for these.  Don't forget to declare storage 
> 
> Same comment as was made in the key input helper function, this is going to be
> brittle from a portability perspective.  If this was the path we were going to
> pursue moving forward, I would ask that these magic values get wrapped up as
> static constexprs in some form of input class.
> 
> That said, I think the way forward is to integrate with Mozart, so
re-organizing
> this is probably moot.

Done.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test_app.cc:87: for (std::string arg : args) {
On 2016/03/21 22:10:47, johngro wrote:
> probably should be const std::string& or const auto&, but see below.

Done.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test_app.cc:97: input_file_names_.pop_front();
On 2016/03/21 22:10:47, johngro wrote:
> instead of doing this, why not just
> 
> for (size_t i = 1; i < args.size(); ++i) {
>    const std::string& arg = args[i]
>    // do good things
> }
> 
> This also eliminates the need for the args.size() check above, and ensures
that
> input_file_names_iter_ (below) will always be properly initialized.

Done.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test_app.cc:99: input_file_names_iter_ =
input_file_names_.begin();
On 2016/03/21 22:10:47, johngro wrote:
> see above; this variable will not be initialized by this method if args.size()
> is <= 1.

Acknowledged.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test_app.cc:104: void CreateNewMediaTest() {
On 2016/03/21 22:10:47, johngro wrote:
> DCHECK(input_file_names_iter_ != input_file_names.end());

Done.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test_app.cc:108:
media_test_->RegisterUpdateCallback([this]() {
On 2016/03/21 22:10:47, johngro wrote:
> I really think that this lambda belongs in its own method (which the lambda
> simply thunks to).

Done.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test_app.cc:112: play_next_ = true;
On 2016/03/21 22:10:47, johngro wrote:
> consider posting a message to yourself in order to advance the track instead
of
> relying on Poll.

Done.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test_app.cc:123: << kUp << kUp << kUp<< kUp << kUp <<
kUp << kUp << kUp << kUp;
On 2016/03/21 22:10:47, johngro wrote:
> This is a bit nasty.  While I understand the lack of Mozart, and the
example-ish
> nature of this code, it would still be good to put a comment here explaining
> what is going on (and why there are 9 kUps delivered, not 8, 10 or some other
> number)
> 
> Also, ' ' after the 3rd kUp

Done.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test_app.cc:171: return "FAULT";
On 2016/03/21 22:10:47, johngro wrote:
> perhaps put this case at the bottom, grouped with a default: for extra
> insurance.
> 
> That, or everyone who calls state_string() needs to be on their guard for
> possibly getting nullptr

Never returning nullptr is a good idea, thanks.

I like that there's no default. The build fails if I add a state and don't have
a case for it.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test_app.cc:265: }
On 2016/03/21 22:10:47, johngro wrote:
> right now, Poll seems to be a necessary evil in order to make the keyboard go,
> but tasks like moving to the next track and could be handled just by posting
> messages (and avoid the 0-100mSec processing delay).
> 
> Given how difficult it is to take back a message after it has been queued
(even
> if it immediately becomes no longer relevant because of, say, they very next
> keystroke waiting in the input queue) it may be the right idea to keep the
> Poll/advance-state-machine style expressed here.  Still, actions like track
> advance and quit could just post Poll callback.
> 
> Someday, when the switch to Mozart can be made, all of the Poll-ing can be
> eliminated.

Agreed. I switched to posting a task for play_next_ as you recommended. Some of
the stuff handled here should ideally be handled in other ways, but the position
update really requires polling. Because we're using a transform for the position
rather than some callback, we need to poll at the resolution of the position
display (100ms in this case).

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test_app.cc:267: if (quit_) {
On 2016/03/21 22:10:47, johngro wrote:
> this quit_ check should probably be first.  We should not be
HandleKeystroke-ing
> when it is time to quit.

HandleKeystroke sets quit_, so this is the best we can do in terms of getting
out after the 'q' is detected. quit_ is false at the top of this method (added a
DCHECK), and Poll will never run again after this block runs.

[johngro, 2016-03-22 16:56:35]

one small comment about the initial value of your Local->Media time
transformation; lgtm after that.

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
File examples/media_test/media_test_app.cc (right):

https://codereview.chromium.org/1809703003/diff/1/examples/media_test/media_t...
examples/media_test/media_test_app.cc:171: return "FAULT";
On 2016/03/21 23:58:35, dalesat wrote:
> On 2016/03/21 22:10:47, johngro wrote:
> > perhaps put this case at the bottom, grouped with a default: for extra
> > insurance.
> > 
> > That, or everyone who calls state_string() needs to be on their guard for
> > possibly getting nullptr
> 
> Never returning nullptr is a good idea, thanks.
> 
> I like that there's no default. The build fails if I add a state and don't
have
> a case for it.

Acknowledged.

https://codereview.chromium.org/1809703003/diff/20001/examples/media_test/med...
File examples/media_test/media_test.h (right):

https://codereview.chromium.org/1809703003/diff/20001/examples/media_test/med...
examples/media_test/media_test.h:62: LinearTransform transform_ =
LinearTransform(0, 1, 1, 0);
Assuming that local->media time is the forward transform here, this should be
initialized to (0, 0, 1, 0).  This way, no matter what local time reads, media
time will be 0 until the system is buffered and running.

Right now, this will end up reporting local time as media time until the system
receives its first update from the player.

[dalesat, 2016-03-22 18:26:21]

Description was changed from

==========
Motown: Add examples/media_test, a command line media player app.
==========

to

==========
Motown: Add examples/media_test, a command line media player app.

R=johngro@google.com

Committed:
https://chromium.googlesource.com/external/mojo/+/6d10d6bbfecec30019b2fe5c06e...
==========

[dalesat, 2016-03-22 18:26:22]

Committed patchset #2 (id:20001) manually as
6d10d6bbfecec30019b2fe5c06e28a809f0d9753 (presubmit successful).

[dalesat, 2016-03-22 19:59:39]

https://codereview.chromium.org/1809703003/diff/20001/examples/media_test/med...
File examples/media_test/media_test.h (right):

https://codereview.chromium.org/1809703003/diff/20001/examples/media_test/med...
examples/media_test/media_test.h:62: LinearTransform transform_ =
LinearTransform(0, 1, 1, 0);
On 2016/03/22 16:56:35, johngro wrote:
> Assuming that local->media time is the forward transform here, this should be
> initialized to (0, 0, 1, 0).  This way, no matter what local time reads, media
> time will be 0 until the system is buffered and running.
> 
> Right now, this will end up reporting local time as media time until the
system
> receives its first update from the player.

Done.