[FFmpeg-devel] [PATCH 3/3] avdevice/decklink: Add support for decoding 8-bit RGB formats.

[Carl Eugen Hoyos]

Chris Spencer <spencercw <at> gmail.com> writes:

> > > -On Windows, you need to run the IDL files 
> > > through  <at> command{widl}.
> > > +On Windows, you need to run the IDL files 
> > > through  <at> command{midl}.
> >
> > Is this intended?
> 
> Yes, widl doesn't exist. The tool is called midl.

Then please send a separate patch.

> > > -DeckLink is very picky about the formats it supports. Pixel format is
> > > +DeckLink is very picky about the formats it supports. Pixel
> > > format is uyvy422,
> >
> > Imo, please do not change this line:
> > Going through old commits is not unusual,
> > such changes make this much more difficult.
> 
> So I'm clear, you mean the text is fine, but 
> avoid changing the position of the line breaks?

I am not a native speaker (so I cannot really 
comment on the actual text) but yes, I have no 
objections.

> > > +    } else if (cctx->pixel_format == AV_PIX_FMT_ARGB) {
> > > +        ctx->bmd_format = bmdFormat8BitARGB;
> > > +    } else if (cctx->pixel_format == AV_PIX_FMT_BGRA) {
> > > +        ctx->bmd_format = bmdFormat8BitBGRA;
> >
> > > +    } else if (ctx->bmd_format == bmdFormat8BitARGB) {
> > > +        st->codec->codec_id    = AV_CODEC_ID_RAWVIDEO;
> > > +        st->codec->pix_fmt     = AV_PIX_FMT_ARGB;
> > > +        st->codec->codec_tag   = MKTAG('A', 'R', 'G', 'B');
> > > +    } else if (ctx->bmd_format == bmdFormat8BitBGRA) {
> > > +        st->codec->codec_id    = AV_CODEC_ID_RAWVIDEO;
> > > +        st->codec->pix_fmt     = AV_PIX_FMT_BGRA;
> > > +        st->codec->codec_tag   = MKTAG('B', 'G', 'R', 'A');
> >
> > I would have expected these to be AV_PIX_FMT_0RGB and
> > AV_PIX_FMT_BGR0.
> 
> The Blackmagic documentation is kind of strange on this 
> one actually. For bmdFormat8BitBGRA it says "alpha 
> channel is valid". For bmdFormat8BitBGRA it says "alpha 
> channel may be valid". Not sure how best to deal with 
> that to be honest.

Can you test?
Is there a theoretical possibility that the content 
contains valid alpha? If yes, please use ARGB.
If not and if you can confirm that the alpha channel 
is correctly set to 0xFF, then you may use ARGB but I 
wonder what the usecase would be (and how probable it 
is that a future driver would not set it to 0xFF).
The codec_tag looks wrong to me because it would be 
0x00 for AV_PIX_FMT_ARGB and AV_PIX_FMT_BGRA. I think 
it should only be set if needed (for YV12 or similar).

> > > +    { "pixel_format", "set video pixel format"               ,
> >
> > Should be pix_fmt imo.
> 
> I was going to do this, but avfoundation, dshow and 
> v4l have similar options all called pixel_format, 
> so I used that for consistency.

Thank you for reminding me!

Carl Eugen