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

FrameBuffer::NextFrame now return a ReturnReason and take an additional std::unique_ptr<FrameObject>* as output parameter.

In order to distinguish between a return caused by the FrameBuffer
being stopped and a return caused by a timeout we now return
a ReturnReason.

BUG=webrtc:5514
R=danilchap@webrtc.org, mflodman@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/7556282a6ce98f243b52d4db436f162e93e69abb

[philipel, 2016-08-31 13:59:02]

philipel@webrtc.org changed reviewers:
+ danilchap@webrtc.org, mflodman@webrtc.org

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

https://codereview.webrtc.org/2302473002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2.h (right):

https://codereview.webrtc.org/2302473002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.h:53: //  - If the FrameBuffer is
stopped then it will return an optional
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]

https://codereview.webrtc.org/2302473002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2.h (right):

https://codereview.webrtc.org/2302473002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.h:53: //  - If the FrameBuffer is
stopped then it will return an optional
On 2016/08/31 15:04:36, danilchap wrote:
> 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);

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]

https://codereview.webrtc.org/2302473002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2.h (right):

https://codereview.webrtc.org/2302473002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.h:53: //  - If the FrameBuffer is
stopped then it will return an optional
On 2016/09/01 09:25:23, philipel wrote:
> On 2016/08/31 15:04:36, danilchap wrote:
> > 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);
> 
> 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.

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.

[philipel, 2016-09-01 10:40:04]

Description was changed from

==========
FrameBuffer::NextFrame now return rtc::Optional.

In order to distinguish between a return caused by the FrameBuffer
being stopped and a return caused by a timeout we now return an
rtc::Optional<std::unique_ptr> instead of a std::unique_ptr.

BUG=webrtc:5514
==========

to

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

In order to distinguish between a return caused by the FrameBuffer
being stopped and a return caused by a timeout we now return an
std::pair<ReturnReason, std::unique_ptr> instead of a std::unique_ptr.

BUG=webrtc:5514
==========

[danilchap, 2016-09-01 11:15:21]

looks more clear to me now.
just some style/name comments now.
(and update CL title)

https://codereview.webrtc.org/2302473002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2.h (right):

https://codereview.webrtc.org/2302473002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.h:23: #include
"webrtc/base/optional.h"
not needed

https://codereview.webrtc.org/2302473002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.h:44: enum ReturnReason {
enum should go before constructor.
Probably make it enum class.
ReturnStatus or FrameStatus probably better names (I'll take previously
suggested name Reason back. this type represent more than just a Reason for not
returning a frame)

https://codereview.webrtc.org/2302473002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.h:45: kFrameFound,
if you choose FrameStatus for the enum type, then this status probably better
rename to kFound

https://codereview.webrtc.org/2302473002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.h:61: std::pair<ReturnReason,
std::unique_ptr<FrameObject>>
in stl it is more often when a pair<object, status> is returned then object goes
first, status - second (thinking about methods like std::set::insert).
Unless you try to make it consistence with something else, make it more
consistent with stl and swap types in the pair.

[philipel, 2016-09-01 11:37:56]

Description was changed from

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

In order to distinguish between a return caused by the FrameBuffer
being stopped and a return caused by a timeout we now return an
std::pair<ReturnReason, std::unique_ptr> instead of a std::unique_ptr.

BUG=webrtc:5514
==========

to

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

In order to distinguish between a return caused by the FrameBuffer
being stopped and a return caused by a timeout we now return an
std::pair<std::unique_ptr, ReturnReason> instead of a std::unique_ptr.

BUG=webrtc:5514
==========

[philipel, 2016-09-01 11:38:03]

https://codereview.webrtc.org/2302473002/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2.h (right):

https://codereview.webrtc.org/2302473002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.h:23: #include
"webrtc/base/optional.h"
On 2016/09/01 11:15:21, danilchap wrote:
> not needed

Done.

https://codereview.webrtc.org/2302473002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.h:44: enum ReturnReason {
On 2016/09/01 11:15:21, danilchap wrote:
> enum should go before constructor.
> Probably make it enum class.
> ReturnStatus or FrameStatus probably better names (I'll take previously
> suggested name Reason back. this type represent more than just a Reason for
not
> returning a frame)

Acknowledged.

https://codereview.webrtc.org/2302473002/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.h:45: kFrameFound,
On 2016/09/01 11:15:21, danilchap wrote:
> if you choose FrameStatus for the enum type, then this status probably better
> rename to kFound

