|
|
DescriptionFrameBuffer::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
Patch Set 1 #Patch Set 2 : Comment fix. #
Total comments: 3
Patch Set 3 : Return pair<reason, frame> instead of optional. #
Total comments: 7
Patch Set 4 : Feedback fixes. #
Total comments: 2
Patch Set 5 : Frame as output paramter. #Patch Set 6 : Comment fix #
Messages
Total messages: 28 (11 generated)
philipel@webrtc.org changed reviewers: + danilchap@webrtc.org, mflodman@webrtc.org
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);
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.
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.
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 ==========
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.
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 ==========
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.
lgtm
The CQ bit was checked by philipel@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/7950)
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?
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 ==========
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.
LGTM
The CQ bit was checked by philipel@webrtc.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
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)
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... ==========
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} ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) manually as 7556282a6ce98f243b52d4db436f162e93e69abb (presubmit successful).
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/7556282a6ce98f243b52d4db436f162e93e69abb Cr-Commit-Position: refs/heads/master@{#14065} |