[FFmpeg-devel] [PATCH] Add support for demuxing the combined TrueHD/AC3 tracks in Blu-Ray m2ts files

[Baptiste Coudurier]

Hi,

On Tue, Sep 29, 2009 at 10:41:30PM +0100, Ian Caulfield wrote:
> 2009/9/29 Baptiste Coudurier <baptiste.coudurier at gmail.com>:
> > On 9/29/09 2:30 AM, Ian Caulfield wrote:
> >>
> >> This patch allows the combined TrueHD/AC3 tracks on Blu-Ray discs to be
> >> demuxed as two separate streams.
> >>
> >> Updated version that doesn't fudge the stream id of the AC3 track - both
> >> ?streams will have the PID as the stream id.
> >>
> >
> > I also find this very ugly, but I guess we do not have any other way of
> > doing it.
> >
> > I'm fine with the patch, though I think I like better sub_st and sub_pes
> > instead of st2 and pes2, if you are ok with that.
> 
> Yeah - adding 2 was just the first thing that came to mind :)
> 
> >> [...]
> >>
> >>
> >> @@ -584,6 +587,27 @@ static AVStream *new_pes_av_stream(PESContext *pes,
> >> uint32_t prog_reg_desc, uint
> >> ? ? ? ? ?}
> >> ? ? ?}
> >>
> >> + ? ?// HDMV TrueHD streams also contain an AC3 coded version of the
> >> + ? ?// audio track - add a second stream for this
> >> + ? ?if (prog_reg_desc == AV_RL32("HDMV")&& ?pes->stream_type == 0x83) {
> >> + ? ? ? ?// priv_data cannot be shered between streams
> >> + ? ? ? ?PESContext *pes2 = av_malloc(sizeof(PESContext));
> >> + ? ? ? ?memcpy(pes2, pes, sizeof(PESContext));
> >
> > sizeof(*pes2);
> >
> > And put the code under the first if (prog_reg_desc == AV_RL32("HDMV"))
> 
> Fixed
> 
> >> + ? ? ? ?if (pes2) {
> >> + ? ? ? ? ? ?AVStream *st2 = av_new_stream(pes->stream, pes2->pid);
> >> +
> >
> > pes->pid seems more logicical
> >
> >> + ? ? ? ? ? ?if (st2) {
> >> + ? ? ? ? ? ? ? ?av_set_pts_info(st2, 33, 1, 90000);
> >> + ? ? ? ? ? ? ? ?st2->priv_data = pes2;
> >> + ? ? ? ? ? ? ? ?st2->codec->codec_type = CODEC_TYPE_AUDIO;
> >> + ? ? ? ? ? ? ? ?st2->codec->codec_id ? = CODEC_ID_AC3;
> >> + ? ? ? ? ? ? ? ?st2->need_parsing = AVSTREAM_PARSE_FULL;
> >> + ? ? ? ? ? ? ? ?pes2->st2 = pes->st2 = st2;
> >> + ? ? ? ? ? ?} else
> >> + ? ? ? ? ? ? ? ?av_free(pes2);
> >
> > Can we continue with pes->st2 not set but pes->st set ?
> > If not pes->st should be freed also.
> >
> > if (!st2) {
> > ? ?av_free(pes2);
> > ? ?return NULL;
> > }
> >
> > Looks better IMHO.
> 
> Yeah, I agree - if there's an error it makes sense to pass it up
> instead of hiding it, too...
> 
> >> [...]
> >>
> >> @@ -592,7 +616,7 @@ static void pmt_cb(MpegTSFilter *filter, const
> >> ? ? ? MpegTSContext *ts = filter->u.section_filter.opaque;
> >> ? ? ? SectionHeader h1, *h =&h1;
> >> ? ? ? PESContext *pes;
> >> - ? ?AVStream *st;
> >> + ? ?AVStream *st, *st2;
> >> ? ? ? const uint8_t *p, *p_end, *desc_list_end, *desc_end;
> >> ? ? ? int program_info_length, pcr_pid, pid, stream_type;
> >> ? ? ? int desc_list_len, desc_len, desc_tag;
> >> @@ -730,6 +754,14 @@ static void pmt_cb(MpegTSFilter *filter, const
> >> ? ? ? ? ? ? ? ? ? break;
> >> ? ? ? ? ? ? ? }
> >> ? ? ? ? ? ? ? p = desc_end;
> >> +
> >> + ? ? ? ? ? ?if (prog_reg_desc == AV_RL32("HDMV")&& ?stream_type == 0
> >> + ? ? ? ? ? ? ? ?st2 = pes->st2;
> >> + ? ? ? ? ? ? ? ?if (st2) {
> >> + ? ? ? ? ? ? ? ? ? ?av_program_add_stream_index(ts->stream, h->id, s
> >> + ? ? ? ? ? ? ? ? ? ?st2->codec->codec_tag = st->codec->codec_tag;
> >> + ? ? ? ? ? ? ? ?}
> >> + ? ? ? ? ? ?}
> >> ? ? ? ? ? }
> >> ? ? ? ? ? p = desc_list_end;
> >
> > Is st2 variable really needed here ?
> 
> No - it was originally there for symmetry with st, but that's used
> differently, so the reasoning falls down. Removed.
> 
> Updated patch addressing comments attached.
> 
> [...]
>
> @@ -568,8 +571,32 @@ static AVStream *new_pes_av_stream(PESContext *pes, uint32_t prog_reg_desc, uint
>  
>      mpegts_find_stream_type(st, pes->stream_type, ISO_types);
>      if (prog_reg_desc == AV_RL32("HDMV") &&
> -        st->codec->codec_id == CODEC_ID_NONE)
> +        st->codec->codec_id == CODEC_ID_NONE) {
>          mpegts_find_stream_type(st, pes->stream_type, HDMV_types);
> +        if (pes->stream_type == 0x83) {
> +            // HDMV TrueHD streams also contain an AC3 coded version of the
> +            // audio track - add a second stream for this
> +            AVStream *sub_st;
> +            // priv_data cannot be shared between streams
> +            PESContext *sub_pes = av_malloc(sizeof(PESContext));

sizeof(*sub_pes);

> +            memcpy(sub_pes, pes, sizeof(*sub_pes));
> +            if (!sub_pes)
> +                return NULL;

It feels better to check for sub_pes before actually memcpying over it :>

> [...]
>  
> @@ -730,6 +757,13 @@ static void pmt_cb(MpegTSFilter *filter, const uint8_t *section, int section_len
>                  break;
>              }
>              p = desc_end;
> +
> +            if (prog_reg_desc == AV_RL32("HDMV") && stream_type == 0x83) {
> +                if (pes->sub_st) {

I think you can merge the condition with the previous one.

Except that you can commit if it works.

[...]

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org