Acknowledged.

[danilchap, 2016-09-01 12:11:27]

lgtm

[philipel, 2016-09-01 12:35:16]

The CQ bit was checked by philipel@webrtc.org

[commit-bot: I haz the power, 2016-09-01 12:35:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-01 13:03:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 13:03:17]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/7950)

[mflodman, 2016-09-02 12:05:57]

https://codereview.webrtc.org/2302473002/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2.h (right):

https://codereview.webrtc.org/2302473002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.h:60:
std::pair<std::unique_ptr<FrameObject>, ReturnReason>
I'm not a big fan of returning a pair like this, wdyt of instead moving
FrameObject to the argument list?

[philipel, 2016-09-02 12:41:24]

Description was changed from

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

In order to distinguish between a return caused by the FrameBuffer
being stopped and a return caused by a timeout we now return an
std::pair<std::unique_ptr, ReturnReason> instead of a std::unique_ptr.

BUG=webrtc:5514
==========

to

==========
FrameBuffer::NextFrame now return a ReturnReason and take an additional
std::unique_ptr<FrameObject>* as output parameter.

In order to distinguish between a return caused by the FrameBuffer
being stopped and a return caused by a timeout we now return
a ReturnReason.

BUG=webrtc:5514
==========

[philipel, 2016-09-02 12:41:31]

https://codereview.webrtc.org/2302473002/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2.h (right):

https://codereview.webrtc.org/2302473002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.h:60:
std::pair<std::unique_ptr<FrameObject>, ReturnReason>
On 2016/09/02 12:05:57, mflodman wrote:
> I'm not a big fan of returning a pair like this, wdyt of instead moving
> FrameObject to the argument list?

The idea was to follow the stl style where pair<object, status> often is
returned, changed now to take an additional output parameter.

[mflodman, 2016-09-02 12:46:49]

LGTM

[philipel, 2016-09-02 12:47:20]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2016-09-02 12:47:21]

The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2302473002/#ps100001 (title:
"Comment fix")

[commit-bot: I haz the power, 2016-09-02 12:47:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-02 14:47:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-02 14:47:50]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_drmemory_light on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_clang_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_clang_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_gyp_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_gyp_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_x64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[philipel, 2016-09-05 08:57:59]

Description was changed from

==========
FrameBuffer::NextFrame now return a ReturnReason and take an additional
std::unique_ptr<FrameObject>* as output parameter.

In order to distinguish between a return caused by the FrameBuffer
being stopped and a return caused by a timeout we now return
a ReturnReason.

BUG=webrtc:5514
==========

to

==========
FrameBuffer::NextFrame now return a ReturnReason and take an additional
std::unique_ptr<FrameObject>* as output parameter.

In order to distinguish between a return caused by the FrameBuffer
being stopped and a return caused by a timeout we now return
a ReturnReason.

BUG=webrtc:5514
R=danilchap@webrtc.org, mflodman@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/7556282a6ce98f243b52d4db4...
==========

[commit-bot: I haz the power, 2016-09-05 08:58:00]

Description was changed from

==========
FrameBuffer::NextFrame now return a ReturnReason and take an additional
std::unique_ptr<FrameObject>* as output parameter.

In order to distinguish between a return caused by the FrameBuffer
being stopped and a return caused by a timeout we now return
a ReturnReason.

BUG=webrtc:5514
R=danilchap@webrtc.org, mflodman@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/7556282a6ce98f243b52d4db4...
==========

to

==========
FrameBuffer::NextFrame now return a ReturnReason and take an additional
std::unique_ptr<FrameObject>* as output parameter.

In order to distinguish between a return caused by the FrameBuffer
being stopped and a return caused by a timeout we now return
a ReturnReason.

BUG=webrtc:5514
R=danilchap@webrtc.org, mflodman@webrtc.org

Committed: https://crrev.com/7556282a6ce98f243b52d4db436f162e93e69abb
Cr-Commit-Position: refs/heads/master@{#14065}
==========

[philipel, 2016-09-05 08:58:01]

Committed patchset #6 (id:100001) manually as
7556282a6ce98f243b52d4db436f162e93e69abb (presubmit successful).

[commit-bot: I haz the power, 2016-09-05 08:58:01]

Patchset 6 (id:??) landed as
https://crrev.com/7556282a6ce98f243b52d4db436f162e93e69abb
Cr-Commit-Position: refs/heads/master@{#14065}