Handles MOJO_HANDLE_SIGNAL_PEER_CLOSED in MojoDemuxerStreamAdapter.

media/mojo/services/mojo_demuxer_stream_adapter.cc

Index: media/mojo/services/mojo_demuxer_stream_adapter.cc
diff --git a/media/mojo/services/mojo_demuxer_stream_adapter.cc b/media/mojo/services/mojo_demuxer_stream_adapter.cc
index 9d540c60a3e5590c0fcd4afd75edf04f024e45dd..0d234bc3e8afccf11185bf7e2c2f2739c94e0955 100644
--- a/media/mojo/services/mojo_demuxer_stream_adapter.cc
+++ b/media/mojo/services/mojo_demuxer_stream_adapter.cc
@@ -112,27 +112,35 @@ void MojoDemuxerStreamAdapter::OnBufferReady(
   scoped_refptr<DecoderBuffer> media_buffer(
       buffer.To<scoped_refptr<DecoderBuffer>>());
 
-  if (!media_buffer->end_of_stream()) {
-    DCHECK_GT(media_buffer->data_size(), 0u);
+  if (media_buffer->end_of_stream()) {
+    base::ResetAndReturn(&read_cb_).Run(DemuxerStream::kOk, media_buffer);
+    return;
+  }
 
-    // Wait for the data to become available in the DataPipe.
-    MojoHandleSignalsState state;
-    CHECK_EQ(MOJO_RESULT_OK,
-             MojoWait(stream_pipe_.get().value(), MOJO_HANDLE_SIGNAL_READABLE,
-                      MOJO_DEADLINE_INDEFINITE, &state));
-    CHECK_EQ(MOJO_HANDLE_SIGNAL_READABLE, state.satisfied_signals);

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

I started hitting this CHECK after https://codereview.chromium.org/2075193002/

+  DCHECK_GT(media_buffer->data_size(), 0u);
+
+  // Wait for the data to become available in the DataPipe.

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

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.

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

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

+  MojoHandleSignalsState state;
+  CHECK_EQ(MOJO_RESULT_OK, MojoWait(stream_pipe_.get().value(),

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

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.

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

Thanks Ken. The code in MojoAudioDecoder does exactly that.

+                                    MOJO_HANDLE_SIGNAL_READABLE |
+                                        MOJO_HANDLE_SIGNAL_PEER_CLOSED,
+                                    MOJO_DEADLINE_INDEFINITE, &state));
 
+  if (state.satisfied_signals & MOJO_HANDLE_SIGNAL_READABLE) {
     // Read the inner data for the DecoderBuffer from our DataPipe.
     uint32_t bytes_to_read =
         base::checked_cast<uint32_t>(media_buffer->data_size());
     uint32_t bytes_read = bytes_to_read;
-    CHECK_EQ(ReadDataRaw(stream_pipe_.get(), media_buffer->writable_data(),
-                         &bytes_read, MOJO_READ_DATA_FLAG_ALL_OR_NONE),
-             MOJO_RESULT_OK);
+    CHECK_EQ(MOJO_RESULT_OK,
+             ReadDataRaw(stream_pipe_.get(), media_buffer->writable_data(),
+                         &bytes_read, MOJO_READ_DATA_FLAG_ALL_OR_NONE));
     CHECK_EQ(bytes_to_read, bytes_read);
+    base::ResetAndReturn(&read_cb_).Run(DemuxerStream::kOk, media_buffer);
+  } else {
+    CHECK_EQ(MOJO_HANDLE_SIGNAL_PEER_CLOSED, state.satisfied_signals);
+    base::ResetAndReturn(&read_cb_).Run(DemuxerStream::kAborted, nullptr);

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

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]

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]

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]

Acknowledged.

+    return;
   }
-
-  base::ResetAndReturn(&read_cb_).Run(DemuxerStream::kOk, media_buffer);
 }
 
 void MojoDemuxerStreamAdapter::UpdateConfig(