FrameBuffer::NextFrame now return pair<frame, reason>.

webrtc/modules/video_coding/frame_buffer2.h

Index: webrtc/modules/video_coding/frame_buffer2.h
diff --git a/webrtc/modules/video_coding/frame_buffer2.h b/webrtc/modules/video_coding/frame_buffer2.h
index 0af2bf952350bc76674f5f42dcbf8f86fff8e80e..207fa096e45ab11165a08669279f2122a32226b0 100644
--- a/webrtc/modules/video_coding/frame_buffer2.h
+++ b/webrtc/modules/video_coding/frame_buffer2.h
@@ -20,6 +20,7 @@
 #include "webrtc/base/constructormagic.h"
 #include "webrtc/base/criticalsection.h"
 #include "webrtc/base/event.h"
+#include "webrtc/base/optional.h"
 #include "webrtc/base/thread_annotations.h"
 #include "webrtc/modules/video_coding/include/video_coding_defines.h"
 #include "webrtc/modules/video_coding/inter_frame_delay.h"
@@ -44,9 +45,15 @@ class FrameBuffer {
   void InsertFrame(std::unique_ptr<FrameObject> frame);
 
   // Get the next frame for decoding. Will return at latest after
-  // |max_wait_time_ms|, with either a managed FrameObject or an empty
-  // unique ptr if there is no available frame for decoding.
-  std::unique_ptr<FrameObject> NextFrame(int64_t max_wait_time_ms);
+  // |max_wait_time_ms|.
+  //  - If a frame is availiable within |max_wait_time_ms| it will return
+  //    an optional with a pointer to the frame.
+  //  - If no frame is available after |max_wait_time_ms| it will return
+  //    an optional with a nullptr value.
+  //  - If the FrameBuffer is stopped then it will return an optional

[danilchap, 2016/08/31 15:04:36]

I do not think rtc::Optional is a good tool to return reason why FrameBuffer
can't return a FrameObject.

Do you need to keep and handle empty FrameObjects?
may be create and return special kind of FrameObjects then.
it will take more space, but might be more readable.

Don't you have other, straightforward way to find out FrameBuffer is stopped?
May be add it.

or have an additional parameter Reason/Status to NextFrame:
enum Reason {
  OK = 200,
  FRAME_NOT_FOUND_IN_TIME = 408,
  END_OF_FRAME_BUFFER = 410
};
std::unique_ptr<FrameObject> NextFrame(int64_t wait_time, Reason* status);
or reverse:
Reason NextFrame(int64_t wait_time, std::unique_ptr<FrameObject>* frame)
or keep same input parameters:
std::tuple<std::unique_ptr<FrameObject>, Reason> NextFrame(int64_t wait_time);

[philipel, 2016/09/01 09:25:23]

I'm not sure I understand why rtc::Optional is the wrong tool in this case,
could you expand on that?

There is always a reason why a function might return an empty optional, but as
long as it is only for one specific reason and not due to multiple different
reasons I think it makes sense.

[danilchap, 2016/09/01 09:44:09]

You have three possible outcomes for this function, i.e. two distinct reasons to
return no frame.

Optional is good when you have binary outcome/one reason to fail (or reason
doesn't matter).
One way to see an optional is as an alternative to a pointer way to return 'no
value'.
NextFrame's return type is already a pointer.
So with all smart types removed it looks like
FrameObject** NextFrame(wait_time);
where returning nullptr means one thing and returning pointer to nullptr means a
different thing.

+  //    without a value.
+  rtc::Optional<std::unique_ptr<FrameObject>>
+      NextFrame(int64_t max_wait_time_ms);
 
   // Tells the FrameBuffer which protection mode that is in use. Affects
   // the frame timing.