Generate more valid configurations in media_vpx_video_decoder_fuzzer.

media/filters/vpx_video_decoder_fuzzertest.cc

Index: media/filters/vpx_video_decoder_fuzzertest.cc
diff --git a/media/filters/vpx_video_decoder_fuzzertest.cc b/media/filters/vpx_video_decoder_fuzzertest.cc
index f22040bcb8b55c84c7b6a83cd39102e9190d5e5d..1007a74626b2829fc101ea470eda5a3aae560391 100644
--- a/media/filters/vpx_video_decoder_fuzzertest.cc
+++ b/media/filters/vpx_video_decoder_fuzzertest.cc
@@ -44,10 +44,36 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
   }
 
   // Compute randomized constants. Put all rng() usages here.
-  auto codec = static_cast<VideoCodec>(rng() % kVideoCodecMax);
+  // Use only values that pass DCHECK in VpxVideoDecoder::ConfigureDecoder().
+  VideoCodec codec;
+  VideoPixelFormat pixel_format;
+  if (rng() & 1) {
+    codec = kCodecVP8;
+    if (rng() & 1) {
+      // PIXEL_FORMAT_YV12 disabled if !defined(DISABLE_FFMPEG_VIDEO_DECODERS).

[jrummell, 2016/09/09 17:33:00]

I wouldn't worry about this case. DISABLE_FFMPEG_VIDEO_DECODERS is only set on
Android.

[mmoroz, 2016/09/15 17:56:02]

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...).

+      pixel_format = PIXEL_FORMAT_YV12A;
+    } else {
+      pixel_format = PIXEL_FORMAT_YV12A;

[jrummell, 2016/09/09 17:33:00]

This is the same format. Did you mean YV12 for one of them?

[mmoroz, 2016/09/15 17:56:01]

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

+    }
+  } else {
+    codec = kCodecVP9;
+    switch (rng() % 3) {
+      case 0:
+        pixel_format = PIXEL_FORMAT_YV12;

[jrummell, 2016/09/09 17:33:00]

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).

[mmoroz, 2016/09/15 17:56:02]

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?

+        break;
+      case 1:
+        pixel_format = PIXEL_FORMAT_YV12A;
+        break;
+      case 2:
+        pixel_format = PIXEL_FORMAT_YV24;
+        break;
+      default:
+        return 0;
+    }
+  }
+
   auto profile =
       static_cast<VideoCodecProfile>(rng() % VIDEO_CODEC_PROFILE_MAX);
-  auto pixel_format = static_cast<VideoPixelFormat>(rng() % PIXEL_FORMAT_MAX);
   auto color_space = static_cast<ColorSpace>(rng() % COLOR_SPACE_MAX);
   auto coded_size = gfx::Size(rng() % 128, rng() % 128);

[jrummell, 2016/09/09 17:33:00]

width and height must be > 0, so use (rng() % 127) + 1 (and for natural_size as
well).

[mmoroz, 2016/09/15 17:56:01]

Done.

   auto visible_rect = gfx::Rect(rng() % 128, rng() % 128);

[jrummell, 2016/09/09 17:33:00]

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.

[mmoroz, 2016/09/15 17:56:01]

Done.

@@ -57,6 +83,9 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
                             coded_size, visible_rect, natural_size,
                             EmptyExtraData(), Unencrypted());
 
+  if (!config.IsValidConfig())
+    return 0;
+
   VpxVideoDecoder decoder;
   base::RunLoop run_loop;
 
@@ -66,7 +95,8 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
 
   auto buffer = DecoderBuffer::CopyFrom(data, size);
   decoder.Decode(buffer, base::Bind(&OnDecodeComplete));

[jrummell, 2016/09/09 17:33:00]

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;

[mmoroz, 2016/09/15 17:56:02]

Actually, the restrictions implemented above provide successful initialization.
Let me upload that patchset. Then I'll try to use the trick you've described.

-  run_loop.RunUntilIdle();
+  // Otherwise crashes on DCHECK in RunLoop::BeforeRun().
+  run_loop.QuitWhenIdle();
 
   return 0;
 }