Handles MOJO_HANDLE_SIGNAL_PEER_CLOSED in MojoDemuxerStreamAdapter.

Handles MOJO_HANDLE_SIGNAL_PEER_CLOSED in MojoDemuxerStreamAdapter.

BUG=621141
Committed: https://crrev.com/57a3c25712bcf3135453838b575126b0e94be7d3
Cr-Commit-Position: refs/heads/master@{#401062}

[alokp, 2016-06-20 23:52:13]

alokp@chromium.org changed reviewers:
+ rockot@chromium.org, xhwang@chromium.org

[alokp, 2016-06-20 23:52:14]

https://codereview.chromium.org/2088633002/diff/1/media/mojo/services/mojo_de...
File media/mojo/services/mojo_demuxer_stream_adapter.cc (left):

https://codereview.chromium.org/2088633002/diff/1/media/mojo/services/mojo_de...
media/mojo/services/mojo_demuxer_stream_adapter.cc:123:
CHECK_EQ(MOJO_HANDLE_SIGNAL_READABLE, state.satisfied_signals);
I started hitting this CHECK after https://codereview.chromium.org/2075193002/

https://codereview.chromium.org/2088633002/diff/1/media/mojo/services/mojo_de...
File media/mojo/services/mojo_demuxer_stream_adapter.cc (right):

https://codereview.chromium.org/2088633002/diff/1/media/mojo/services/mojo_de...
media/mojo/services/mojo_demuxer_stream_adapter.cc:141:
base::ResetAndReturn(&read_cb_).Run(DemuxerStream::kAborted, nullptr);
Should I reset stream_pipe_ here and early return on the next
MojoDemuxerStreamAdapter::Read?

Or should MojoDemuxerStreamAdapter monitor |demuxer_stream_| connection error
and bubble the error to MojoRendererService to handle?

[Ken Rockot(use gerrit already), 2016-06-21 00:15:50]

https://codereview.chromium.org/2088633002/diff/1/media/mojo/services/mojo_de...
File media/mojo/services/mojo_demuxer_stream_adapter.cc (right):

https://codereview.chromium.org/2088633002/diff/1/media/mojo/services/mojo_de...
media/mojo/services/mojo_demuxer_stream_adapter.cc:124: CHECK_EQ(MOJO_RESULT_OK,
MojoWait(stream_pipe_.get().value(),
I don't think a CHECK was ever appropriate here really, and I don't think you
should bother waiting for PEER_CLOSED specifically. Here's what you probably
want:

MojoResult result = MojoWait(stream_pipe_.get().value(),
                             MOJO_HANDLE_SIGNAL_READABLE,
                             MOJO_DEADLINE_INDEFINITE, nullptr);
if (result != MOJO_RESULT_OK) {
  // The pipe has been closed or some other unrecoverable error has
  // occurred. Shut down gracefully.
  return;
}

// There's definitely data to read, continue normally.


If you really wanted to do a sanity check could add inside the if block

  CHECK_EQ(result, MOJO_RESULT_FAILED_PRECONDITION);

to verify that Wait failed because the pipe is permanently unreadable (i.e. peer
closed). This is realistically the only other result code you can ever see when
waiting for READABLE here. If you do it this way, you don't need to bother
capturing MojoHandleSignalsState and testing satisfied_signals yourself: the
return value is meaningful enough.

https://codereview.chromium.org/2088633002/diff/1/media/mojo/services/mojo_de...
media/mojo/services/mojo_demuxer_stream_adapter.cc:141:
base::ResetAndReturn(&read_cb_).Run(DemuxerStream::kAborted, nullptr);
On 2016/06/20 at 23:52:14, alokp wrote:
> Should I reset stream_pipe_ here and early return on the next
MojoDemuxerStreamAdapter::Read?
> 
> Or should MojoDemuxerStreamAdapter monitor |demuxer_stream_| connection error
and bubble the error to MojoRendererService to handle?

I'll let xhwang comment on this. I don't understand this code well enough to
have an opinion

[xhwang, 2016-06-21 05:15:21]

https://chromiumcodereview.appspot.com/2088633002/diff/1/media/mojo/services/...
File media/mojo/services/mojo_demuxer_stream_adapter.cc (right):

https://chromiumcodereview.appspot.com/2088633002/diff/1/media/mojo/services/...
media/mojo/services/mojo_demuxer_stream_adapter.cc:122: // Wait for the data to
become available in the DataPipe.
timav@ has fixed this on MojoAudioDecoder recently, can you check what's being
done there and do something similar?

https://cs.chromium.org/chromium/src/media/mojo/services/mojo_audio_decoder_s...

FYI, I think we have too many duplicate code for this same logic. I filed
crbug.com/621784 to unify them.

https://chromiumcodereview.appspot.com/2088633002/diff/1/media/mojo/services/...
media/mojo/services/mojo_demuxer_stream_adapter.cc:141:
base::ResetAndReturn(&read_cb_).Run(DemuxerStream::kAborted, nullptr);
On 2016/06/21 00:15:50, Ken Rockot wrote:
> On 2016/06/20 at 23:52:14, alokp wrote:
> > Should I reset stream_pipe_ here and early return on the next
> MojoDemuxerStreamAdapter::Read?
> > 
> > Or should MojoDemuxerStreamAdapter monitor |demuxer_stream_| connection
error
> and bubble the error to MojoRendererService to handle?
> 
> I'll let xhwang comment on this. I don't understand this code well enough to
> have an opinion

The Renderer hosted by MojoRendererService will get the kAborted and handle that
as needed. So I don't feel we should have another channel to signal the failure.
Historically kAborted has been used for both abort during reset/teardown/seek,
and failures. Maybe we should add a kError state to make this more clear.

[alokp, 2016-06-21 17:09:32]

https://chromiumcodereview.appspot.com/2088633002/diff/1/media/mojo/services/...
File media/mojo/services/mojo_demuxer_stream_adapter.cc (right):

https://chromiumcodereview.appspot.com/2088633002/diff/1/media/mojo/services/...
media/mojo/services/mojo_demuxer_stream_adapter.cc:122: // Wait for the data to
become available in the DataPipe.
On 2016/06/21 05:15:21, xhwang wrote:
> timav@ has fixed this on MojoAudioDecoder recently, can you check what's being
> done there and do something similar?
> 
>
https://cs.chromium.org/chromium/src/media/mojo/services/mojo_audio_decoder_s...
> 
> FYI, I think we have too many duplicate code for this same logic. I filed
> crbug.com/621784 to unify them.

OK. I copied the implementation from MojoAudioDecoder. Thanks for
crbug.com/621784 to avoid duplicate code.

https://chromiumcodereview.appspot.com/2088633002/diff/1/media/mojo/services/...
media/mojo/services/mojo_demuxer_stream_adapter.cc:124: CHECK_EQ(MOJO_RESULT_OK,
MojoWait(stream_pipe_.get().value(),
On 2016/06/21 00:15:50, Ken Rockot wrote:
> I don't think a CHECK was ever appropriate here really, and I don't think you
> should bother waiting for PEER_CLOSED specifically. Here's what you probably
> want:
> 
> MojoResult result = MojoWait(stream_pipe_.get().value(),
>                              MOJO_HANDLE_SIGNAL_READABLE,
>                              MOJO_DEADLINE_INDEFINITE, nullptr);
> if (result != MOJO_RESULT_OK) {
>   // The pipe has been closed or some other unrecoverable error has
>   // occurred. Shut down gracefully.
>   return;
> }
> 
> // There's definitely data to read, continue normally.
> 
> 
> If you really wanted to do a sanity check could add inside the if block
> 
>   CHECK_EQ(result, MOJO_RESULT_FAILED_PRECONDITION);
> 
> to verify that Wait failed because the pipe is permanently unreadable (i.e.
peer
> closed). This is realistically the only other result code you can ever see
when
> waiting for READABLE here. If you do it this way, you don't need to bother
> capturing MojoHandleSignalsState and testing satisfied_signals yourself: the
> return value is meaningful enough.

Thanks Ken. The code in MojoAudioDecoder does exactly that.

https://chromiumcodereview.appspot.com/2088633002/diff/1/media/mojo/services/...
media/mojo/services/mojo_demuxer_stream_adapter.cc:141:
base::ResetAndReturn(&read_cb_).Run(DemuxerStream::kAborted, nullptr);
On 2016/06/21 05:15:21, xhwang wrote:
> On 2016/06/21 00:15:50, Ken Rockot wrote:
> > On 2016/06/20 at 23:52:14, alokp wrote:
> > > Should I reset stream_pipe_ here and early return on the next
> > MojoDemuxerStreamAdapter::Read?
> > > 
> > > Or should MojoDemuxerStreamAdapter monitor |demuxer_stream_| connection
> error
> > and bubble the error to MojoRendererService to handle?
> > 
> > I'll let xhwang comment on this. I don't understand this code well enough to
> > have an opinion
> 
> The Renderer hosted by MojoRendererService will get the kAborted and handle
that
> as needed. So I don't feel we should have another channel to signal the
failure.
> Historically kAborted has been used for both abort during reset/teardown/seek,
> and failures. Maybe we should add a kError state to make this more clear.

Acknowledged.

[xhwang, 2016-06-21 17:24:42]

lgtm

[alokp, 2016-06-21 17:37:19]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2016-06-21 17:38:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2088633002/20001

[commit-bot: I haz the power, 2016-06-21 19:00:36]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-21 19:11:42]

Description was changed from

==========
Handles MOJO_HANDLE_SIGNAL_PEER_CLOSED in MojoDemuxerStreamAdapter.

BUG=621141
==========

to

==========
Handles MOJO_HANDLE_SIGNAL_PEER_CLOSED in MojoDemuxerStreamAdapter.

BUG=621141

Committed: https://crrev.com/57a3c25712bcf3135453838b575126b0e94be7d3
Cr-Commit-Position: refs/heads/master@{#401062}
==========

[commit-bot: I haz the power, 2016-06-21 19:11:44]

Patchset 2 (id:??) landed as
https://crrev.com/57a3c25712bcf3135453838b575126b0e94be7d3
Cr-Commit-Position: refs/heads/master@{#401062}