Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2324843004:
Generate more valid configurations in media_vpx_video_decoder_fuzzer. (Closed)
DescriptionGenerate more valid configurations in media_vpx_video_decoder_fuzzer.
That helps to bypass some DCHECK()'s and to make the Debug build work.
Also that increases execution speed and helps to find new coverage.
Speed increased ~5-10 times. Also I see lot of NEW testcases while
running locally both Debug and Release builds.
R=jrummell@chromium.org
BUG=644672
Committed: https://crrev.com/29b3abb7b00ec613920892937199d39f684c9493
Cr-Commit-Position: refs/heads/master@{#430921}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Address comments by jrummell@. #Patch Set 3 : Rebase onto fresh master. #Patch Set 4 : Rebase + change the description. #Messages
Total messages: 17 (5 generated)
LG https://codereview.chromium.org/2324843004/diff/1/media/filters/vpx_video_dec... File media/filters/vpx_video_decoder_fuzzertest.cc (right): https://codereview.chromium.org/2324843004/diff/1/media/filters/vpx_video_dec... media/filters/vpx_video_decoder_fuzzertest.cc:53: // PIXEL_FORMAT_YV12 disabled if !defined(DISABLE_FFMPEG_VIDEO_DECODERS). I wouldn't worry about this case. DISABLE_FFMPEG_VIDEO_DECODERS is only set on Android. https://codereview.chromium.org/2324843004/diff/1/media/filters/vpx_video_dec... media/filters/vpx_video_decoder_fuzzertest.cc:56: pixel_format = PIXEL_FORMAT_YV12A; This is the same format. Did you mean YV12 for one of them? https://codereview.chromium.org/2324843004/diff/1/media/filters/vpx_video_dec... media/filters/vpx_video_decoder_fuzzertest.cc:62: pixel_format = PIXEL_FORMAT_YV12; Since this is a common format, I would make this case the default. No need to quit the test for case 3: (since we want to test the parser, and more attempts at calling Decode() are better). https://codereview.chromium.org/2324843004/diff/1/media/filters/vpx_video_dec... media/filters/vpx_video_decoder_fuzzertest.cc:78: auto coded_size = gfx::Size(rng() % 128, rng() % 128); width and height must be > 0, so use (rng() % 127) + 1 (and for natural_size as well). https://codereview.chromium.org/2324843004/diff/1/media/filters/vpx_video_dec... media/filters/vpx_video_decoder_fuzzertest.cc:79: auto visible_rect = gfx::Rect(rng() % 128, rng() % 128); Since visible_rect <= coded_size, I would just make it the same (i.e. visible_rect = gfx::Rect(coded_size)). I don't think the parser even uses it, so might as well improve the chances of the config being valid. https://codereview.chromium.org/2324843004/diff/1/media/filters/vpx_video_dec... media/filters/vpx_video_decoder_fuzzertest.cc:97: decoder.Decode(buffer, base::Bind(&OnDecodeComplete)); Decode() has a DCHECK to make sure Initialize passed(). Unfortunately there is no way to tell other than checking |success| on OnInitDone(). Not sure if the following would work: void OnInitDone(bool* result, bool success) { *result = success } bool result; ... base::Bind(&OnInitDone, &result) ... ... RunUntilIdle(); if (!result) return 0;
https://codereview.chromium.org/2324843004/diff/1/media/filters/vpx_video_dec... File media/filters/vpx_video_decoder_fuzzertest.cc (right): https://codereview.chromium.org/2324843004/diff/1/media/filters/vpx_video_dec... media/filters/vpx_video_decoder_fuzzertest.cc:53: // PIXEL_FORMAT_YV12 disabled if !defined(DISABLE_FFMPEG_VIDEO_DECODERS). On 2016/09/09 17:33:00, jrummell wrote: > I wouldn't worry about this case. DISABLE_FFMPEG_VIDEO_DECODERS is only set on > Android. Hmmm, it quickly crashes on Linux if I use PIXEL_FORMAT_YV12 for kCodecVP8. May be we build it incorrectly? Though look like for Linux we have to avoid using PIXEL_FORMAT_YV12 and kCodecVP8 together (https://cs.chromium.org/chromium/src/media/BUILD.gn?q=%22DISABLE_FFMPEG_VIDEO...). https://codereview.chromium.org/2324843004/diff/1/media/filters/vpx_video_dec... media/filters/vpx_video_decoder_fuzzertest.cc:56: pixel_format = PIXEL_FORMAT_YV12A; On 2016/09/09 17:33:00, jrummell wrote: > This is the same format. Did you mean YV12 for one of them? I left it here to discuss the point we've discussed in the comment above. I'll leave only PIXEL_FORMAT_YV12A for kCodecVP8, because otherwise it crashes: #7 0x7fbb7e65a8d7 in media::VpxVideoDecoder::DecodeBuffer(scoped_refptr<media::DecoderBuffer> const&, base::Callback<void (media::DecodeStatus), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) media/filters/vpx_video_decoder.cc:381:3 #8 0x7fbb7e65d634 in media::VpxVideoDecoder::Decode(scoped_refptr<media::DecoderBuffer> const&, base::Callback<void (media::DecodeStatus), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) media/filters/vpx_video_decoder.cc:432:5 https://codereview.chromium.org/2324843004/diff/1/media/filters/vpx_video_dec... media/filters/vpx_video_decoder_fuzzertest.cc:62: pixel_format = PIXEL_FORMAT_YV12; On 2016/09/09 17:33:00, jrummell wrote: > Since this is a common format, I would make this case the default. No need to > quit the test for case 3: (since we want to test the parser, and more attempts > at calling Decode() are better). It doesn't work with kCodecVP8, crashes pretty quickly (as showed above). For case 3, I used return because I don't believe that it can happen for rng() % 3. Should I remove that |default| case at all? https://codereview.chromium.org/2324843004/diff/1/media/filters/vpx_video_dec... media/filters/vpx_video_decoder_fuzzertest.cc:78: auto coded_size = gfx::Size(rng() % 128, rng() % 128); On 2016/09/09 17:33:00, jrummell wrote: > width and height must be > 0, so use (rng() % 127) + 1 (and for natural_size as > well). Done. https://codereview.chromium.org/2324843004/diff/1/media/filters/vpx_video_dec... media/filters/vpx_video_decoder_fuzzertest.cc:79: auto visible_rect = gfx::Rect(rng() % 128, rng() % 128); On 2016/09/09 17:33:00, jrummell wrote: > Since visible_rect <= coded_size, I would just make it the same (i.e. > visible_rect = gfx::Rect(coded_size)). I don't think the parser even uses it, so > might as well improve the chances of the config being valid. Done. https://codereview.chromium.org/2324843004/diff/1/media/filters/vpx_video_dec... media/filters/vpx_video_decoder_fuzzertest.cc:97: decoder.Decode(buffer, base::Bind(&OnDecodeComplete)); On 2016/09/09 17:33:00, jrummell wrote: > Decode() has a DCHECK to make sure Initialize passed(). Unfortunately there is > no way to tell other than checking |success| on OnInitDone(). Not sure if the > following would work: > > void OnInitDone(bool* result, bool success) { *result = success } > > bool result; > ... base::Bind(&OnInitDone, &result) ... > ... RunUntilIdle(); > if (!result) return 0; Actually, the restrictions implemented above provide successful initialization. Let me upload that patchset. Then I'll try to use the trick you've described.
By the way, we can simply disable that fuzzer for Debug build. We've done so for skia fuzzers because they provide lot of irrelevant crashes on debug asserts.
On 2016/09/15 17:57:59, mmoroz wrote:
> By the way, we can simply disable that fuzzer for Debug build. We've done so
for
> skia fuzzers because they provide lot of irrelevant crashes on debug asserts.
The trick you've suggested (void OnInitDone(bool* result, bool success) {
*result = success; }) works! Should we use it? Or disable Debug build for this
fuzzer?
On 2016/09/15 18:13:59, mmoroz wrote:
> On 2016/09/15 17:57:59, mmoroz wrote:
> > By the way, we can simply disable that fuzzer for Debug build. We've done so
> for
> > skia fuzzers because they provide lot of irrelevant crashes on debug
asserts.
>
> The trick you've suggested (void OnInitDone(bool* result, bool success) {
> *result = success; }) works! Should we use it? Or disable Debug build for this
> fuzzer?
(Sorry for the delay).
I've been thinking about this. We have the DCHECK to verify that callers are
using this API in the correct way, so I think it is best that the code only
proceed if Init() is successful. The API is not user accessible, so I don't
think it's a big concern that somebody will be calling Decode() without Init()
succeeding. In fact, I suspect there will be a lot of things that fail due to
Init() not setting them up properly, and it would simply mean that the DCHECK
becomes a CHECK, and the test will still need to verify that Init() succeeded.
The other failures are concerning. I'll do some more investigation on
PIXEL_FORMAT_YV12 with kCodecVP8 to see if we actually test that combination.
On 2016/09/23 00:50:13, jrummell wrote:
> On 2016/09/15 18:13:59, mmoroz wrote:
> > On 2016/09/15 17:57:59, mmoroz wrote:
> > > By the way, we can simply disable that fuzzer for Debug build. We've done
so
> > for
> > > skia fuzzers because they provide lot of irrelevant crashes on debug
> asserts.
> >
> > The trick you've suggested (void OnInitDone(bool* result, bool success) {
> > *result = success; }) works! Should we use it? Or disable Debug build for
this
> > fuzzer?
>
> (Sorry for the delay).
>
> I've been thinking about this. We have the DCHECK to verify that callers are
> using this API in the correct way, so I think it is best that the code only
> proceed if Init() is successful. The API is not user accessible, so I don't
> think it's a big concern that somebody will be calling Decode() without Init()
> succeeding. In fact, I suspect there will be a lot of things that fail due to
> Init() not setting them up properly, and it would simply mean that the DCHECK
> becomes a CHECK, and the test will still need to verify that Init() succeeded.
>
> The other failures are concerning. I'll do some more investigation on
> PIXEL_FORMAT_YV12 with kCodecVP8 to see if we actually test that combination.
John, can we land this version to have working debug build or any other
improvements are necessary?
I didn't realize that the other changes have been submitted. LGTM.
PS: You should probably change the description to simply indicate that this tries to generate realistic configs.
Description was changed from ========== Draft CL for bypassing DCHECK()'s by vpx_video_decoder_fuzzer. R=jrummell@chromium.org BUG=644672 ========== to ========== Generate more valid configurations in media_vpx_video_decoder_fuzzer. That helps to bypass some DCHECK()'s and to make the Debug build work. Also that increases execution speed and helps to find new coverage. Speed increased ~5-10 times. Also I see lot of NEW testcases while running locally both Debug and Release builds. R=jrummell@chromium.org BUG=644672 ==========
On 2016/11/08 18:26:34, jrummell wrote: > PS: You should probably change the description to simply indicate that this > tries to generate realistic configs. Thanks John for your help. I changed the description. The CL seems to be a positive change (speed increased for some reason + finding a lot of NEW inputs).
The CQ bit was checked by mmoroz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jrummell@chromium.org Link to the patchset: https://codereview.chromium.org/2324843004/#ps60001 (title: "Rebase + change the description.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Generate more valid configurations in media_vpx_video_decoder_fuzzer. That helps to bypass some DCHECK()'s and to make the Debug build work. Also that increases execution speed and helps to find new coverage. Speed increased ~5-10 times. Also I see lot of NEW testcases while running locally both Debug and Release builds. R=jrummell@chromium.org BUG=644672 ========== to ========== Generate more valid configurations in media_vpx_video_decoder_fuzzer. That helps to bypass some DCHECK()'s and to make the Debug build work. Also that increases execution speed and helps to find new coverage. Speed increased ~5-10 times. Also I see lot of NEW testcases while running locally both Debug and Release builds. R=jrummell@chromium.org BUG=644672 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Generate more valid configurations in media_vpx_video_decoder_fuzzer. That helps to bypass some DCHECK()'s and to make the Debug build work. Also that increases execution speed and helps to find new coverage. Speed increased ~5-10 times. Also I see lot of NEW testcases while running locally both Debug and Release builds. R=jrummell@chromium.org BUG=644672 ========== to ========== Generate more valid configurations in media_vpx_video_decoder_fuzzer. That helps to bypass some DCHECK()'s and to make the Debug build work. Also that increases execution speed and helps to find new coverage. Speed increased ~5-10 times. Also I see lot of NEW testcases while running locally both Debug and Release builds. R=jrummell@chromium.org BUG=644672 Committed: https://crrev.com/29b3abb7b00ec613920892937199d39f684c9493 Cr-Commit-Position: refs/heads/master@{#430921} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/29b3abb7b00ec613920892937199d39f684c9493 Cr-Commit-Position: refs/heads/master@{#430921} |
