[FFmpeg-devel] [PATCH] rmdec.c: implement RMVideo/AudioStream

[Michael Niedermayer]

On Sun, Dec 14, 2008 at 01:49:07PM -0500, Ronald S. Bultje wrote:
> Hi,
> 
> as per $subject, this patch implements RMVideoStream and
> RMAudioStream, so that a single RM file can be demuxed, even if it
> contains multiple video or audio streams. RM files from
> samples.mphq.hu that used to play, still play.
> 
> This breaks RDT support, because I'm not actually creating new
> AVStreams for the RM demuxer context in the RDT parser, but I rather
> reuse the ones from the RTSP demuxer, where priv_data points to
> RTSPStream instead of RMVideo/AudioStream. This causes crashes etc,
> and is bad design of my side. I'll fix the RDT parser in another patch
> by creating a new AVStream for each stream in the RM demuxer context
> and using these (instead of the RTSP demuxer AVStreams) in calls to
> ff_rm_*(). I will apply both right after each other when approved (and
> if others think this is OK).

why video and audio and not a single struct?

[...]

> -    int curpic_num;    ///< picture number of current frame
> +    int curpic_num;    ///< picture number of current frame;

ehm


>      int cur_slice, slices;
> -    int64_t pktpos;    ///< first slice position in file
> -    /// Audio descrambling matrix parameters
> +    int64_t pktpos;    ///< first slice position in file;
> +} RMVideoStream;
> +
> +typedef struct {
> +    ///Audio descrambling matrix parameters
>      uint8_t *audiobuf; ///< place to store reordered audio data

> -    int64_t audiotimestamp; ///< Audio packet timestamp
> +    uint64_t audiotimestamp; ///< Audio packet timestamp

does not belong in this patch


>      int sub_packet_cnt; // Subpacket counter, used while reading
>      int sub_packet_size, sub_packet_h, coded_framesize; ///< Descrambling parameters from container
> +    int audio_framesize; /// Audio frame size from container
> +    int sub_packet_lengths[16]; /// Length of each subpacket
> +} RMAudioStream;
> +
> +typedef struct {
> +    int nb_packets;
> +    int old_format;
> +    int current_stream;
> +    int remaining_len;
>      int audio_stream_num; ///< Stream number for audio packets
>      int audio_pkt_cnt; ///< Output packet counter
> -    int audio_framesize; /// Audio frame size from container
> -    int sub_packet_lengths[16]; /// Length of each aac subpacket
>  } RMDemuxContext;
>  
>  static inline void get_strl(ByteIOContext *pb, char *buf, int buf_size, int len)
> @@ -71,10 +77,12 @@
>  static int rm_read_audio_stream_info(AVFormatContext *s, ByteIOContext *pb,
>                                       AVStream *st, int read_all)
>  {
> -    RMDemuxContext *rm = s->priv_data;
> +    RMAudioStream *ast;
>      char buf[256];
>      uint32_t version;
>  
> +    st->priv_data = ast = av_mallocz(sizeof(RMAudioStream));

i do not like this, having priv_data being 2 different structs depending on
codec_type needs very strong justification and a full review of the whole
source tree to ensure nothing can flip the type
also would require to be documented in the public API and, and and
basically, just dont do this please

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081214/3f4d939f/attachment.pgp>