Issue 1806883004: Use the encoded video mode for capture setup of decklink devices.

Created:
4 years, 9 months ago by jensf

Modified:
4 years, 7 months ago

Reviewers:
perkj_chrome, mcasas, tommi (sloooow) - chröme

CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, mcasas+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Use the encoded video mode for capture setup of decklink devices. The user selects a combination of vide device name and video mode containing the resolution, framerate and interlaced/progressive information. I changed the capture setup routine to use device-videomode combination instead of the given constraints in the parameters. In the parameters, the i/p information is not available, so for a video signal of e.g. 1080p50 the capture was setup with 1080i50 which showed a black screen only. fixed selection of video mode for decklink capture devices in getUserMedia. BUG=603027

Patch Set 1 #

Total comments: 8

Patch Set 2 : update code style #

Total comments: 4

Patch Set 3 : update code style #

Patch Set 4 : updated code style the next #

Created: 4 years, 8 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+74 lines, -53 lines)			Patch
	M	media/capture/video/mac/video_capture_device_decklink_mac.mm	View	1 2 3	7 chunks	+74 lines, -53 lines	0 comments	Download

Messages

Total messages: 17 (5 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

jensf

I've made a change to the decklink media capture for mac such that the video ...

4 years, 8 months ago (2016-04-06 10:33:57 UTC) #2

davve

On 2016/04/06 10:33:57, jensf wrote: > Would you have a look on this and see ...

4 years, 8 months ago (2016-04-06 12:46:55 UTC) #3

mcasas

A few comments. I'm also wary that you're changing this functionality completely - would there ...

4 years, 8 months ago (2016-04-12 17:54:42 UTC) #8

jensf

On 2016/04/12 17:54:42, mcasas wrote: > A few comments. I'm also wary that you're > ...

4 years, 8 months ago (2016-04-13 14:33:38 UTC) #10

mcasas

On 2016/04/13 14:33:38, jensf wrote: > On 2016/04/12 17:54:42, mcasas wrote: > > A few ...

4 years, 8 months ago (2016-04-19 21:53:24 UTC) #11

On 2016/04/13 14:33:38, jensf wrote:
> On 2016/04/12 17:54:42, mcasas wrote:
> > A few comments. I'm also wary that you're
> > changing this functionality completely - 
> > would there be a way to have a fallback to
> > the existing ToT behaviour if the names do
> > not include resolution&frame rates? 
> 
> The device names are generated in the same
> file (VideoCaptureDeviceDeckLinkMac::EnumerateDevices see
> device_names->push_back(..) )  with the corresponding video format - so
> restoring
> the old behavior without modification does not make much sense.
> 
> It may be possible to rewrite the device detection to only include the device
> name and on activation to try to find the video mode from the given
> constraints. This will be a much larger change and I'm not aware of any
> constraint for the progressive/interlaced format selection.
> 
> > Also, this CL needs a BUG= and a TEST=
> > detailing under which conditions is this
> > tested and what issues are being addressed.
> 
> Just filed a bug report. Don't know how to get a TEST= ?
> 

Just write in a TEST= line what you did to test
that this was working, by hand or otherwise.

> Updated the code style as you mentioned and submitted a new patch.
> 
> >
>
https://codereview.chromium.org/1806883004/diff/1/media/capture/video/mac/vid...
> > media/capture/video/mac/video_capture_device_decklink_mac.mm:157:
DVLOG_IF(1,
> hr
> > != S_OK) << "Error reading Blackmagic device model name";
> > Consider DLOG_IF(ERROR,...
> 
> I just used the logging commands which were present before. Have not looked up
> the macro here to avoid the magic numbers.
> 
> Kind regards Jens

It's customary to click on every comment and
mark it as "Done" or comment on it, then click
on "Publish and mail comments" and write PTAL,
so reviewers get it in their email inbox.

I have a question on the overall approach. IIRC,
the Blackmagic/Decklink devices I tested would
only work with a certain resolution+framerate 
(a mode) that is marked by the external device. 
The enumeration would display (many) more modes 
which would not work. Am I right to understand
that with this CL any other mode would also work,
beside the one provided by the external capture
device? Or is the goal to address just the
interlaced/progressive issue mentioned in the
description?

mcasas

https://codereview.chromium.org/1806883004/diff/20001/media/capture/video/mac/video_capture_device_decklink_mac.mm File media/capture/video/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/1806883004/diff/20001/media/capture/video/mac/video_capture_device_decklink_mac.mm#newcode155 media/capture/video/mac/video_capture_device_decklink_mac.mm:155: DVLOG_IF(1, hr != S_OK) << "Error reading Blackmagic device ...

4 years, 8 months ago (2016-04-19 21:53:33 UTC) #12

jensf

all code style problems should be solved now https://codereview.chromium.org/1806883004/diff/1/media/capture/video/mac/video_capture_device_decklink_mac.mm File media/capture/video/mac/video_capture_device_decklink_mac.mm (right): https://codereview.chromium.org/1806883004/diff/1/media/capture/video/mac/video_capture_device_decklink_mac.mm#newcode17 media/capture/video/mac/video_capture_device_decklink_mac.mm:17: static ...

4 years, 8 months ago (2016-04-21 07:29:47 UTC) #13

jensf

> It's customary to click on every comment and > mark it as "Done" or ...

4 years, 8 months ago (2016-04-21 07:50:52 UTC) #14

mcasas

On 2016/04/21 07:50:52, jensf wrote: > > It's customary to click on every comment and ...

4 years, 8 months ago (2016-04-21 15:06:57 UTC) #15

On 2016/04/21 07:50:52, jensf wrote:
> > It's customary to click on every comment and
> > mark it as "Done" or comment on it, then click
> > on "Publish and mail comments" and write PTAL,
> > so reviewers get it in their email inbox.
> 
> Sorry this is my first contribution to Chromium. Thanks 
> for your patience.
>

No worries!
 
> 
> > I have a question on the overall approach. IIRC,
> > the Blackmagic/Decklink devices I tested would
> > only work with a certain resolution+framerate 
> > (a mode) that is marked by the external device. 
> > The enumeration would display (many) more modes 
> > which would not work. Am I right to understand
> > that with this CL any other mode would also work,
> > beside the one provided by the external capture
> > device? Or is the goal to address just the
> > interlaced/progressive issue mentioned in the
> > description?
> 
> 
> Your right the devices deliver exactly that mode which is connected to the
> input. There is no way to specify any conversion. If the wrong mode is
selected
> the driver returns a black frame. (Even worse with some HDMI signals the
> colorspace is some RGBA and not YUV any more and you have to select and handle
> the incoming data differently, unfortunately this is not addressed in my
> approach)
> 
> With recent SDKs and drivers some of the devices from Blackmagic does support
> auto-detection of the connected signals. For now my approach was to let the
user
> exactly set the mode which is connected because this will work well for all
> devices.
> 
> For a more sophisticated  approach one has to utilize the auto-detection on
> devices which supports that and some other method to make the other devices do
> work.

I see. Then I have a suggestion for an alternative 
implementation:

Extend JoinDeviceAndFormat() to add an indication of the
scanning method (progressive or interlaced), possibly in
between the name and the format name around l.371 [1].
Then fill in that field with a stringified indication of the 
said scanning mode that can be read from |display_mode|
by calling GetFieldDominance() [2]. This is for enumeration,
then on AllocateAndStart() also take that scanning mode
into account. WDYT?


[1]
https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/vide...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/deckli...

mcasas

On 2016/04/21 15:06:57, mcasas wrote: > On 2016/04/21 07:50:52, jensf wrote: > > > It's ...

4 years, 7 months ago (2016-04-27 02:36:05 UTC) #16

On 2016/04/21 15:06:57, mcasas wrote:
> On 2016/04/21 07:50:52, jensf wrote:
> > > It's customary to click on every comment and
> > > mark it as "Done" or comment on it, then click
> > > on "Publish and mail comments" and write PTAL,
> > > so reviewers get it in their email inbox.
> > 
> > Sorry this is my first contribution to Chromium. Thanks 
> > for your patience.
> >
> 
> No worries!
>  
> > 
> > > I have a question on the overall approach. IIRC,
> > > the Blackmagic/Decklink devices I tested would
> > > only work with a certain resolution+framerate 
> > > (a mode) that is marked by the external device. 
> > > The enumeration would display (many) more modes 
> > > which would not work. Am I right to understand
> > > that with this CL any other mode would also work,
> > > beside the one provided by the external capture
> > > device? Or is the goal to address just the
> > > interlaced/progressive issue mentioned in the
> > > description?
> > 
> > 
> > Your right the devices deliver exactly that mode which is connected to the
> > input. There is no way to specify any conversion. If the wrong mode is
> selected
> > the driver returns a black frame. (Even worse with some HDMI signals the
> > colorspace is some RGBA and not YUV any more and you have to select and
handle
> > the incoming data differently, unfortunately this is not addressed in my
> > approach)
> > 
> > With recent SDKs and drivers some of the devices from Blackmagic does
support
> > auto-detection of the connected signals. For now my approach was to let the
> user
> > exactly set the mode which is connected because this will work well for all
> > devices.
> > 
> > For a more sophisticated  approach one has to utilize the auto-detection on
> > devices which supports that and some other method to make the other devices
do
> > work.
> 
> I see. Then I have a suggestion for an alternative 
> implementation:
> 
> Extend JoinDeviceAndFormat() to add an indication of the
> scanning method (progressive or interlaced), possibly in
> between the name and the format name around l.371 [1].
> Then fill in that field with a stringified indication of the 
> said scanning mode that can be read from |display_mode|
> by calling GetFieldDominance() [2]. This is for enumeration,
> then on AllocateAndStart() also take that scanning mode
> into account. WDYT?
> 
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/vide...
> [2]
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/deckli...

And?

mcasas

4 years, 7 months ago (2016-05-23 17:26:45 UTC) #17

On 2016/04/27 02:36:05, mcasas wrote:
> On 2016/04/21 15:06:57, mcasas wrote:
> > On 2016/04/21 07:50:52, jensf wrote:
> > > > It's customary to click on every comment and
> > > > mark it as "Done" or comment on it, then click
> > > > on "Publish and mail comments" and write PTAL,
> > > > so reviewers get it in their email inbox.
> > > 
> > > Sorry this is my first contribution to Chromium. Thanks 
> > > for your patience.
> > >
> > 
> > No worries!
> >  
> > > 
> > > > I have a question on the overall approach. IIRC,
> > > > the Blackmagic/Decklink devices I tested would
> > > > only work with a certain resolution+framerate 
> > > > (a mode) that is marked by the external device. 
> > > > The enumeration would display (many) more modes 
> > > > which would not work. Am I right to understand
> > > > that with this CL any other mode would also work,
> > > > beside the one provided by the external capture
> > > > device? Or is the goal to address just the
> > > > interlaced/progressive issue mentioned in the
> > > > description?
> > > 
> > > 
> > > Your right the devices deliver exactly that mode which is connected to the
> > > input. There is no way to specify any conversion. If the wrong mode is
> > selected
> > > the driver returns a black frame. (Even worse with some HDMI signals the
> > > colorspace is some RGBA and not YUV any more and you have to select and
> handle
> > > the incoming data differently, unfortunately this is not addressed in my
> > > approach)
> > > 
> > > With recent SDKs and drivers some of the devices from Blackmagic does
> support
> > > auto-detection of the connected signals. For now my approach was to let
the
> > user
> > > exactly set the mode which is connected because this will work well for
all
> > > devices.
> > > 
> > > For a more sophisticated  approach one has to utilize the auto-detection
on
> > > devices which supports that and some other method to make the other
devices
> do
> > > work.
> > 
> > I see. Then I have a suggestion for an alternative 
> > implementation:
> > 
> > Extend JoinDeviceAndFormat() to add an indication of the
> > scanning method (progressive or interlaced), possibly in
> > between the name and the format name around l.371 [1].
> > Then fill in that field with a stringified indication of the 
> > said scanning mode that can be read from |display_mode|
> > by calling GetFieldDominance() [2]. This is for enumeration,
> > then on AllocateAndStart() also take that scanning mode
> > into account. WDYT?
> > 
> > 
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/capture/vide...
> > [2]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/deckli...
> 
> And?

Ping.

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages