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

[Janne Grunau]

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.

> >>
> >>  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.

Janne