On Fri, Jun 1, 2012 at 1:59 PM, David Girault <[email protected]> wrote:
> Le vendredi 01 juin 2012 à 10:23 -0700, Alex Converse a écrit :
>> On Fri, Jun 1, 2012 at 8:11 AM, David Girault <[email protected]>
>> wrote:
>> >
>> > Added support for HDMV Interactive Graphics Stream in mpegts
>> >
>> > This new muxer/demuxer will allow to read/write menu stream
>> > from/to 'mnu' files. This will allow to extract menu from BDAV
>> > m2ts file to a elementary stream file suitable for Bluray
>> > authoring software (such as BDedit from domm9).
>> >
>> > Mpeg TS muxer/demuxer was also updated to recognize these IGS
>> > menu streams. More update required to allow muxing IGS menu
>> > inside an mpegts file.
>> >
>> > Signed-off-by: David Girault <[email protected]>
>> > ---
>> > libavformat/Makefile | 2 +
>> > libavformat/allformats.c | 1 +
>> > libavformat/avformat.h | 1 +
>> > libavformat/mnudec.c | 103
>> > ++++++++++++++++++++++++++++++++++++++++++++++
>> > libavformat/mnuenc.c | 64 ++++++++++++++++++++++++++++
>> > libavformat/mpegts.c | 1 +
>> > 6 files changed, 172 insertions(+)
>> > create mode 100644 libavformat/mnudec.c
>> > create mode 100644 libavformat/mnuenc.c
>> >
>>
>> Seems pretty good overall but I have a few concerns about the probe.
>>
>> > diff --git a/libavformat/Makefile b/libavformat/Makefile
>> [...]
>> > diff --git a/libavformat/allformats.c b/libavformat/allformats.c
>> [...]
>> > diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>> [...]
>> > diff --git a/libavformat/mnudec.c b/libavformat/mnudec.c
>> > new file mode 100644
>> > index 0000000..857b0f1
>> > --- /dev/null
>> > +++ b/libavformat/mnudec.c
>> > @@ -0,0 +1,103 @@
>> > +/*
>> > + * MNU demuxer
>> > + * Copyright (c) 2012 David Girault
>> > + *
>> > + * This file is part of Libav.
>> > + *
>> > + * Libav 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.
>> > + *
>> > + * Libav 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 Libav; if not, write to the Free Software
>> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
>> > 02110-1301 USA
>> > + */
>> > +
>> > +#include "libavutil/mathematics.h"
>> > +#include "libavcodec/bytestream.h"
>> > +#include "avformat.h"
>> > +#include "internal.h"
>> > +
>> > +typedef struct MNUContext{
>> > + uint8_t *buffer;
>> > +} MNUContext;
>> > +
>> > +static int probe(AVProbeData *p)
>> > +{
>> > + const char *header= "IG";
>>
>> static const char header[]
>
> ok. no problem. it's a better syntax for that kind anyway. ;)
>
>>
>> > +
>> > + if(!memcmp(p->buf , header, strlen(header)))
>>
>> do we guarantee p->buf_size is always >= 2?
>
> No. if we have a corrupted source. By the way, Next point will resolve
> this issue.
>
This part is fine, we do have a minimum size
>>
>> > + return AVPROBE_SCORE_MAX;
>>
>> Two bytes seems like very little for AVPROBE_SCORE_MAX? Can we score
>> lower for just one packet and increase if we see 2+?
>
> This format, to be valid, require at least 4 frames (or segments):
> pages/buttons description segment, one palette segment, one picture
> segment and the last, the display segment.
>
> Usually, first segment may be large, more than probe buffer (which is 2k
> if I remind well).
>
Background on the probe. The probe buffer starts with 2048 bytes, if
it fails to find a format with confidence it retries with twice as
much data. It continues this retry process all the way up to 1 MB. At
1 MB if there is still no confident format it picks the format with
the highest score. The max segment size seems to be 65535 so this
should easily fit.
> So, we may check length is at least enough for ('IG'+ 8 bytes PTS&DTS +
> segment type + segment size) = 13 bytes. If IG and segment type is
> valid, I think it should be OK for scoring max*3/4.
>
This seems more reasonable.
> If probe buffer size is larger than first segment size, we may also
> check the marker of the second segment and result with score max.
>
> Is it ok for you?
>
This is probably ok
> Ps: There is some little questions at the end of the email not
> concerning your comments....
>
>>
>> > +
>> > + return 0;
>> > +}
>> > +
>> > +static int read_close(AVFormatContext *s)
>> > +{
>> > + MNUContext *mnu = s->priv_data;
>> > + av_freep(&mnu->buffer);
>> > + return 0;
>> > +}
>> > +
>> > +static int read_header(AVFormatContext *s)
>> > +{
>> > + AVStream *st = avformat_new_stream(s, NULL);
>> > + if (!st)
>> > + return -1;
>>
>> return AVERROR(ENOMEM)
>>
>> > + //avpriv_set_pts_info(st, 64, 1, 100);
>> > + avpriv_set_pts_info(st, 33, 1, 90000);
>> > + st->codec->codec_type = AVMEDIA_TYPE_OVERLAY;
>> > + st->codec->codec_id= CODEC_ID_HDMV_IGS_MENU;
>> > + return 0;
>> > +}
>> > +
>> > +static int read_packet(AVFormatContext *s, AVPacket *pkt)
>> > +{
>> > +// MNUContext *mnu = s->priv_data;
>> > + uint64_t pos = avio_tell(s->pb);
>> > + int res = AVERROR_EOF;
>> > +
>> > + char header[2];
>> > + uint32_t pts, dts;
>> > + uint8_t seg_type;
>> > + uint16_t seg_length;
>> > +
>> > + // "IG",PTS,DTS,SEG_TYPE,SEG_LENGTH
>> > + res = avio_read(s->pb, header, 2);
>> > + if (header[0]!='I'||header[1]!='G') return AVERROR_INVALIDDATA;
>> > + pts = avio_rb32(s->pb);
>> > + dts = avio_rb32(s->pb);
>> > + seg_type = avio_r8(s->pb);
>> > + seg_length = avio_rb16(s->pb);
>> > +
>> > + res = av_new_packet(pkt, seg_length+3);
>> > + if (!res) {
>> > + uint8_t *buf = pkt->data;
>> > + bytestream_put_byte(&buf, seg_type);
>> > + bytestream_put_be16(&buf, seg_length);
>> > + avio_read(s->pb, buf, seg_length);
>> > + pkt->flags |= AV_PKT_FLAG_KEY;
>> > + pkt->pos = pos;
>> > + pkt->pts = pts;
>> > + pkt->dts = dts;
>> > + av_dlog(s, "New IG packet @ 0x%lx: type %d, length %d\n",
>> > + pos, seg_type, seg_length);
>> > + }
>> > + return res;
>> > +}
>> > +
>> > +AVInputFormat ff_mnu_demuxer = {
>> > + .name = "mnu",
>> > + .long_name = NULL_IF_CONFIG_SMALL("HDMV Interactive Graphic
>> > menus format"),
>> > + .priv_data_size = sizeof(MNUContext),
>> > + .read_probe = probe,
>> > + .read_header = read_header,
>> > + .read_packet = read_packet,
>> > + .read_close = read_close,
>> > +};
>> > diff --git a/libavformat/mnuenc.c b/libavformat/mnuenc.c
>> > new file mode 100644
>> > index 0000000..15d0ff8
>> > --- /dev/null
>> > +++ b/libavformat/mnuenc.c
>> > @@ -0,0 +1,64 @@
>> > +/*
>> > + * MNU demuxer
>> > + * Copyright (c) 2012 David Girault
>> > + *
>> > + * This file is part of Libav.
>> > + *
>> > + * Libav 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.
>> > + *
>> > + * Libav 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 Libav; if not, write to the Free Software
>> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
>> > 02110-1301 USA
>> > + */
>> > +
>> > +#include "libavutil/mathematics.h"
>> > +#include "libavcodec/bytestream.h"
>> > +#include "avformat.h"
>> > +#include "internal.h"
>> > +
>> > +static int mnu_write_header(AVFormatContext *s)
>> > +{
>> > + AVCodecContext *ctx;
>> > +
>> > + if (s->nb_streams != 1) {
>> > + av_log(s, AV_LOG_ERROR, "Format supports only exactly one overlay
>> > stream\n");
>> > + return AVERROR(EINVAL);
>> > + }
>> > + ctx = s->streams[0]->codec;
>> > + if (ctx->codec_type != AVMEDIA_TYPE_OVERLAY || ctx->codec_id !=
>> > CODEC_ID_HDMV_IGS_MENU) {
>> > + av_log(s, AV_LOG_ERROR, "Currently only HDMV IGS menu is
>> > supported!\n");
>> > + return AVERROR(EINVAL);
>> > + }
>> > + return 0;
>> > +}
>> > +
>> > +static int mnu_write_packet(AVFormatContext *s, AVPacket *pkt)
>> > +{
>> > + AVIOContext *pb = s->pb;
>> > + avio_write(pb, "IG", 2);
>> > + avio_wb32(pb, pkt->pts);
>> > + avio_wb32(pb, pkt->dts);
>> > + avio_write(pb, pkt->data, pkt->size);
>> > + avio_flush(pb);
>> > + return 0;
>> > +}
>> > +
>> > +AVOutputFormat ff_mnu_muxer = {
>> > + .name = "mnu",
>> > + .long_name = NULL_IF_CONFIG_SMALL("HDMV Interactive Graphic menus
>> > format"),
>> > + .extensions = "mnu",
>> > + .audio_codec = CODEC_ID_NONE,
>> > + .video_codec = CODEC_ID_NONE,
>> > + .subtitle_codec = CODEC_ID_NONE,
>> > + .overlay_codec = CODEC_ID_HDMV_IGS_MENU,
>> > + .write_header = mnu_write_header,
>> > + .write_packet = mnu_write_packet,
>> > +};
>> > diff --git a/libavformat/mpegts.c b/libavformat/mpegts.c
>> [...]
>>
>> --Alex
>
>
>
> Just a little question now, what is the best way for sending the new
> version of this patch? The five patches are on a same local git branch.
> Is an additional patch (6/5) that fixes your concern is ok?
> Is someone have some links to read?
>
The preferred way to do it is to rebase the changes into this patch,
then format-patch the whole patchset and resend just the changed
patches (in this case just this patch with with git send-email, use
the message ID of something in this thread when prompted for message
id).
If you are unfamiliar with rebase, the following is good background:
http://learn.github.com/p/rebasing.html and
http://git-scm.com/book/en/Git-Tools-Rewriting-History#Changing-Multiple-Commit-Messages
--Alex
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
