On Sun, 4 May 2014 12:24:48 +0300 (EEST), Martin Storsjö <[email protected]>
wrote:
> On Sat, 3 May 2014, Anton Khirnov wrote:
>
> > ---
> > libavformat/mux.c | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/libavformat/mux.c b/libavformat/mux.c
> > index dd88225..7f67c4d 100644
> > --- a/libavformat/mux.c
> > +++ b/libavformat/mux.c
> > @@ -407,6 +407,9 @@ static int compute_pkt_fields2(AVFormatContext *s,
> > AVStream *st, AVPacket *pkt)
> >
> > static int write_packet(AVFormatContext *s, AVPacket *pkt)
> > {
> > + AVStream *st = s->streams[pkt->stream_index];
> > + uint8_t *new_extradata;
> > + int extradata_size;
> > int ret;
> > if (!(s->oformat->flags & (AVFMT_TS_NEGATIVE | AVFMT_NOTIMESTAMPS))) {
> > AVRational time_base = s->streams[pkt->stream_index]->time_base;
> > @@ -424,6 +427,21 @@ static int write_packet(AVFormatContext *s, AVPacket
> > *pkt)
> > if (pkt->pts != AV_NOPTS_VALUE)
> > pkt->pts += offset;
> > }
> > +
> > + new_extradata = av_packet_get_side_data(pkt, AV_PKT_DATA_NEW_EXTRADATA,
> > + &extradata_size);
> > + if (new_extradata) {
> > + av_freep(&st->codec->extradata);
> > + st->codec->extradata_size = 0;
> > +
> > + st->codec->extradata = av_mallocz(extradata_size +
> > FF_INPUT_BUFFER_PADDING_SIZE);
> > + if (!st->codec->extradata)
> > + return AVERROR(ENOMEM);
> > +
> > + memcpy(st->codec->extradata, new_extradata, extradata_size);
> > + st->codec->extradata_size = extradata_size;
> > + }
> > +
> > ret = s->oformat->write_packet(s, pkt);
> >
> > if (s->pb && ret >= 0 && s->flags & AVFMT_FLAG_FLUSH_PACKETS)
> > --
> > 1.7.10.4
>
> I didn't read most of the rest of the patchset yet - is this used
> somewhere or just a suggestion on how to fix general behaviour?
>
Yes, 08/10 uses it.
The practical problem I'm solving here is that the flac extradata contains
information that is only known at the end of encoding -- md5 of the whole
content and total number of samples.
The way our encoder does this currently is that on flush it just updates the
extradata, but does not return any packets. Then the flac muxer overwrites this
updated extradata in the header.
Obviously, this will only work if:
- the stream codec context is the encoding context (which I think we all agree
is evil and needs to be eliminated)
- or if the new extradata is somehow sent to the muxer. From currently available
mechanisms, i see only two ways for this:
* we use the side data, as is done in this patchset
* we mandate that the codec context needs to be updated manually (or perhaps
though avcodec_copy_context()) by the caller. But this brings other kinds
of breakage.
> If one wants to support changes in extradata, the muxer itself needs to be
> made aware of the change in one way or another. Doing this here doesn't
> stop the muxer from reading the same side data and doing something with
> it, of course, but a muxer unaware of the changes will only see the latest
> extradata, not the first. Which one of them is the less wrong one depends
> on the actual case of course, but neither of them is really right if you
> want to handle the changes (e.g. write them inline in the stream at the
> right place).
>
> Secondly; this overwrites data in AVFormatContext->streams[x]->codec,
> which muxers didn't use to touch at all before. If the user uses this as
> the actual context for the encoders (which you ideally shouldn't, but
> which I bet a lot do, which I'd prefer we didn't break), we'd be
> clobbering data which only the encoder is supposed to set. OTOH I don't
> think anything bad would happen by doing it in practice though.
>
I guess I can avoid those problems for now, by dropping this patch and making
the flac muxer explicitly check for updated extradata in the side data.
Or if you see a nicer way, I'm listening.
--
Anton Khirnov
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
