[FFmpeg-devel] [PATCHv2 1/5] avcodec: add side_data type for updated metadata

[wm4]

On Sat, 26 Oct 2013 19:49:35 +0200
Michael Niedermayer <michaelni at gmx.at> wrote:

> On Sat, Oct 26, 2013 at 06:18:36PM +0200, wm4 wrote:
> > On Sat, 26 Oct 2013 17:37:12 +0200
> > Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
> > 
> > > On Sat, Oct 26, 2013 at 01:58:07PM +0200, wm4 wrote:
> > > > On Fri, 25 Oct 2013 22:18:04 -0400
> > > > Ben Boeckel <mathstuf at gmail.com> wrote:
> > > > 
> > > > > This type is intended to be used to allow codecs to pass updated
> > > > > metadata to applications.
> > > > > 
> > > > > Signed-off-by: Ben Boeckel <mathstuf at gmail.com>
> > > > > ---
> > > > >  libavcodec/avcodec.h | 6 ++++++
> > > > >  1 file changed, 6 insertions(+)
> > > > > 
> > > > > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > > > > index b64331d..f78475e 100644
> > > > > --- a/libavcodec/avcodec.h
> > > > > +++ b/libavcodec/avcodec.h
> > > > > @@ -1012,6 +1012,12 @@ enum AVPacketSideDataType {
> > > > >       * follow the timestamp specifier of a WebVTT cue.
> > > > >       */
> > > > >      AV_PKT_DATA_WEBVTT_SETTINGS,
> > > > > +
> > > > > +    /**
> > > > > +     * A list of zero terminated key/value strings. There is no end marker for
> > > > > +     * the list, so it is required to rely on the side data size to stop.
> > > > > +     */
> > > > > +    AV_PKT_DATA_METADATA_UPDATE,
> > > > >  };
> > > > >  
> > > > >  /**
> > > > 
> > > > Looks ok, but I think the comment should also say that it updates
> > > > AVStream.metadata.
> > > 
> > > Actually I'd kind of appreciate some documentation on what its purpose
> > > is.
> > > I mean, if it's just to signal that the .metadata entry was updated, I
> > > would guess you could put that e.g. as a flag into the context/packet?
> > > I mostly wonder what the basic design idea/philosophy is for doing it
> > > that way, and I think that kind of information will be useful to have
> > > in the comment, otherwise users will just get incredibly confused when
> > > there are multiple ways to get metadata with no good explanation why
> > > there is more than one and they can't decide which to support...
> > 
> > The problem with a flag is IMO that it introduces extra mutable state
> > to AVStream (and the user has to reset the flag, getting all pretty
> > awkward...), so a packet-based way to signal it wouldn't be too bad.
> > The same is done for decoder parameter changes etc., though that's
> > perhaps more because there's no other way to communicate something to
> > the decoder.
> > 
> > Now, the question is why the side data needs to contain any data at all
> > (instead of merely signaling a metadata update). I think I agree that
> > this is not needed - unless the data contains only _changed_ entries.
> > (Changed as in explicitly updated in the file, no matter whether an
> > entry actually has a different value from the old one.) This way it
> > would be easier to know which keys changed, instead of having to figure
> > it out manually.
> > 
> > If we don't do this, the side data should perhaps have size 0.
> 
> If you (in a player) want the display of metadata and decoder
> presentation to be in sync then its easiest to send the actual
> metadata through the AVPacket queues of the player to the decoder.
> 
> otherwise the player would have to copy the AVStream metadata into a
> fifo with timestamps and wait until they match the decoders output
> before being presentet to the user
> 
> also if you dont inlude the full metadata, then storing that stream
> again (as in stream copy or also re encoding) will not be easy
> 
> and just changed data can cause problems with seeking as changed
> depends on the previous state, seeking can have an arbitrary previous
> state.

An application could add support for this to its own packet structure,
so I don't think this is necessarily a good argument for more
complexity.

Anyway, I'm not against it.