[FFmpeg-devel] [patch][GSoC]WTV muxer (2_wtvenc.patch, 3_add_wtv_muxer.patch)

zhentan feng spyfeng at gmail.com
Tue Aug 23 15:37:35 CEST 2011


On Sat, Aug 20, 2011 at 11:20 PM, Peter Ross <pross at xvid.org> wrote:

> Hi. Comments against 2_wtvenc.patch and 3_add_wtv_muxer.patch
>
> On Sun, Aug 14, 2011 at 09:07:59PM +0800, zhentan feng wrote:
> > Hi,
> > The attachment are 3 patches.
> > #1 patch extracts common code from wtvdec.c and wtvenc.c to wtv.c and
> wtv.h
> > #2 patch is the implementation wtvenc.c
> > #3 patch integrates the muxer.
>
> > diff --git a/libavformat/wtvenc.c b/libavformat/wtvenc.c
> > new file mode 100644
> > index 0000000..56c8baa
> > --- /dev/null
> > +++ b/libavformat/wtvenc.c
> > @@ -0,0 +1,739 @@
> > +/*
> > + * Windows Television (WTV) muxer
> > + * Copyright (c) 2011 Zhentan Feng <spyfeng at gmail dot com>
> > + * Copyright (c) 2011 Peter Ross <pross at xvid.org>
> > + * This file is part of FFmpeg.
> > + *
> > + * FFmpeg is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU Lesser General Public
> > + * License as published by the Free Software Foundation; either
> > + * version 2.1 of the License, or (at your option) any later version.
> > + *
> > + * FFmpeg is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > + * Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public
> > + * License along with FFmpeg; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301 USA
> > + */
> > +
> > +#include "libavutil/intreadwrite.h"
> > +#include "avformat.h"
> > +#include "internal.h"
> > +#include "wtv.h"
>
> > +
> > +// ---
> > +// FIXME: when stable, move this stuff to wtv.h
> > +
>
> Yhis comment is no longer needed
>


removed.

>
> [...]
> > +
> > +    put_guid(pb, media_type); // mediatype
> > +    put_guid(pb, &mediasubtype_cpfilters_processed); // subtype
> > +    write_pad(pb, 12);
> > +    put_guid(pb,&format_cpfilters_processed); // format type
> > +    avio_wl32(pb, 0); // size
> > +
>     ^^
>     trailing whitespace detected
>


removed. and make sure all these whitespace are removed.

>
> [...]
> > +
> > +    write_chunk_header(s, &sync_guid, 0x18, 0);
> > +    avio_wl64(pb, 0);  // FIXME: file-offset to previous keyframe
> > +    avio_wl64(pb, -1); // FIXME: ??
> > +    avio_wl64(pb, 0);  // FIXME: ??
>
> Remove the 'FIXME: ??' comments. Like so much of the file format, these
> values are unknown.
>
>
removed.

> [...]
> > +
> > +    //write initial root fields
> > +    avio_wl32(pb, 0);  // root_size, update later
>
> nit: why the double space before comment?
>
>
fixed.

