Possible errors in media/ code found by running /analyze on Windows |
||||
Issue descriptionThere is a Windows bot that runs /analyze on Windows. Looking at the latest run [1], I searched for warnings generated by media code. Most of the complaints are about "Local declaration of "x" hides declaration of the same name in outer scope." However, there are a couple of other errors that should be investigated as they may be potential issues. media\filters\wsola_internals.cc(207) : warning C6385: Reading invalid data from "similarity": the readable size is "_Old_20`8" bytes, but "12" bytes may be read.: Lines: 164, 165, 166, 167, 168, 170, 171, 173, 178, 179, 181, 182, 186, 188, 192, 193, 199, 200, 203, 207, 224, 231, 199, 200, 203, 207 (Not sure about this one. No idea what "_Old_20`8" is, but the array is float[3] which is 12 bytes.) media\base\video_frame.cc(1156) : warning C6001: Using uninitialized memory "offset".: Lines: 1127, 1130, 1131, 1132, 1147, 1148, 1150, 1152, 1155, 1156 (Not sure this is valid. "offset" is only referenced on 3 lines, so not sure why it lists 10 lines. And both loops (one writes it, the other reads it) only index up to NumPlanes(format_), so unclear how uninitialized values are used.) media\gpu\fake_video_decode_accelerator.cc(109) : warning C6336: Arithmetic operator has precedence over question operator, use parentheses to clarify intent. media\gpu\d3d11_h264_accelerator.cc(305) : warning C6386: Buffer overrun while writing to "buffer": the writable size is "buffer_size" bytes, but "1040" bytes might be written.: Lines: 214, 217, 225, 226, 227, 229, 231, 232, 234, 235, 236, 237, 238, 239, 240, 241, 242, 243, 246, 247, 248, 250, 253, 257, 258, 260, 262, 263, 264, 265, 266, 267, 269, 272, 274, 275, 276, 277, 278, 279, 280, 281, 282, 283, 285, 286, 288, 289, 296, 298, 299, 300, 303, 305 (This one seems possible. "buffer" is "buffer_size" bytes, but then a DXVA_PicParams_H264 is copied into it. I assume that is 1040 bytes, so if "buffer_size" is less than 1040, it would be a problem.) media\gpu\dxva_video_decode_accelerator_win.cc(741) : warning C6230: Implicit cast between semantically different integer types: using HRESULT in a Boolean context. media\gpu\dxva_video_decode_accelerator_win.cc(751) : warning C6230: Implicit cast between semantically different integer types: using HRESULT in a Boolean context. media\gpu\dxva_video_decode_accelerator_win.cc(766) : warning C6230: Implicit cast between semantically different integer types: using HRESULT in a Boolean context. media\blink\multibuffer_data_source.cc(339) : warning C6297: Arithmetic overflow: 32-bit value is shifted, then cast to 64-bit value. Results might not be an expected value. (This is a size_t conversion problem. Probably should remove "no_size_t_to_int_warning" and fix all the things it complains about.) [1] https://build.chromium.org/p/chromium.fyi/builders/Chromium%20Windows%20Analyze/builds/4782/steps/compile/logs/stdio
,
Feb 1 2017
TL;DR - /analyze gives a lot of false positives. I wish we could tweak the heuristics. I'm confused by the first one. It seems like it doesn't know how long the similarity array is - the warning suggests that it is dynamically allocated. It looks bogus, but I'm slightly surprised it warned there. Did similarity used to be dynamically allocated? The second one (Using uninitialized memory "offset") is also disappointing. The initialization loop and usage loop have the same condition so it seems pretty pessimistic to assume that some elements might not be initialized. The third warning is a pet peeve of mine. When the operator in front of a ?: operator is '+' or '-' then humans get the precedence wrong. When it is '*', '/', or '%' humans get it right. So the warning is useless in this context. Parentheses would be harmless, but I'm not sure they are needed. The "buffer" overrun warning at least makes sense. /analyze is worried that the buffer isn't large enough, and at least this time it is a dynamically allocated buffer. As long as *you* know the size is sufficient you should ignore this. Warning C6230 is possibly valid. The "if (hr)" check is equivalent to "if (hr == S_OK)" and that is generally wrong - you should be going "if (SUCCEEDED(hr))" - but there are exceptions to this rule. The C6297 (shift) warnings are a bit of a pain. They trigger most often on size_t. They are correct, and arguably we should be having functions like CachedSize() return size_t to avoid the potential loss of bits. But, don't let /analyze bully you into doing this if it is clearly unneeded or messy.
,
Feb 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c88b8d3330e3b647b4e2b266bd9ffc53083e01e7 commit c88b8d3330e3b647b4e2b266bd9ffc53083e01e7 Author: jrummell <jrummell@chromium.org> Date: Thu Feb 02 22:15:09 2017 Add checked_cast to size_t conversion in multibuffer_data_source.cc /analyze on Windows complains about the size_t conversion, so add a checked_cast call to do the conversion. Memory usage is limited to 4Gb, so this should never fail. BUG= 687394 TEST=compiles Review-Url: https://codereview.chromium.org/2671613003 Cr-Commit-Position: refs/heads/master@{#447855} [modify] https://crrev.com/c88b8d3330e3b647b4e2b266bd9ffc53083e01e7/media/blink/multibuffer_data_source.cc
,
Feb 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5429b39dda87c47ace86635ab4cf998255ced5fb commit 5429b39dda87c47ace86635ab4cf998255ced5fb Author: jrummell <jrummell@chromium.org> Date: Fri Feb 03 04:13:12 2017 Check HRESULT using macros in DXVAVideoDecodeAccelerator /analyze on Windows complains about using HRESULT in a Boolean context, so check returned values using the appropriate macro. BUG= 687394 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2669083002 Cr-Commit-Position: refs/heads/master@{#447925} [modify] https://crrev.com/5429b39dda87c47ace86635ab4cf998255ced5fb/media/gpu/dxva_video_decode_accelerator_win.cc
,
Feb 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f0e9b0e1c113c2df05b2c6c71c6f4507d95a9415 commit f0e9b0e1c113c2df05b2c6c71c6f4507d95a9415 Author: mahmadi <mahmadi@chromium.org> Date: Fri Feb 03 14:32:58 2017 Revert of Check HRESULT using macros in DXVAVideoDecodeAccelerator (patchset #1 id:1 of https://codereview.chromium.org/2669083002/ ) Reason for revert: Causing failures in http/tests/media/media-source/mediasource-duration.html https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%29/builds/8902 22:44:47.228 2772 worker/0 http/tests/media/media-source/mediasource-duration.html output stderr lines: 22:44:47.228 2772 [4800:5628:0202/224446.112:3700733:ERROR:audio_manager_win.cc(414)] GetPreferredAudioParameters failed: 88890004 22:44:47.228 2772 [5600:2452:0202/224446.116:3700749:INFO:gpu_video_decode_accelerator_factory.cc(181)] Initializing DXVA HW decoder for windows. 22:44:47.228 2772 [5600:2452:0202/224446.116:3700749:ERROR:mf_helpers.cc(12)] Error in dxva_video_decode_accelerator_win.cc on line 593 22:44:47.228 2772 [5600:2452:0202/224446.117:3700749:ERROR:gpu_video_decode_accelerator.cc(375)] HW video decode not available for profile vp8 22:44:47.228 2772 [4800:5628:0202/224446.123:3700749:ERROR:audio_manager_win.cc(414)] GetPreferredAudioParameters failed: 88890004 22:44:47.228 2772 [4800:5628:0202/224446.123:3700749:ERROR:audio_manager_base.cc(257)] Invalid audio output parameters received; using fake audio path. Channels: 0, Sample Rate: 0, Bits Per Sample: 0, Frames Per Buffer: 0 22:44:47.228 2772 [2916:5884:0202/224446.171:3700796:ERROR:gpu_video_decode_accelerator_host.cc(99)] Send(GpuCommandBufferMsg_CreateVideoDecoder()) failed 22:44:47.228 2772 [5600:4620:0202/224446.172:3700796:ERROR:gpu_channel.cc(564)] Could not find message queue 22:44:47.228 2772 [4800:5628:0202/224446.207:3700827:WARNING:audio_sync_reader.cc(188)] AudioSyncReader::Read timed out, audio glitch count=1 22:44:47.228 2772 [5600:2452:0202/224446.222:3700842:INFO:gpu_video_decode_accelerator_factory.cc(181)] Initializing DXVA HW decoder for windows. 22:44:47.228 2772 [5600:2452:0202/224446.223:3700842:ERROR:mf_helpers.cc(12)] Error in dxva_video_decode_accelerator_win.cc on line 593 22:44:47.228 2772 [5600:2452:0202/224446.223:3700842:ERROR:gpu_video_decode_accelerator.cc(375)] HW video decode not available for profile vp8 22:44:47.228 2772 [2916:5884:0202/224446.224:3700842:ERROR:gpu_video_decode_accelerator_host.cc(99)] Send(GpuCommandBufferMsg_CreateVideoDecoder()) failed 22:44:47.228 2772 [5600:4620:0202/224446.225:3700842:ERROR:gpu_channel.cc(564)] Could not find message queue 22:44:47.228 2772 [4800:5628:0202/224446.263:3700889:WARNING:audio_sync_reader.cc(188)] AudioSyncReader::Read timed out, audio glitch count=2 22:44:47.228 2772 [2916:5884:0202/224446.383:3701014:ERROR:render_media_log.cc(30)] MediaEvent: MEDIA_ERROR_LOG_ENTRY {"error":"Failed to reconcile encoded audio times with decoded output."} 22:44:47.228 2772 [5600:2452:0202/224446.405:3701030:INFO:gpu_video_decode_accelerator_factory.cc(181)] Initializing DXVA HW decoder for windows. 22:44:47.228 2772 [5600:2452:0202/224446.406:3701030:ERROR:mf_helpers.cc(12)] Error in dxva_video_decode_accelerator_win.cc on line 593 22:44:47.228 2772 [5600:2452:0202/224446.407:3701030:ERROR:gpu_video_decode_accelerator.cc(375)] HW video decode not available for profile vp8 22:44:47.228 2772 [2916:5884:0202/224446.410:3701030:ERROR:gpu_video_decode_accelerator_host.cc(99)] Send(GpuCommandBufferMsg_CreateVideoDecoder()) failed 22:44:47.228 2772 [5600:4620:0202/224446.411:3701030:ERROR:gpu_channel.cc(564)] Could not find message queue 22:44:47.229 2772 [5600:2452:0202/224446.412:3701030:INFO:gpu_video_decode_accelerator_factory.cc(181)] Initializing DXVA HW decoder for windows. 22:44:47.229 2772 [5600:2452:0202/224446.412:3701030:ERROR:mf_helpers.cc(12)] Error in dxva_video_decode_accelerator_win.cc on line 593 22:44:47.229 2772 [5600:2452:0202/224446.413:3701045:ERROR:gpu_video_decode_accelerator.cc(375)] HW video decode not available for profile vp8 22:44:47.229 2772 [2916:5884:0202/224446.414:3701045:ERROR:gpu_video_decode_accelerator_host.cc(99)] Send(GpuCommandBufferMsg_CreateVideoDecoder()) failed 22:44:47.229 2772 [5600:4620:0202/224446.415:3701045:ERROR:gpu_channel.cc(564)] Could not find message queue 22:44:47.229 2772 [5600:2452:0202/224446.416:3701045:INFO:gpu_video_decode_accelerator_factory.cc(181)] Initializing DXVA HW decoder for windows. 22:44:47.229 2772 [5600:2452:0202/224446.417:3701045:ERROR:mf_helpers.cc(12)] Error in dxva_video_decode_accelerator_win.cc on line 593 22:44:47.229 2772 [5600:2452:0202/224446.417:3701045:ERROR:gpu_video_decode_accelerator.cc(375)] HW video decode not available for profile vp8 22:44:47.229 2772 [2916:5884:0202/224446.419:3701045:ERROR:gpu_video_decode_accelerator_host.cc(99)] Send(GpuCommandBufferMsg_CreateVideoDecoder()) failed 22:44:47.229 2772 [5600:4620:0202/224446.419:3701045:ERROR:gpu_channel.cc(564)] Could not find message queue 22:44:47.229 2772 [2916:5884:0202/224446.431:3701061:ERROR:render_media_log.cc(30)] MediaEvent: MEDIA_ERROR_LOG_ENTRY {"error":"Failed to reconcile encoded audio times with decoded output."} 22:44:47.229 2772 [2916:5884:0202/224446.574:3701201:ERROR:render_media_log.cc(30)] MediaEvent: MEDIA_ERROR_LOG_ENTRY {"error":"Failed to reconcile encoded audio times with decoded output."} 22:44:47.229 2772 [2916:5884:0202/224446.575:3701201:ERROR:render_media_log.cc(30)] MediaEvent: MEDIA_ERROR_LOG_ENTRY {"error":"Failed to reconcile encoded audio times with decoded output."} 22:44:47.229 2772 [2916:5884:0202/224446.598:3701217:ERROR:render_media_log.cc(30)] MediaEvent: MEDIA_ERROR_LOG_ENTRY {"error":"Failed to reconcile encoded audio times with decoded output."} Original issue's description: > Check HRESULT using macros in DXVAVideoDecodeAccelerator > > /analyze on Windows complains about using HRESULT in a Boolean context, > so check returned values using the appropriate macro. > > BUG= 687394 > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel > > Review-Url: https://codereview.chromium.org/2669083002 > Cr-Commit-Position: refs/heads/master@{#447925} > Committed: https://chromium.googlesource.com/chromium/src/+/5429b39dda87c47ace86635ab4cf998255ced5fb TBR=dalecurtis@chromium.org,jrummell@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 687394 Review-Url: https://codereview.chromium.org/2673023002 Cr-Commit-Position: refs/heads/master@{#447989} [modify] https://crrev.com/f0e9b0e1c113c2df05b2c6c71c6f4507d95a9415/media/gpu/dxva_video_decode_accelerator_win.cc
,
Feb 12 2018
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Feb 12 2018
Seems like this bot doesn't run anymore. Bruce, are you still running this anywhere?
,
Feb 12 2018
The bot does not run anymore. The HRESULT warnings may still be important, but even that warning sometimes has false positives. The other warnings are almost certainly false positives and should be ignored.
,
Feb 12 2018
Hmm, John tried to fix the HRESULT ones in https://codereview.chromium.org/2669083002 but it was reverted - they are still present in the code today. John do you remember what happened there?
,
Feb 13 2018
,
Mar 12 2018
For the HRESULT fix, the existing code is:
hr = video_processor_service_->do_something(...);
if (hr)
continue;
This is done for 3 different functions. Changing the test to FAILED(hr) causes problems, presumably due to 1 (or more) of the calls returning a positive value. I checked the MSDN documentation for all 3 calls, and basically all they say is "Possible values include, but are not limited to, those in the following table" followed by a table with just S_OK listed. Only 1 of the 3 pages (GetVideoProcessorCaps) contains example code, and it does include FAILED(hr). The other 2 don't have examples, although 1 (CreateVideoProcessor) does state "Otherwise, it returns an HRESULT error code", which could imply that FAILED(hr) is also the correct test.
However, given that it doesn't work with FAILED(hr) in all 3 places, it won't get rid of the /analyze complaints.
Closing this as "WontFix" as the /analyze bot doesn't run anymore, and these are now out of date.
|
||||
►
Sign in to add a comment |
||||
Comment 1 by dalecur...@chromium.org
, Feb 1 2017