Handles MOJO_HANDLE_SIGNAL_PEER_CLOSED in MojoDemuxerStreamAdapter.

media/mojo/services/mojo_demuxer_stream_adapter.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/mojo/services/mojo_demuxer_stream_adapter.h"
 
 #include <stdint.h>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 94 matching lines @@
 
   if (status == mojom::DemuxerStream::Status::ABORTED) {
     base::ResetAndReturn(&read_cb_).Run(DemuxerStream::kAborted, nullptr);
     return;
   }
 
   DCHECK_EQ(status, mojom::DemuxerStream::Status::OK);
   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(
     mojom::AudioDecoderConfigPtr audio_config,
     mojom::VideoDecoderConfigPtr video_config) {
   DCHECK_NE(type_, DemuxerStream::UNKNOWN);
 
   switch(type_) {
     case DemuxerStream::AUDIO:
       DCHECK(audio_config && !video_config);
       audio_config_ = audio_config.To<AudioDecoderConfig>();
       break;
     case DemuxerStream::VIDEO:
       DCHECK(video_config && !audio_config);
       video_config_ = video_config.To<VideoDecoderConfig>();
       break;
     default:
       NOTREACHED();
   }
 }
 
 }  // namespace media