Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
177583002:
VAVDA: Switch to using max_num_reorder_frames. (Closed)
|
Created:
6 years, 10 months ago by Pawel Osciak Modified:
6 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, piman+watch_chromium.org, darin-cc_chromium.org, jam, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionVAVDA: Switch to using max_num_reorder_frames.
H.264 standard allows for an optional max_num_reorder_frames parameter to be
specified in VUI parameters. When present, it indicates the maximum number
of frames that may precede a given frame in decode order, but come after it
in output order. This means, that if the number of decoded and not yet outputted
frames exceeds max_num_reorder_frames, we are allowed to output until this no
longer holds true.
In case max_num_reorder_frames is not present, the value is to be inferred
to be either 0 or equal to the size of DPB. In case of the latter, we would
always have to wait until the DPB is full before outputting anything. This means
we could incur visible stuttering whenever DPB had to be cleared (on IDRs,
etc.). max_num_reorder_frames seems to be very common in streams these days
however, so it should be a relatively rare occurrence.
Relying on max_num_reorder_frames also gives us an ability to handle streams
with negative POC values, which are produced by some camera models.
TEST=video playbacks, vdatest, switchtests
BUG=chromium:177692
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255898
Patch Set 1 #
Total comments: 6
Patch Set 2 : #Patch Set 3 : #
Messages
Total messages: 42 (0 generated)
Can this be exercised by modifying the test stream in vdatest? https://chromiumcodereview.appspot.com/177583002/diff/1/media/filters/h264_pa... File media/filters/h264_parser.cc (right): https://chromiumcodereview.appspot.com/177583002/diff/1/media/filters/h264_pa... media/filters/h264_parser.cc:706: READ_BOOL_OR_RETURN(&sps->constraint_setx_flag[0]); s/0/i/ ?? https://chromiumcodereview.appspot.com/177583002/diff/1/media/filters/h264_pa... File media/filters/h264_parser.h (right): https://chromiumcodereview.appspot.com/177583002/diff/1/media/filters/h264_pa... media/filters/h264_parser.h:56: bool constraint_setx_flag[6]; This is C++ so bool is int8_t so bool[] is a pretty good way of confusing readers, IMO. Why the change?
On 2014/02/25 09:45:37, Ami Fischman wrote: > Can this be exercised by modifying the test stream in vdatest? > By "this" do you mean having max_num_reorder_frames in it, instead of not? Having it seems to be the more common case in the wild now, so it probably makes sense. Or well, at some point we could get more crafty and have a test stream with a few different SPSes, each with different values (well, apart from profile changes, that will not get through upper media layers)... I plan to rely on the VDA stress test for more thorough testing in different scenarios though.
On Tue, Feb 25, 2014 at 3:04 AM, <posciak@chromium.org> wrote: > By "this" do you mean having max_num_reorder_frames in it, instead of not? > Yes. I mean testing the new functionality this code now supports To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/02/25 19:14:23, Ami Fischman wrote: > On Tue, Feb 25, 2014 at 3:04 AM, <mailto:posciak@chromium.org> wrote: > > > By "this" do you mean having max_num_reorder_frames in it, instead of not? > > > > Yes. I mean testing the new functionality this code now supports max_num_reorder_frames is used always, for all streams, in the same way, including the existing test stream. It's just the way that value is calculated differs depending on the in-stream parameters. I can't have all of them in one test stream. Stress test will be testing those.
https://chromiumcodereview.appspot.com/177583002/diff/1/media/filters/h264_pa... File media/filters/h264_parser.cc (right): https://chromiumcodereview.appspot.com/177583002/diff/1/media/filters/h264_pa... media/filters/h264_parser.cc:706: READ_BOOL_OR_RETURN(&sps->constraint_setx_flag[0]); On 2014/02/25 09:45:37, Ami Fischman wrote: > s/0/i/ ?? Uh... yeah, and tests can't catch it, because default is false and that means max_num_reorder_frames becomes max, which is always ok (using m_n_r_f is an optional optimization). https://chromiumcodereview.appspot.com/177583002/diff/1/media/filters/h264_pa... File media/filters/h264_parser.h (right): https://chromiumcodereview.appspot.com/177583002/diff/1/media/filters/h264_pa... media/filters/h264_parser.h:56: bool constraint_setx_flag[6]; On 2014/02/25 09:45:37, Ami Fischman wrote: > This is C++ so bool is int8_t so bool[] is a pretty good way of confusing > readers, IMO. > Why the change? So this was actually used to store all six flags (6 bits) before. Now we keep each of them separately and treat them as all the other *_flag variables in this struct, i.e. actual bools. For me this way is definitely less confusing. Why would this be more confusing?
https://chromiumcodereview.appspot.com/177583002/diff/1/media/filters/h264_pa... File media/filters/h264_parser.cc (right): https://chromiumcodereview.appspot.com/177583002/diff/1/media/filters/h264_pa... media/filters/h264_parser.cc:706: READ_BOOL_OR_RETURN(&sps->constraint_setx_flag[0]); On 2014/02/26 06:02:15, Pawel Osciak wrote: > On 2014/02/25 09:45:37, Ami Fischman wrote: > > s/0/i/ ?? > > Uh... yeah, and tests can't catch it, because default is false and that means > max_num_reorder_frames becomes max, which is always ok (using m_n_r_f is an > optional optimization). I don't get it. m_n_r_f allows handling negative-POC streams, so it's not "just an optimization" - handling it correctly is required for some streams. I'm asking can the test stream use this feature to ensure we don't regress? (IDK what the "stress test" you mention is, but if you're saying you want to add a negative-POC stream to the autotest suite, instead, that's fine by me too) https://chromiumcodereview.appspot.com/177583002/diff/1/media/filters/h264_pa... File media/filters/h264_parser.h (right): https://chromiumcodereview.appspot.com/177583002/diff/1/media/filters/h264_pa... media/filters/h264_parser.h:56: bool constraint_setx_flag[6]; On 2014/02/26 06:02:15, Pawel Osciak wrote: > On 2014/02/25 09:45:37, Ami Fischman wrote: > > This is C++ so bool is int8_t so bool[] is a pretty good way of confusing > > readers, IMO. > > Why the change? > > So this was actually used to store all six flags (6 bits) before. Now we keep > each of them separately and treat them as all the other *_flag variables in this > struct, i.e. actual bools. > > For me this way is definitely less confusing. Why would this be more confusing? In the spec the bits are packed. Using a bool[] _looks_ like a packet array of bits, but in fact is a uint8[] where each uint8 carries one bit of information and 7 bits of empty space. I'd say either the old approach (single int to hold 6 bits) or 6 separate bool fields (like what the spec does) would be fine, but a bool[] is pretty much the worst choice.
On 2014/02/26 17:15:57, Ami Fischman wrote: > https://chromiumcodereview.appspot.com/177583002/diff/1/media/filters/h264_pa... > File media/filters/h264_parser.cc (right): > > https://chromiumcodereview.appspot.com/177583002/diff/1/media/filters/h264_pa... > media/filters/h264_parser.cc:706: > READ_BOOL_OR_RETURN(&sps->constraint_setx_flag[0]); > On 2014/02/26 06:02:15, Pawel Osciak wrote: > > On 2014/02/25 09:45:37, Ami Fischman wrote: > > > s/0/i/ ?? > > > > Uh... yeah, and tests can't catch it, because default is false and that means > > max_num_reorder_frames becomes max, which is always ok (using m_n_r_f is an > > optional optimization). > > I don't get it. m_n_r_f allows handling negative-POC streams, so it's not "just > an optimization" - handling it correctly is required for some streams. I'm > asking can the test stream use this feature to ensure we don't regress? > > (IDK what the "stress test" you mention is, but if you're saying you want to add > a negative-POC stream to the autotest suite, instead, that's fine by me too) > The "stress test" is a large suite of tests streams ran less frequently. If you are ok with that, I'll have it test the negative-POC streams. > https://chromiumcodereview.appspot.com/177583002/diff/1/media/filters/h264_pa... > File media/filters/h264_parser.h (right): > > https://chromiumcodereview.appspot.com/177583002/diff/1/media/filters/h264_pa... > media/filters/h264_parser.h:56: bool constraint_setx_flag[6]; > On 2014/02/26 06:02:15, Pawel Osciak wrote: > > On 2014/02/25 09:45:37, Ami Fischman wrote: > > > This is C++ so bool is int8_t so bool[] is a pretty good way of confusing > > > readers, IMO. > > > Why the change? > > > > So this was actually used to store all six flags (6 bits) before. Now we keep > > each of them separately and treat them as all the other *_flag variables in > this > > struct, i.e. actual bools. > > > > For me this way is definitely less confusing. Why would this be more > confusing? > > In the spec the bits are packed. Using a bool[] _looks_ like a packet array of > bits, but in fact is a uint8[] where each uint8 carries one bit of information > and 7 bits of empty space. > > I'd say either the old approach (single int to hold 6 bits) or 6 separate bool > fields (like what the spec does) would be fine, but a bool[] is pretty much the > worst choice. Ok changed, I'm not exactly convinced, but don't mind too much.
The CQ bit was checked by posciak@chromium.org
The CQ bit was unchecked by posciak@chromium.org
LGTM
The CQ bit was checked by posciak@google.com
The CQ bit was unchecked by posciak@google.com
The CQ bit was checked by posciak@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/posciak@chromium.org/177583002/60001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/posciak@chromium.org/177583002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by posciak@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/posciak@chromium.org/177583002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by posciak@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/posciak@chromium.org/177583002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by posciak@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/posciak@chromium.org/177583002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by posciak@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/posciak@chromium.org/177583002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by posciak@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/posciak@chromium.org/177583002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by posciak@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/posciak@chromium.org/177583002/60001
Message was sent while issue was closed.
Change committed as 255898 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
