Updating OnPictureReady() method name per CR feedback

remoting/client/plugin/pepper_video_renderer_3d.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 "remoting/client/plugin/pepper_video_renderer_3d.h"
 
 #include <math.h>
 
 #include <algorithm>
 #include <utility>
 
 #include "ppapi/c/pp_codecs.h"
 #include "ppapi/c/ppb_opengles2.h"
 #include "ppapi/c/ppb_video_decoder.h"
 #include "ppapi/cpp/instance.h"
 #include "ppapi/lib/gl/include/GLES2/gl2.h"
 #include "ppapi/lib/gl/include/GLES2/gl2ext.h"
 #include "remoting/proto/video.pb.h"
 #include "remoting/protocol/frame_stats.h"
 #include "remoting/protocol/performance_tracker.h"
 #include "remoting/protocol/session_config.h"
 #include "third_party/webrtc/modules/desktop_capture/desktop_region.h"
 
 namespace remoting {
 
+namespace {
 // The implementation here requires that the decoder allocates at least 3
 // pictures. PPB_VideoDecoder didn't support this parameter prior to
 // 1.1, so we have to pass 0 for backwards compatibility with older versions of
 // the browser. Currently all API implementations allocate more than 3 buffers
 // by default.
 const uint32_t kMinimumPictureCount = 3;
+}  // namespace

[joedow, 2017/02/14 00:24:47]

This felt like it should be in an anonymous namespace so I wrapped it in this
CL.

 
 class PepperVideoRenderer3D::FrameTracker {
  public:
   FrameTracker(std::unique_ptr<VideoPacket> packet,
                protocol::FrameStatsConsumer* stats_consumer,
                const base::Closure& done)
       : packet_(std::move(packet)),
         stats_consumer_(stats_consumer),
         done_(done) {
     stats_.host_stats = protocol::HostFrameStats::GetForVideoPacket(*packet_);
@@ skipping 280 matching lines @@
     event_handler_->OnVideoDecodeError();
     return;
   }
 
   pending_frames_.front()->OnDecoded();
   // Move the frame from |pending_frames_| to |decoded_frames_|.
   decoded_frames_.splice(decoded_frames_.end(), pending_frames_,
                          pending_frames_.begin());
 
   DecodeNextPacket();
-  GetNextPicture();
+  GetNextPictureIfReady();
 }
 
-void PepperVideoRenderer3D::GetNextPicture() {
+void PepperVideoRenderer3D::GetNextPictureIfReady() {

[joedow, 2017/02/14 00:24:47]

Wez's comment about only needing the |decoded_frames_| check in OnPictureReady()
is correct, however it is really easy to call this function and not know about
this insidious and difficult to repro bug.  I would prefer to make all function
calls safe and update the method name.

[Wez, 2017/02/14 00:33:00]

The alternative would be to DCHECK(!decoded_frames_.empty()) here. :) (Which
raises the question of whether there is any way to mock out slow-decoder
behaviour to create a unit-test for this logic. ;)

[joedow, 2017/02/14 16:01:34]

The unit test is a fair point (as was the suggested of refactoring we had
offline).  I am unsure how much longer the plugin code is going to  live though
so I am hesitant to put more effort into it than fixing critical bugs.

   // Return early if |decoded_frames_| is empty or the decoder is already
   // preparing a picture.  If we call GetPicture() before a new frame has been
   // prepared (i.e. |decoded_frames_| is populated), the OnPictureReady callback
   // could be called before OnDecodeDone() is called which will cause a crash.
   // See crbug.com/689229 for more details.
   if (get_picture_pending_ || decoded_frames_.empty()) {
     return;
   }
 
   int32_t result =
@@ skipping 32 matching lines @@
   DCHECK_EQ(static_cast<int32_t>(picture.decode_id),
             decoded_frames_.front()->packet()->frame_id());
 
   // Move the frame from |decoded_frames_| to |next_picture_frames_|.
   next_picture_frames_.splice(next_picture_frames_.end(), decoded_frames_,
                               decoded_frames_.begin());
 
   next_picture_.reset(new Picture(&video_decoder_, picture));
 
   PaintIfNeeded();
-  GetNextPicture();
+  GetNextPictureIfReady();
 }
 
 void PepperVideoRenderer3D::PaintIfNeeded() {
   bool need_repaint = next_picture_ || (force_repaint_ && current_picture_);
   if (paint_pending_ || !need_repaint)
     return;
 
   if (next_picture_) {
     current_picture_ = std::move(next_picture_);
     current_picture_frames_.splice(current_picture_frames_.end(),
@@ skipping 180 matching lines @@
   gles2_if_->AttachShader(graphics_.pp_resource(), shader_program_, shader);
   gles2_if_->DeleteShader(graphics_.pp_resource(), shader);
 }
 
 void PepperVideoRenderer3D::CheckGLError() {
   GLenum error = gles2_if_->GetError(graphics_.pp_resource());
   CHECK_EQ(error, static_cast<GLenum>(GL_NO_ERROR)) << "GL error: " << error;
 }
 
 }  // namespace remoting