Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1967893002:
H264POC: Allow gaps in frame_num. (Closed)
|
Created:
4 years, 7 months ago by sandersd (OOO until July 31) Modified:
4 years, 7 months ago Reviewers:
Pawel Osciak CC:
chromium-reviews, posciak+watch_chromium.org, piman+watch_chromium.org, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionH264POC: Allow gaps in frame_num.
This change only removes the restriction, it does not try to detect
all the cases where doing so is likely to result in problems. In most
cases (specifically, those where the number of missing frames is much
less than max_frame_num), the result will be correct.
In incorrect cases the reorder queue could get confused and result in
strange playback. The existing TODOs for the reorder queue explain how
to reduce that, but also inserting dummy frames for gaps would help as
well.
BUG=550854
Committed: https://crrev.com/b6f4380493084007dc4237d28153c0dd3e5d520b
Cr-Commit-Position: refs/heads/master@{#395164}
Committed: https://crrev.com/7b9ef9cd272c2e245424028df652291a4db283f8
Cr-Commit-Position: refs/heads/master@{#395661}
Patch Set 1 #Patch Set 2 : Fix gap detection. #
Total comments: 3
Patch Set 3 : Warning for redundant pictures. #Patch Set 4 : Update tests to not duplicate |frame_num|. #Patch Set 5 : Handle redundant slices. #
Messages
Total messages: 37 (14 generated)
sandersd@chromium.org changed reviewers: + posciak@chromium.org
posciak@: ping.
Given that H264POC is used by VTVDA only, and this change is based on the specifics of it, could we perhaps consider integrating it into VTVDA more tightly? This modification is based on the fact that VTVDA can cope with gaps to some extent, so we are diverging from the standard and we may perhaps prefer for this class to not be perceived as generally reusable (vs. using the H264Decoder)... What do you think? Thanks.
On 2016/05/13 02:00:10, Pawel Osciak wrote: > Given that H264POC is used by VTVDA only, and this change is based on the > specifics of it, could we perhaps consider integrating it into VTVDA more > tightly? Even if that was the only consideration, that would at most be a documentation change. I am very happy with the level of encapsulation of H264POC. > This modification is based on the fact that VTVDA can cope with gaps to some > extent, so we are diverging from the standard and we may perhaps prefer for this > class to not be perceived as generally reusable (vs. using the H264Decoder)... This class still precisely matches the H.264 standard for computation of POC. The ideal situation would be for H264Decoder to adopt this class specifically for POC computation, likely with modifications to support all the features it needs. (For example, if it is desirable for POC computation to trigger creation of missing pictures, a callback for that would be straightforward. That sounds a little backwards though.)
On 2016/05/13 18:03:47, sandersd wrote: > On 2016/05/13 02:00:10, Pawel Osciak wrote: > > Given that H264POC is used by VTVDA only, and this change is based on the > > specifics of it, could we perhaps consider integrating it into VTVDA more > > tightly? > > Even if that was the only consideration, that would at most be a documentation > change. I am very happy with the level of encapsulation of H264POC. > > > This modification is based on the fact that VTVDA can cope with gaps to some > > extent, so we are diverging from the standard and we may perhaps prefer for > this > > class to not be perceived as generally reusable (vs. using the H264Decoder)... > > This class still precisely matches the H.264 standard for computation of POC. > > The ideal situation would be for H264Decoder to adopt this class specifically > for POC computation, likely with modifications to support all the features it > needs. (For example, if it is desirable for POC computation to trigger creation > of missing pictures, a callback for that would be straightforward. That sounds a > little backwards though.) Ping.
On 2016/05/13 18:03:47, sandersd wrote: > On 2016/05/13 02:00:10, Pawel Osciak wrote: > > Given that H264POC is used by VTVDA only, and this change is based on the > > specifics of it, could we perhaps consider integrating it into VTVDA more > > tightly? > > Even if that was the only consideration, that would at most be a documentation > change. I am very happy with the level of encapsulation of H264POC. > > > This modification is based on the fact that VTVDA can cope with gaps to some > > extent, so we are diverging from the standard and we may perhaps prefer for > this > > class to not be perceived as generally reusable (vs. using the H264Decoder)... > > This class still precisely matches the H.264 standard for computation of POC. > > The ideal situation would be for H264Decoder to adopt this class specifically > for POC computation, likely with modifications to support all the features it > needs. (For example, if it is desirable for POC computation to trigger creation > of missing pictures, a callback for that would be straightforward. That sounds a > little backwards though.) Unfortunately, the calculation is not correct now, but you can very easily fix this; to generate correct POCs in presence of gaps, you need to move prev_frame_num_ forward to compensate for the gap frames (please see how H264Decoder does it here: https://code.google.com/p/chromium/codesearch#chromium/src/media/gpu/h264_dec...).
On 2016/05/18 08:02:39, Pawel Osciak wrote: > On 2016/05/13 18:03:47, sandersd wrote: > > On 2016/05/13 02:00:10, Pawel Osciak wrote: > > > Given that H264POC is used by VTVDA only, and this change is based on the > > > specifics of it, could we perhaps consider integrating it into VTVDA more > > > tightly? > > > > Even if that was the only consideration, that would at most be a documentation > > change. I am very happy with the level of encapsulation of H264POC. > > > > > This modification is based on the fact that VTVDA can cope with gaps to some > > > extent, so we are diverging from the standard and we may perhaps prefer for > > this > > > class to not be perceived as generally reusable (vs. using the > H264Decoder)... > > > > This class still precisely matches the H.264 standard for computation of POC. > > > > The ideal situation would be for H264Decoder to adopt this class specifically > > for POC computation, likely with modifications to support all the features it > > needs. (For example, if it is desirable for POC computation to trigger > creation > > of missing pictures, a callback for that would be straightforward. That sounds > a > > little backwards though.) > > Unfortunately, the calculation is not correct now, but you can very easily fix > this; to generate correct POCs in presence of gaps, you need to move > prev_frame_num_ forward to compensate for the gap frames (please see how > H264Decoder does it here: > https://code.google.com/p/chromium/codesearch#chromium/src/media/gpu/h264_dec...). I have verified that the behavior of this code is identical to the reference decoder. If we modified |prev_frame_num_|, then we would miss detecting wrapping, and so that logic would also be required here, but the computed result would be identical. FFmpeg has two implementations, but the version used by the decoder was rather too hard to follow to be certain for every case. (The version used by their parser is pretty clear, and is actually identical in structure to this CL--it entirely ignores gaps.)
lgtm % comment https://chromiumcodereview.appspot.com/1967893002/diff/20001/media/video/h264... File media/video/h264_poc.cc (right): https://chromiumcodereview.appspot.com/1967893002/diff/20001/media/video/h264... media/video/h264_poc.cc:71: slice_hdr.frame_num != (prev_frame_num_ + 1) % max_frame_num) { Not sure if this is intentional, but this changes the logic so that frame_num has to be strictly larger than prev_frame_num, although them being equal is sometimes valid (even without gaps).
https://chromiumcodereview.appspot.com/1967893002/diff/20001/media/video/h264... File media/video/h264_poc.cc (right): https://chromiumcodereview.appspot.com/1967893002/diff/20001/media/video/h264... media/video/h264_poc.cc:71: slice_hdr.frame_num != (prev_frame_num_ + 1) % max_frame_num) { On 2016/05/20 00:21:31, Pawel Osciak wrote: > Not sure if this is intentional, but this changes the logic so that frame_num > has to be strictly larger than prev_frame_num, although them being equal is > sometimes valid (even without gaps). My understanding of the spec is that this only happens when there is interlacing (not implemented by H264POC yet) or redundant pictures (these are not passed to H264POC). Would you prefer that H264POC can handle (or internally ignore) redundant pictures?
https://chromiumcodereview.appspot.com/1967893002/diff/20001/media/video/h264... File media/video/h264_poc.cc (right): https://chromiumcodereview.appspot.com/1967893002/diff/20001/media/video/h264... media/video/h264_poc.cc:71: slice_hdr.frame_num != (prev_frame_num_ + 1) % max_frame_num) { On 2016/05/20 00:39:09, sandersd wrote: > On 2016/05/20 00:21:31, Pawel Osciak wrote: > > Not sure if this is intentional, but this changes the logic so that frame_num > > has to be strictly larger than prev_frame_num, although them being equal is > > sometimes valid (even without gaps). > > My understanding of the spec is that this only happens when there is interlacing > (not implemented by H264POC yet) or redundant pictures (these are not passed to > H264POC). > > Would you prefer that H264POC can handle (or internally ignore) redundant > pictures? I just thought that not changing the condition would keep the existing behavior, but please feel free to make a decision based on what is supported by VTVDA.
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from posciak@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1967893002/#ps40001 (title: "Warning for redundant pictures.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967893002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967893002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from posciak@chromium.org Link to the patchset: https://codereview.chromium.org/1967893002/#ps60001 (title: "Update tests to not duplicate |frame_num|.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967893002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967893002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== H264POC: Allow gaps in frame_num. This change only removes the restriction, it does not try to detect all the cases where doing so is likely to result in problems. In most cases (specifically, those where the number of missing frames is much less than max_frame_num), the result will be correct. In incorrect cases the reorder queue could get confused and result in strange playback. The existing TODOs for the reorder queue explain how to reduce that, but also inserting dummy frames for gaps would help as well. BUG=550854 ========== to ========== H264POC: Allow gaps in frame_num. This change only removes the restriction, it does not try to detect all the cases where doing so is likely to result in problems. In most cases (specifically, those where the number of missing frames is much less than max_frame_num), the result will be correct. In incorrect cases the reorder queue could get confused and result in strange playback. The existing TODOs for the reorder queue explain how to reduce that, but also inserting dummy frames for gaps would help as well. BUG=550854 Committed: https://crrev.com/b6f4380493084007dc4237d28153c0dd3e5d520b Cr-Commit-Position: refs/heads/master@{#395164} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b6f4380493084007dc4237d28153c0dd3e5d520b Cr-Commit-Position: refs/heads/master@{#395164}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2008473002/ by rnephew@chromium.org. The reason for reverting is: Causing perf test failure on mac. crbug.com/614023.
Message was sent while issue was closed.
posciak@: PTAL, I've changed handling of redundant slices.
Message was sent while issue was closed.
Description was changed from ========== H264POC: Allow gaps in frame_num. This change only removes the restriction, it does not try to detect all the cases where doing so is likely to result in problems. In most cases (specifically, those where the number of missing frames is much less than max_frame_num), the result will be correct. In incorrect cases the reorder queue could get confused and result in strange playback. The existing TODOs for the reorder queue explain how to reduce that, but also inserting dummy frames for gaps would help as well. BUG=550854 Committed: https://crrev.com/b6f4380493084007dc4237d28153c0dd3e5d520b Cr-Commit-Position: refs/heads/master@{#395164} ========== to ========== H264POC: Allow gaps in frame_num. This change only removes the restriction, it does not try to detect all the cases where doing so is likely to result in problems. In most cases (specifically, those where the number of missing frames is much less than max_frame_num), the result will be correct. In incorrect cases the reorder queue could get confused and result in strange playback. The existing TODOs for the reorder queue explain how to reduce that, but also inserting dummy frames for gaps would help as well. BUG=550854 Committed: https://crrev.com/b6f4380493084007dc4237d28153c0dd3e5d520b Cr-Commit-Position: refs/heads/master@{#395164} ==========
The CQ bit was checked by sandersd@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967893002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967893002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sandersd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from posciak@chromium.org Link to the patchset: https://codereview.chromium.org/1967893002/#ps80001 (title: "Handle redundant slices.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1967893002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1967893002/80001
Message was sent while issue was closed.
Description was changed from ========== H264POC: Allow gaps in frame_num. This change only removes the restriction, it does not try to detect all the cases where doing so is likely to result in problems. In most cases (specifically, those where the number of missing frames is much less than max_frame_num), the result will be correct. In incorrect cases the reorder queue could get confused and result in strange playback. The existing TODOs for the reorder queue explain how to reduce that, but also inserting dummy frames for gaps would help as well. BUG=550854 Committed: https://crrev.com/b6f4380493084007dc4237d28153c0dd3e5d520b Cr-Commit-Position: refs/heads/master@{#395164} ========== to ========== H264POC: Allow gaps in frame_num. This change only removes the restriction, it does not try to detect all the cases where doing so is likely to result in problems. In most cases (specifically, those where the number of missing frames is much less than max_frame_num), the result will be correct. In incorrect cases the reorder queue could get confused and result in strange playback. The existing TODOs for the reorder queue explain how to reduce that, but also inserting dummy frames for gaps would help as well. BUG=550854 Committed: https://crrev.com/b6f4380493084007dc4237d28153c0dd3e5d520b Cr-Commit-Position: refs/heads/master@{#395164} ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== H264POC: Allow gaps in frame_num. This change only removes the restriction, it does not try to detect all the cases where doing so is likely to result in problems. In most cases (specifically, those where the number of missing frames is much less than max_frame_num), the result will be correct. In incorrect cases the reorder queue could get confused and result in strange playback. The existing TODOs for the reorder queue explain how to reduce that, but also inserting dummy frames for gaps would help as well. BUG=550854 Committed: https://crrev.com/b6f4380493084007dc4237d28153c0dd3e5d520b Cr-Commit-Position: refs/heads/master@{#395164} ========== to ========== H264POC: Allow gaps in frame_num. This change only removes the restriction, it does not try to detect all the cases where doing so is likely to result in problems. In most cases (specifically, those where the number of missing frames is much less than max_frame_num), the result will be correct. In incorrect cases the reorder queue could get confused and result in strange playback. The existing TODOs for the reorder queue explain how to reduce that, but also inserting dummy frames for gaps would help as well. BUG=550854 Committed: https://crrev.com/b6f4380493084007dc4237d28153c0dd3e5d520b Cr-Commit-Position: refs/heads/master@{#395164} Committed: https://crrev.com/7b9ef9cd272c2e245424028df652291a4db283f8 Cr-Commit-Position: refs/heads/master@{#395661} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/7b9ef9cd272c2e245424028df652291a4db283f8 Cr-Commit-Position: refs/heads/master@{#395661}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2036913002/ by ccameron@chromium.org. The reason for reverting is: Still causing stuttering on Mac. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
