Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
171233007:
Don't process pending prefetch events if we fail to create video decoder job (Closed)
|
Created:
6 years, 10 months ago by qinmin Modified:
6 years, 10 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDon't process pending prefetch events if we fail to create video decoder job
In the current logic, we continue to process prefetch requests if video decoder job fails to create due to empty surface.
However, that may lead to a NPE because we do not do null check on video_decoder_job_ in our code
Since StartInternal() won't do anything if video decoder job is null, we can return early when the surface is empty.
Playback will continue once a non-empty surface is passed to MSP.
BUG=343508
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252489
Patch Set 1 #
Total comments: 2
Patch Set 2 : modify the test to have a pending prefetch request after setting an empty surface #
Total comments: 4
Patch Set 3 : nits #
Messages
Total messages: 15 (0 generated)
PTAL
https://codereview.chromium.org/171233007/diff/1/media/base/android/media_sou... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/171233007/diff/1/media/base/android/media_sou... media/base/android/media_source_player_unittest.cc:901: player_.SetVideoSurface(empty_surface.Pass()); Is this where the NPE previously occurred prior to this patch? Shouldn't the player already have entered PREFETCH_DONE_EVENT_PENDING in StartVideoDecoderJob(true), above? Then, OnPrefetchDone() is invoked by OnDemuxerDataAvailable(), above, and PREFETCH_DONE_EVENT_PENDING is cleared and VDJ->Decode() is invoked on the prefetched data. So at this point, no event is pending, IIUC. Is perhaps there some race between OnPrefetchDone()'s calling of DecodeMoreVideo() (which could NPE if VDJ is null) and resetting the VDJ? If so, I'm not sure this patch fixes that issue. In general, the early ProcessPendingEvents() return condition added by the patch looks good, but I'm not sure that it solves the problem (or how this test tests the problem's existence and solution).
On 2014/02/20 20:52:10, wolenetz wrote: > https://codereview.chromium.org/171233007/diff/1/media/base/android/media_sou... > File media/base/android/media_source_player_unittest.cc (right): > > https://codereview.chromium.org/171233007/diff/1/media/base/android/media_sou... > media/base/android/media_source_player_unittest.cc:901: > player_.SetVideoSurface(empty_surface.Pass()); > Is this where the NPE previously occurred prior to this patch? Shouldn't the > player already have entered PREFETCH_DONE_EVENT_PENDING in > StartVideoDecoderJob(true), above? Then, OnPrefetchDone() is invoked by > OnDemuxerDataAvailable(), above, and PREFETCH_DONE_EVENT_PENDING is cleared and > VDJ->Decode() is invoked on the prefetched data. So at this point, no event is > pending, IIUC. > > Is perhaps there some race between OnPrefetchDone()'s calling of > DecodeMoreVideo() (which could NPE if VDJ is null) and resetting the VDJ? If so, > I'm not sure this patch fixes that issue. > > In general, the early ProcessPendingEvents() return condition added by the patch > looks good, but I'm not sure that it solves the problem (or how this test tests > the problem's existence and solution). Could the root problem simply be that MDJ::received_data_ is not initialized properly, so HasData() that occurs during Prefetch() attempts to invoke size() on an uninitialized access_units vector?
On 2014/02/20 21:05:25, wolenetz wrote: > On 2014/02/20 20:52:10, wolenetz wrote: > > > https://codereview.chromium.org/171233007/diff/1/media/base/android/media_sou... > > File media/base/android/media_source_player_unittest.cc (right): > > > > > https://codereview.chromium.org/171233007/diff/1/media/base/android/media_sou... > > media/base/android/media_source_player_unittest.cc:901: > > player_.SetVideoSurface(empty_surface.Pass()); > > Is this where the NPE previously occurred prior to this patch? Shouldn't the > > player already have entered PREFETCH_DONE_EVENT_PENDING in > > StartVideoDecoderJob(true), above? Then, OnPrefetchDone() is invoked by > > OnDemuxerDataAvailable(), above, and PREFETCH_DONE_EVENT_PENDING is cleared > and > > VDJ->Decode() is invoked on the prefetched data. So at this point, no event is > > pending, IIUC. > > > > Is perhaps there some race between OnPrefetchDone()'s calling of > > DecodeMoreVideo() (which could NPE if VDJ is null) and resetting the VDJ? If > so, > > I'm not sure this patch fixes that issue. > > > > In general, the early ProcessPendingEvents() return condition added by the > patch > > looks good, but I'm not sure that it solves the problem (or how this test > tests > > the problem's existence and solution). > > Could the root problem simply be that MDJ::received_data_ is not initialized > properly, so HasData() that occurs during Prefetch() attempts to invoke size() > on an uninitialized access_units vector? Please ignore my last comment (#3). My concern is simply that the new test does not repro the bug's failure stack/scenario (without the rest of the patch applied) for me on Nexus 4. Indeed, all 58 tests pass locally for me on Nexus 4, including the new one from patch set 1, using ToT's MSP.cc.
On 2014/02/20 21:38:54, wolenetz wrote: > On 2014/02/20 21:05:25, wolenetz wrote: > > On 2014/02/20 20:52:10, wolenetz wrote: > > > > > > https://codereview.chromium.org/171233007/diff/1/media/base/android/media_sou... > > > File media/base/android/media_source_player_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/171233007/diff/1/media/base/android/media_sou... > > > media/base/android/media_source_player_unittest.cc:901: > > > player_.SetVideoSurface(empty_surface.Pass()); > > > Is this where the NPE previously occurred prior to this patch? Shouldn't the > > > player already have entered PREFETCH_DONE_EVENT_PENDING in > > > StartVideoDecoderJob(true), above? Then, OnPrefetchDone() is invoked by > > > OnDemuxerDataAvailable(), above, and PREFETCH_DONE_EVENT_PENDING is cleared > > and > > > VDJ->Decode() is invoked on the prefetched data. So at this point, no event > is > > > pending, IIUC. > > > > > > Is perhaps there some race between OnPrefetchDone()'s calling of > > > DecodeMoreVideo() (which could NPE if VDJ is null) and resetting the VDJ? If > > so, > > > I'm not sure this patch fixes that issue. > > > > > > In general, the early ProcessPendingEvents() return condition added by the > > patch > > > looks good, but I'm not sure that it solves the problem (or how this test > > tests > > > the problem's existence and solution). > > > > Could the root problem simply be that MDJ::received_data_ is not initialized > > properly, so HasData() that occurs during Prefetch() attempts to invoke size() > > on an uninitialized access_units vector? > > Please ignore my last comment (#3). My concern is simply that the new test does > not repro the bug's failure stack/scenario (without the rest of the patch > applied) for me on Nexus 4. Indeed, all 58 tests pass locally for me on Nexus 4, > including the new one from patch set 1, using ToT's MSP.cc. modify the test to repro the crash without the patch.
https://codereview.chromium.org/171233007/diff/1/media/base/android/media_sou... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/171233007/diff/1/media/base/android/media_sou... media/base/android/media_source_player_unittest.cc:901: player_.SetVideoSurface(empty_surface.Pass()); Added a TriggerPlayerStarvation() call to let the player to have a PREFETCH_DONE_EVENT_PENDING status after setting the empty surface. On 2014/02/20 20:52:11, wolenetz wrote: > Is this where the NPE previously occurred prior to this patch? Shouldn't the > player already have entered PREFETCH_DONE_EVENT_PENDING in > StartVideoDecoderJob(true), above? Then, OnPrefetchDone() is invoked by > OnDemuxerDataAvailable(), above, and PREFETCH_DONE_EVENT_PENDING is cleared and > VDJ->Decode() is invoked on the prefetched data. So at this point, no event is > pending, IIUC. > > Is perhaps there some race between OnPrefetchDone()'s calling of > DecodeMoreVideo() (which could NPE if VDJ is null) and resetting the VDJ? If so, > I'm not sure this patch fixes that issue. > > In general, the early ProcessPendingEvents() return condition added by the patch > looks good, but I'm not sure that it solves the problem (or how this test tests > the problem's existence and solution).
lgtm % nits I also confirmed on Nexus 4 that Release mode crash with stack with the new test but without the patched MSP.cc looks like the referenced bug, and no crash occurs with the patched MSP.cc. Thanks! https://codereview.chromium.org/171233007/diff/90001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/171233007/diff/90001/media/base/android/media... media/base/android/media_source_player_unittest.cc:889: TEST_F(MediaSourcePlayerTest, SetEmptySurfaceWhileDecoding) { nit: s/SurfaceWhile/SurfaceAndStarveWhile/ https://codereview.chromium.org/171233007/diff/90001/media/base/android/media... media/base/android/media_source_player_unittest.cc:911: // No seek and data request should have been received since the surface is nit: s/ No seek and data request / No further seek or data requests /
https://codereview.chromium.org/171233007/diff/90001/media/base/android/media... File media/base/android/media_source_player_unittest.cc (right): https://codereview.chromium.org/171233007/diff/90001/media/base/android/media... media/base/android/media_source_player_unittest.cc:889: TEST_F(MediaSourcePlayerTest, SetEmptySurfaceWhileDecoding) { On 2014/02/20 23:23:31, wolenetz wrote: > nit: s/SurfaceWhile/SurfaceAndStarveWhile/ Done. https://codereview.chromium.org/171233007/diff/90001/media/base/android/media... media/base/android/media_source_player_unittest.cc:911: // No seek and data request should have been received since the surface is On 2014/02/20 23:23:31, wolenetz wrote: > nit: s/ No seek and data request / No further seek or data requests / Done.
The CQ bit was checked by qinmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/171233007/130001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, events_unittests, google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by qinmin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/171233007/130001
Message was sent while issue was closed.
Change committed as 252489 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
