Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
318133002:
Always round up coded size to avoid VideoFrame restrictions. (Closed)
|
Created:
6 years, 6 months ago by DaleCurtis Modified:
6 years, 6 months ago Reviewers:
scherkus (not reviewing) CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAlways round up coded size to avoid VideoFrame restrictions.
Prior to http://crrev.com/268831 we never considered the actual
coded size when constructing buffers for FFmpeg. We do now, but
were not aligning the extents properly.
Sadly we had no test coverage for odd sized videos :( I've fixed
the extents and added a test using one of the bug clips (which
will be landed separately).
BUG=379127
TEST=new pipeline test
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=275719
Patch Set 1 : Test! #
Total comments: 4
Messages
Total messages: 21 (0 generated)
lgtm w/ questions https://codereview.chromium.org/318133002/diff/20001/media/filters/ffmpeg_vid... File media/filters/ffmpeg_video_decoder.cc (right): https://codereview.chromium.org/318133002/diff/20001/media/filters/ffmpeg_vid... media/filters/ffmpeg_video_decoder.cc:121: if (!VideoFrame::IsValidConfig( was this check failing with that test clip? https://codereview.chromium.org/318133002/diff/20001/media/filters/pipeline_i... File media/filters/pipeline_integration_test.cc (right): https://codereview.chromium.org/318133002/diff/20001/media/filters/pipeline_i... media/filters/pipeline_integration_test.cc:1439: // Verify that VP8A video with odd width/height can be played back. FYI I think the VpxVideoDecoder path got hit by the same thing, but it had CHECKs that were triggering http://crrev.com/238237
https://codereview.chromium.org/318133002/diff/20001/media/filters/ffmpeg_vid... File media/filters/ffmpeg_video_decoder.cc (right): https://codereview.chromium.org/318133002/diff/20001/media/filters/ffmpeg_vid... media/filters/ffmpeg_video_decoder.cc:121: if (!VideoFrame::IsValidConfig( On 2014/06/06 21:25:50, scherkus wrote: > was this check failing with that test clip? Yes. https://codereview.chromium.org/318133002/diff/20001/media/filters/pipeline_i... File media/filters/pipeline_integration_test.cc (right): https://codereview.chromium.org/318133002/diff/20001/media/filters/pipeline_i... media/filters/pipeline_integration_test.cc:1439: // Verify that VP8A video with odd width/height can be played back. On 2014/06/06 21:25:50, scherkus wrote: > FYI I think the VpxVideoDecoder path got hit by the same thing, but it had > CHECKs that were triggering > > http://crrev.com/238237 Ah, interesting. I think this is okay as a soft failure now that we have a test.
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/318133002/20001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/318133002/20001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/318133002/20001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/318133002/20001
Message was sent while issue was closed.
Change committed as 275719 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