> > +    write_pad(pb, 4);
> > +    avio_wl32(pb, 0); // root_sector, update it later.
> > +
> > +    write_pad(pb, 32);
> > +    avio_wl32(pb, 0); // file ends pointer, update it later.
> > +
> > +    pad = (1 << WTV_SECTOR_BITS) - avio_tell(pb);
> > +    write_pad(pb, pad);
> > +    wctx->timeline_start_pos = avio_tell(pb);
> > +
> > +    wctx->serial = 1;
> > +    wctx->last_chunk_pos = -1;
> > +    wctx->first_video_flag = 1;
> > +
> > +    for (i = 0; i < s->nb_streams; i++) {
> > +        int ret;
>
> Consider declaring this at the top of the function.
>
>
fixed.

> [...]
> > +
> > +static const WTVRootEntryTable wtv_root_entry_table[] = {
> > +    { timeline_table_0_header_events,
> sizeof(timeline_table_0_header_events), write_table0_header_envents},
> > +    { timeline_table_0_entries_Events_le16,
> sizeof(timeline_table_0_entries_Events_le16), NULL},
> > +    { timeline_le16, sizeof(timeline_le16), NULL},
> > +    { table_0_header_legacy_attrib,
> sizeof(table_0_header_legacy_attrib), write_table0_header_legacy_attrib},
> > +    { table_0_entries_legacy_attrib_le16,
> sizeof(table_0_entries_legacy_attrib_le16), NULL},
> > +    { table_0_redirector_legacy_attrib,
> sizeof(table_0_redirector_legacy_attrib), NULL},
> > +    { table_0_header_time, sizeof(table_0_header_time),
> write_table0_header_time},
> > +    { table_0_entries_time_le16, sizeof(table_0_entries_time_le16),
> NULL},
> > +};
>
> Consider using spaces to align the table columns.
>   e.g.
>    { timeline_table_0_header_events,
> sizeof(timeline_table_0_header_events),       write_table0_header_envents},
>    { timeline_table_0_entries_Events_le16,
> sizeof(timeline_table_0_entries_Events_le16), NULL},
>
>

aligned the struct.

> [...]
> > +
> > +// table 2
> > +static void write_table_entries_events(AVFormatContext *s)
> > +{
> > +    AVIOContext *pb = s->pb;
> > +    //WtvContext *wctx = s->priv_data;
> > +
> > +    //FIXME: output frame_nb, position pairs
> > +    //avio_wl64(pb, 0x2);   avio_wl64(pb, 0xc0);
> > +    avio_wl64(pb, 0x2);   avio_wl64(pb, 0x170);
>
> I thought this single-entry had to be present for WMC to playback the file
> _and_ that
> it had to point to the a chunk.
>
> 0x170 is the file-offset (aka position) within the timeline file. 0x170 was
> just
> hard-coded for testing. Should this be caclulated from write_headers()?
>
>

Fixed.

> [...]
> > +// table 7
> > +// The content can be pad as zeroes because it is not necessary for
> playback.
> > +static void write_table_entries_time(AVFormatContext *s)
> > +{
> > +    //AVIOContext *pb = s->pb;
> > +    //WtvContext *wctx = s->priv_data;
> > +
> > +#if 0
> > +    //avio_wl64(pb, 0x0);      avio_wl64(pb, 0x30);
> > +    //avio_wl64(pb, 0x4c4b40); avio_wl64(pb, 0x43);
> > +    //avio_wl64(pb, 0x7c8300); avio_wl64(pb, 0x50);
> > +#else
> > +    //FIXME: output timestamp, frame_nb pairs
> > +#endif
> > +}
>
> If the muxer is not going to output timestmaps, then remove this
> placeholder function
> and insert the FIXME down below where write_table_entries_time() is called.
>
>

removed.

> [...]
> > diff --git a/libavformat/Makefile b/libavformat/Makefile
> > index 9f5bfb4..a47e758 100644
> > --- a/libavformat/Makefile
> > +++ b/libavformat/Makefile
> > @@ -303,6 +303,7 @@ OBJS-$(CONFIG_WSAUD_DEMUXER)             +=
> westwood.o
> >  OBJS-$(CONFIG_WSVQA_DEMUXER)             += westwood.o
> >  OBJS-$(CONFIG_WTV_DEMUXER)               += wtvdec.o wtv.o asfdec.o
> asf.o asfcrypt.o \
> >                                              avlanguage.o mpegts.o isom.o
> riff.o
> > +OBJS-$(CONFIG_WTV_MUXER)                 += wtvenc.o wtv.o asf.o riff.o
> >  OBJS-$(CONFIG_WV_DEMUXER)                += wv.o apetag.o
> >  OBJS-$(CONFIG_XA_DEMUXER)                += xa.o
> >  OBJS-$(CONFIG_XWMA_DEMUXER)              += xwma.o riff.o
> > diff --git a/libavformat/allformats.c b/libavformat/allformats.c
> > index a9fa117..c11d744 100644
> > --- a/libavformat/allformats.c
> > +++ b/libavformat/allformats.c
> > @@ -224,7 +224,7 @@ void av_register_all(void)
> >      REGISTER_MUXER    (WEBM, webm);
> >      REGISTER_DEMUXER  (WSAUD, wsaud);
> >      REGISTER_DEMUXER  (WSVQA, wsvqa);
> > -    REGISTER_DEMUXER  (WTV, wtv);
> > +    REGISTER_MUXDEMUX (WTV, wtv);
> >      REGISTER_DEMUXER  (WV, wv);
> >      REGISTER_DEMUXER  (XA, xa);
> >      REGISTER_DEMUXER  (XWMA, xwma);
>
> Please go over the 'New codecs or formats checklist' in doc/developer.texi.
> Your patch needs to update Changelog, doc/muxers.texi, etc.
>
> Also patches 2 and 3 could probably be merged together to form a single
> 'WTV_muxer.patch'.
>
>
>
The latest patch is attached below.
please review.
Thanks
zhentan
-- 
Best wishes~
-------------- next part --------------
A non-text attachment was scrubbed...
Name: wtv_muxer.patch
Type: application/octet-stream
Size: 24531 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110823/a0445020/attachment.obj>


More information about the ffmpeg-devel mailing list