[FFmpeg-devel] [PATCH 2/2] spdifenc: fix byte order on big-endian systems

[Anssi Hannula]

On 11.02.2011 10:45, Janne Grunau wrote:
> On Fri, Feb 11, 2011 at 05:24:02AM +0200, Anssi Hannula wrote:
>> On 10.02.2011 23:46, Janne Grunau wrote:
>>> On Fri, Jan 21, 2011 at 08:33:00PM +0200, Anssi Hannula wrote:
>>>> There is a check for HAVE_BIGENDIAN when outputting the IEC 61937
>>>> stream. On big-endian systems the payload data is not byteswapped,
>>>> causing in effect the outputted payload data to be in a different byte
>>>> order on big-endian than on little-endian systems.
>>>>
>>>> However, the IEC 61937 preamble (and the final odd byte if present) is
>>>> always outputted in the same byte order. This means that on big-endian
>>>> systems the headers have a different byte order than the payload,
>>>> preventing useful use of the output.
>>>>
>>>> Fix that by outputting the data in a format suitable for sending to an
>>>> audio device in S16LE format by default. Output as big-endian (S16BE)
>>>> is added as an AVOption. This makes the muxer output the same on all
>>>> archs by default.
>>>>
>>>> ---
>>>>
>>>> Other ways to fix this would be to
>>>> a) simply always output in little-endian format, or
>>>> b) always output in native-endian format (i.e. different muxer output
>>>> depending on arch), or
>>>> c) have two different logical muxers.
>>>
>>> I think I prefer this solution.
>>
>> OK. I slightly prefer the AVOption to avoid small things like "ffmpeg
>> -help" listing the same AVOptions twice for both logical muxers.
>>
>> However, I don't have a strong opinion on this so I can make it two
>> logical muxers, then (unless other people chime in).
> 
> stop, with this I meant the AVOption solution in your patch. Sorry for
> not making this clear.

Ah, OK :)

>>>>
>>>>  libavformat/spdifenc.c |   26 ++++++++++++++++++++------
>>>>  1 files changed, 20 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/libavformat/spdifenc.c b/libavformat/spdifenc.c
>>>> index 8e35190..f85be8a 100644
>>>> --- a/libavformat/spdifenc.c
>>>> +++ b/libavformat/spdifenc.c
>>>> @@ -76,6 +76,8 @@ typedef struct IEC61937Context {
>>>>      /* AVOptions: */
>>>>      int dtshd_rate;
>>>>      int dtshd_fallback;
>>>> +#define SPDIF_FLAG_BIGENDIAN    0x01
>>>> +    int spdif_flags;
>>>>  
>>>>      /// function, which generates codec dependent header information.
>>>>      /// Sets data_type and pkt_offset, and length_code, out_bytes, out_buf if necessary
>>>> @@ -83,6 +85,8 @@ typedef struct IEC61937Context {
>>>>  } IEC61937Context;
>>>>  
>>>>  static const AVOption options[] = {
>>>> +{ "spdif_flags", "IEC 61937 encapsulation flags", offsetof(IEC61937Context, spdif_flags), FF_OPT_TYPE_FLAGS, 0, 0, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "spdif_flags" },
>>>> +{ "be", "output in big-endian format (for use as s16be)", 0, FF_OPT_TYPE_CONST, SPDIF_FLAG_BIGENDIAN, 0, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM, "spdif_flags" },
>>>>  { "dtshd_rate", "mux complete DTS frames in HD mode at the specified IEC958 rate (in Hz, default 0=disabled)", offsetof(IEC61937Context, dtshd_rate), FF_OPT_TYPE_INT, 0, 0, 768000, AV_OPT_FLAG_ENCODING_PARAM },
>>>>  { "dtshd_fallback_time", "min secs to strip HD for after an overflow (-1: till the end, default 60)", offsetof(IEC61937Context, dtshd_fallback), FF_OPT_TYPE_INT, 60, -1, INT_MAX, AV_OPT_FLAG_ENCODING_PARAM },
>>>>  { NULL },
>>>> @@ -476,6 +480,15 @@ static int spdif_write_trailer(AVFormatContext *s)
>>>>      return 0;
>>>>  }
>>>>  
>>>> +static void spdif_put_16(struct AVFormatContext *s, unsigned int val)
>>>> +{
>>>
>>> av_always_inline
>>
>> OK.
>>
>>>> +    IEC61937Context *ctx = s->priv_data;
>>>
>>> just pass the IEC61937Context pointer as argument
>>
>> OK, though then I'll have to pass ByteIOContext* as well.
>>
>>>> +    if (ctx->spdif_flags & SPDIF_FLAG_BIGENDIAN)
>>>> +        put_be16(s->pb, val);
>>>> +    else
>>>> +        put_le16(s->pb, val);
>>>> +}
>>>
>>> I'm not sure if this is the cleanest solution. factoring the data writing
>>> parts into spdif_write_packet_data_le|be might be nicer. Has anyone else
>>> an opinion?
>>
>> Wouldn't it cause some unnecessary code duplication?
> 
> I thought the 'if (ctx->extra_bswap) ... else ...' could be splitted,
> i.e. the code under the if goes to one function, the else to the other.
> forget my suggestion. It looks a little bit fishy though since
> spdif_write_packet sets extra_bswap unconditionally to 0.

Well, it inits it to 0 but spdif_header_dts() sets it to 1 when needed.

-- 
Anssi Hannula