Re: [FFmpeg-devel] [PATCH v4] decklink: Add support for compressed AC-3 output over SDI

[Devin Heitmueller <devin.heitmueller@ltnglobal.com>]

Hello Marton,

Thanks for the continued feedback.  Comments inline.

On Wed, Apr 5, 2023 at 5:52 PM Marton Balint <cus@passwd.hu> wrote:
> > --- a/libavdevice/decklink_enc.cpp
> > +++ b/libavdevice/decklink_enc.cpp
> > +/* Wrap the AC-3 packet into an S337 payload that is in S16LE format which can be easily
> > +   injected into the PCM stream.  Note: despite the function name, only AC-3 is implemented */
> > +static int create_s337_payload(AVPacket *pkt, enum AVCodecID codec_id, uint8_t **outbuf, int *outsize)
>
> Actually you can remove the codec_id parameter as well...

Ok.

> > +{
> > +    // Note: if the packet is an odd-number of bytes, we need to make
> > +    // the actual payload one byte larger to ensure it ends on an S16LE boundary
> > +    int payload_size = pkt->size + (pkt->size % 2) + 8;
>
> FFALIGN(pkt->size, 2). But you'd want FFALIGN(pkt->size, 4) because you
> want the buffer size to be divisable by 4 because later decklink needs a
> sample count...

Ok.

> > +    uint16_t bitcount = pkt->size * 8;
>
> Is this supposed to be aligned too? I see similar code in
> libavformat/spdifenc.c and FFALIGN(pkt->size, 2) << 3 is used there.

I reviewed SMPTE ST337:2015 as well as ST338:2016, and I think this
might actually be a mistake in spdifenc.c.  There's nothing to suggest
a hard requirement that the payload be aligned on a two byte boundary,
and in fact I suspect it would cause checksum failures in certain
codecs given the checksums are often at the end of the packet payload
(and adding a padding byte would cause the checksum field itself to be
in the wrong position relative to the end of the packet).

Now in practice I suspect you wouldn't likely find packets that aren't
aligned on a two-byte boundary, simply because of the nature of the
codecs used (e.g. AC-3 packets are always 1536 bytes).  This would
certainly explain how the logic in spdifenc.c is incorrect but it
never causes any failures in real-world use.

I'm inclined to leave the logic as-is, unless somebody can offer a
good counter argument.

> > +    if (pkt->size % 2)
>
> pkt->size & 1

Ok.

> > +        bytestream2_put_le16u(&pb, pkt->data[pkt->size - 1] << 8);
> > +
>
> And you likely want a bytestream2_put_le16(&pb, 0) in the end so even
> the end of the 4-byte aligned buffer is properly zeroed.

Ok.

I will submit an updated patch reflecting the changes above.

Devin

-- 
Devin Heitmueller, Senior Software Engineer
LTN Global Communications
o: +1 (301) 363-1001
w: https://ltnglobal.com  e: devin.heitmueller@ltnglobal.com
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".