Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
649023002:
Win Video Capture: Add device blacklisting, used for IPCamera and YouCam, collect UMA for those (Closed)
|
Created:
6 years, 2 months ago by mcasas Modified:
6 years, 2 months ago CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, wjia+watch_chromium.org, mcasas+watch_chromium.org, asvitkine+watch_chromium.org, perkj_chrome Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionWin Video Capture: Add device blacklisting, used for IPCamera and YouCam, collect UMA for those
BUG=422048, 421991
Committed: https://crrev.com/db1ccf962f7785f4c37edfce00ac3f5d685522d7
Cr-Commit-Position: refs/heads/master@{#299558}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Consolidated 'google camera adapter' into blacklist; other tommi@s comments #
Total comments: 3
Patch Set 3 : Removed VfW (Video For Windows) blacklisting. #
Total comments: 9
Patch Set 4 : asvitkine@ comments #
Messages
Total messages: 26 (7 generated)
mcasas@chromium.org changed reviewers: + asvitkine@chromium.org, tommi@chromium.org
tommi@: video_capture_device_factory_win.cc asvitkine@: histograms.xml
https://codereview.chromium.org/649023002/diff/1/media/video/capture/win/vide... File media/video/capture/win/video_capture_device_factory_win.cc (right): https://codereview.chromium.org/649023002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_factory_win.cc:51: {"IP Camera [JPEG/MJPEG]" }, nit: no space before } (for consistency) https://codereview.chromium.org/649023002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_factory_win.cc:52: {"CyberLink Webcam Splitter"} Can you point me to a report where we encountered this one? Do you know which progid or clsid this corresponds to? https://codereview.chromium.org/649023002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_factory_win.cc:112: if (name.find (kBlacklistedCameraNames[i]) != std::string::npos) { no space before ( What about StartsWith? (string_util.h) https://codereview.chromium.org/649023002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_factory_win.cc:166: static const char kGoogleCameraAdapter[] = "google camera adapter"; What about moving this into the blacklist now that we have one? We'd have to be careful to make sure it's not case sensitive. For now, if you could add a TODO here and ping perkj about the details, that would be good.
guys PTAL https://codereview.chromium.org/649023002/diff/1/media/video/capture/win/vide... File media/video/capture/win/video_capture_device_factory_win.cc (right): https://codereview.chromium.org/649023002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_factory_win.cc:51: {"IP Camera [JPEG/MJPEG]" }, On 2014/10/12 09:46:03, tommi wrote: > nit: no space before } (for consistency) Done. https://codereview.chromium.org/649023002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_factory_win.cc:52: {"CyberLink Webcam Splitter"} On 2014/10/12 09:46:03, tommi wrote: > Can you point me to a report where we encountered this one? Sure, there are a few in the other bug linked to this CL: hhtp://crbug.com/421991 > Do you know which progid or clsid this corresponds to? I'm afraid I don't ... :( https://codereview.chromium.org/649023002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_factory_win.cc:112: if (name.find (kBlacklistedCameraNames[i]) != std::string::npos) { On 2014/10/12 09:46:03, tommi wrote: > no space before ( > > What about StartsWith? (string_util.h) Done. https://codereview.chromium.org/649023002/diff/1/media/video/capture/win/vide... media/video/capture/win/video_capture_device_factory_win.cc:166: static const char kGoogleCameraAdapter[] = "google camera adapter"; On 2014/10/12 09:46:03, tommi wrote: > What about moving this into the blacklist now that we have one? > We'd have to be careful to make sure it's not case sensitive. > > For now, if you could add a TODO here and ping perkj about the details, that > would be good. Done, moved "google camera adapter" to the blacklist, and updated the comments, enum and histogram. I took your comment of using StartsWith(), which is used case-insensitively.
+perkj fyi on the google camera adapter. https://codereview.chromium.org/649023002/diff/90001/media/video/capture/win/... File media/video/capture/win/video_capture_device_factory_win.cc (right): https://codereview.chromium.org/649023002/diff/90001/media/video/capture/win/... media/video/capture/win/video_capture_device_factory_win.cc:168: if (wcsstr(str_ptr, L"(VFW)") != NULL) I think this check (and |str_ptr|) should also be removed since it's related to the check for the Google Camera Adapter.
On 2014/10/13 10:32:01, tommi wrote: > +perkj fyi on the google camera adapter. > > https://codereview.chromium.org/649023002/diff/90001/media/video/capture/win/... > File media/video/capture/win/video_capture_device_factory_win.cc (right): > > https://codereview.chromium.org/649023002/diff/90001/media/video/capture/win/... > media/video/capture/win/video_capture_device_factory_win.cc:168: if > (wcsstr(str_ptr, L"(VFW)") != NULL) > I think this check (and |str_ptr|) should also be removed since it's related to > the check for the Google Camera Adapter. lgtm with the comment^ addressed.
perkj@chromium.org changed reviewers: + perkj@chromium.org
lgtm https://codereview.chromium.org/649023002/diff/90001/media/video/capture/win/... File media/video/capture/win/video_capture_device_factory_win.cc (right): https://codereview.chromium.org/649023002/diff/90001/media/video/capture/win/... media/video/capture/win/video_capture_device_factory_win.cc:168: if (wcsstr(str_ptr, L"(VFW)") != NULL) On 2014/10/13 10:32:01, tommi wrote: > I think this check (and |str_ptr|) should also be removed since it's related to > the check for the Google Camera Adapter. I think there might be other VFW drivers we want to avoid so I think this is fine. This was done a loooong time ago though (before I joined Chrome) so I am not sure if it is still a problem or not.
On 2014/10/13 10:46:40, perkj wrote: > lgtm > > https://codereview.chromium.org/649023002/diff/90001/media/video/capture/win/... > File media/video/capture/win/video_capture_device_factory_win.cc (right): > > https://codereview.chromium.org/649023002/diff/90001/media/video/capture/win/... > media/video/capture/win/video_capture_device_factory_win.cc:168: if > (wcsstr(str_ptr, L"(VFW)") != NULL) > On 2014/10/13 10:32:01, tommi wrote: > > I think this check (and |str_ptr|) should also be removed since it's related > to > > the check for the Google Camera Adapter. > > I think there might be other VFW drivers we want to avoid so I think this is > fine. This was done a loooong time ago though (before I joined Chrome) so I am > not sure if it is still a problem or not. Should we then also track how many times this happens?
PTAL at the rationale, strong enough for me. https://codereview.chromium.org/649023002/diff/90001/media/video/capture/win/... File media/video/capture/win/video_capture_device_factory_win.cc (right): https://codereview.chromium.org/649023002/diff/90001/media/video/capture/win/... media/video/capture/win/video_capture_device_factory_win.cc:168: if (wcsstr(str_ptr, L"(VFW)") != NULL) On 2014/10/13 10:46:40, perkj wrote: > On 2014/10/13 10:32:01, tommi wrote: > > I think this check (and |str_ptr|) should also be removed since it's related > to > > the check for the Google Camera Adapter. > > I think there might be other VFW drivers we want to avoid so I think this is > fine. This was done a loooong time ago though (before I joined Chrome) so I am > not sure if it is still a problem or not. Removing VFW blacklisting. Rationale follows: Wikipedia on Video For Windows [1] says: "Video for Windows was mostly replaced by the July 1996 release of ActiveMovie, later known as DirectShow. [...] Video for Windows was still used for video capture until the release of Windows Driver Model capture drivers, which only started to become popular in 2000." VfW drivers are supported via Direct Show VfW drivers [2]; such are not enumerated since they have CLSID_VfwCapture and we don't use such CLSID [3,4]. [1] http://en.wikipedia.org/wiki/Video_for_Windows [2] http://msdn.microsoft.com/en-us/library/windows/desktop/dd407319(v=vs.85).aspx [3] https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur... [4] https://code.google.com/p/chromium/codesearch#chromium/src/media/video/captur...
fine by me.
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/649023002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
LGTM % comments, thanks! https://codereview.chromium.org/649023002/diff/200001/media/video/capture/win... File media/video/capture/win/video_capture_device_factory_win.cc (right): https://codereview.chromium.org/649023002/diff/200001/media/video/capture/win... media/video/capture/win/video_capture_device_factory_win.cc:34: enum BlacklistedCameraNames{ Nit: Space before {. https://codereview.chromium.org/649023002/diff/200001/media/video/capture/win... media/video/capture/win/video_capture_device_factory_win.cc:52: // This prefix is used case-insensitively. Mention that this should be updated in sync with |BlacklistedCameraNames| and vice versa in the comment on that one. Maybe put them next to each other. https://codereview.chromium.org/649023002/diff/200001/media/video/capture/win... media/video/capture/win/video_capture_device_factory_win.cc:116: BLACKLISTED_CAMERA_MAX); Nit: Expected value should be the first arg, so swap them. https://codereview.chromium.org/649023002/diff/200001/media/video/capture/win... media/video/capture/win/video_capture_device_factory_win.cc:121: i, BLACKLISTED_CAMERA_MAX + 1); Remove "+ 1" from the last param, since it's already 1 greater than the maximum value of |i|.
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/649023002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by mcasas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/649023002/400001
Message was sent while issue was closed.
Committed patchset #4 (id:400001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/db1ccf962f7785f4c37edfce00ac3f5d685522d7 Cr-Commit-Position: refs/heads/master@{#299558}
Message was sent while issue was closed.
https://codereview.chromium.org/649023002/diff/200001/media/video/capture/win... File media/video/capture/win/video_capture_device_factory_win.cc (right): https://codereview.chromium.org/649023002/diff/200001/media/video/capture/win... media/video/capture/win/video_capture_device_factory_win.cc:34: enum BlacklistedCameraNames{ On 2014/10/14 15:18:45, Alexei Svitkine wrote: > Nit: Space before {. Done. https://codereview.chromium.org/649023002/diff/200001/media/video/capture/win... media/video/capture/win/video_capture_device_factory_win.cc:52: // This prefix is used case-insensitively. On 2014/10/14 15:18:45, Alexei Svitkine wrote: > Mention that this should be updated in sync with |BlacklistedCameraNames| and > vice versa in the comment on that one. Done. > Maybe put them next to each other. Don't think is needed, in any case there's the DCHECK in l.115. https://codereview.chromium.org/649023002/diff/200001/media/video/capture/win... media/video/capture/win/video_capture_device_factory_win.cc:116: BLACKLISTED_CAMERA_MAX); On 2014/10/14 15:18:45, Alexei Svitkine wrote: > Nit: Expected value should be the first arg, so swap them. Done. https://codereview.chromium.org/649023002/diff/200001/media/video/capture/win... media/video/capture/win/video_capture_device_factory_win.cc:121: i, BLACKLISTED_CAMERA_MAX + 1); On 2014/10/14 15:18:45, Alexei Svitkine wrote: > Remove "+ 1" from the last param, since it's already 1 greater than the maximum > value of |i|. Presubmit.py complained: ** Presubmit ERRORS ** UMA_HISTOGRAM_ENUMERATION reports in src/media/ are expected to adhere to the following guidelines: - The max value (3rd argument) should be an enum value equal to the last valid value, e.g. FOO_MAX = LAST_VALID_FOO. - 1 must be added to that max value. Contact rileya@chromium.org if you have questions. So I followed its instructions.
Message was sent while issue was closed.
https://codereview.chromium.org/649023002/diff/200001/media/video/capture/win... File media/video/capture/win/video_capture_device_factory_win.cc (right): https://codereview.chromium.org/649023002/diff/200001/media/video/capture/win... media/video/capture/win/video_capture_device_factory_win.cc:121: i, BLACKLISTED_CAMERA_MAX + 1); On 2014/10/15 07:25:09, mcasas wrote: > On 2014/10/14 15:18:45, Alexei Svitkine wrote: > > Remove "+ 1" from the last param, since it's already 1 greater than the > maximum > > value of |i|. > > Presubmit.py complained: > > ** Presubmit ERRORS ** > UMA_HISTOGRAM_ENUMERATION reports in src/media/ are expected to adhere > to the following guidelines: > - The max value (3rd argument) should be an enum value equal to the > last valid value, e.g. FOO_MAX = LAST_VALID_FOO. > - 1 must be added to that max value. > Contact mailto:rileya@chromium.org if you have questions. > > So I followed its instructions. Sigh, it's because media code expects the MAX value to be the last value, not +1 from the last value. So if you follow the presubmit, you'd want BLACKLISTED_CAMERA_MAX = BLACKLISTED_CAMERA_CYBERLINK_WEBCAM_SPLITTER I guess it doesn't matter either way - just results in an extra unused bucket being allocated, but no effect on data sent. (FWIW, I'm not a fan of the media/ code having a different convention than the rest of Chromium, but I guess it's hard to change at this point. :\) 38 BLACKLISTED_CAMERA_MAX |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
