| Message ID | 20180625230332.197656-1-modmaker@google.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jun 25, 2018 at 4:04 PM Jacob Trimble <modmaker@google.com> wrote: > > Signed-off-by: Jacob Trimble <modmaker@google.com> > --- > libavformat/avformat.h | 8 ++++++++ > libavformat/utils.c | 11 +++++++++++ > 2 files changed, 19 insertions(+) > > diff --git a/libavformat/avformat.h b/libavformat/avformat.h > index fdaffa5bf4..434c88837e 100644 > --- a/libavformat/avformat.h > +++ b/libavformat/avformat.h > @@ -2167,6 +2167,14 @@ AVStream *avformat_new_stream(AVFormatContext *s, const AVCodec *c); > int av_stream_add_side_data(AVStream *st, enum AVPacketSideDataType type, > uint8_t *data, size_t size); > > +/** > + * Removes any existing side data of the given type. > + * > + * @param st stream > + * @param type side information type > + */ > +void av_stream_remove_side_data(AVStream *st, enum AVPacketSideDataType type); > + > /** > * Allocate new information from stream. > * > diff --git a/libavformat/utils.c b/libavformat/utils.c > index c9cdd2b470..4f7c408d93 100644 > --- a/libavformat/utils.c > +++ b/libavformat/utils.c > @@ -5491,6 +5491,17 @@ int av_stream_add_side_data(AVStream *st, enum AVPacketSideDataType type, > return 0; > } > > +void av_stream_remove_side_data(AVStream *st, enum AVPacketSideDataType type) > +{ > + for (int i = 0; i < st->nb_side_data; i++) { > + if (st->side_data[i].type == type) { > + av_freep(&st->side_data[i].data); > + st->side_data[i] = st->side_data[st->nb_side_data - 1]; > + st->nb_side_data--; > + } > + } > +} > + > uint8_t *av_stream_new_side_data(AVStream *st, enum AVPacketSideDataType type, > int size) > { > -- > 2.18.0.rc2.346.g013aa6912e-goog > Ping.
On Mon, Jun 25, 2018 at 04:03:32PM -0700, Jacob Trimble wrote: > Signed-off-by: Jacob Trimble <modmaker@google.com> > --- > libavformat/avformat.h | 8 ++++++++ > libavformat/utils.c | 11 +++++++++++ > 2 files changed, 19 insertions(+) > > diff --git a/libavformat/avformat.h b/libavformat/avformat.h > index fdaffa5bf4..434c88837e 100644 > --- a/libavformat/avformat.h > +++ b/libavformat/avformat.h > @@ -2167,6 +2167,14 @@ AVStream *avformat_new_stream(AVFormatContext *s, const AVCodec *c); > int av_stream_add_side_data(AVStream *st, enum AVPacketSideDataType type, > uint8_t *data, size_t size); > > +/** > + * Removes any existing side data of the given type. > + * > + * @param st stream > + * @param type side information type > + */ > +void av_stream_remove_side_data(AVStream *st, enum AVPacketSideDataType type); What would use this and why ? The commit message does not explain this If side data is changing it probably should be put in AVPackets or AVFrames not the stream. [...]
On Mon, Jul 2, 2018 at 6:07 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Mon, Jun 25, 2018 at 04:03:32PM -0700, Jacob Trimble wrote: > > Signed-off-by: Jacob Trimble <modmaker@google.com> > > --- > > libavformat/avformat.h | 8 ++++++++ > > libavformat/utils.c | 11 +++++++++++ > > 2 files changed, 19 insertions(+) > > > > > > > > > diff --git a/libavformat/avformat.h b/libavformat/avformat.h > > index fdaffa5bf4..434c88837e 100644 > > --- a/libavformat/avformat.h > > +++ b/libavformat/avformat.h > > @@ -2167,6 +2167,14 @@ AVStream *avformat_new_stream(AVFormatContext *s, const AVCodec *c); > > int av_stream_add_side_data(AVStream *st, enum AVPacketSideDataType type, > > uint8_t *data, size_t size); > > > > +/** > > + * Removes any existing side data of the given type. > > + * > > + * @param st stream > > + * @param type side information type > > + */ > > +void av_stream_remove_side_data(AVStream *st, enum AVPacketSideDataType type); > > What would use this and why ? > The commit message does not explain this > > If side data is changing it probably should be put in AVPackets or AVFrames > not the stream. > I am using this to removing the side data that contains the AVEncryptionInitInfo objects once I handle them. Since an MP4 file can contain multiple pssh atoms, there can be multiple AVEncryptionInitInfo structs. To make it easier for me, I want to remove the side data that contain them once I have handled them. This means that if the AVStream contains the side data, it is because of new init info I haven't seen. Since the pssh atoms are more "global" it makes more sense to put them in the AVStream. > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > You can kill me, but you cannot change the truth. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Tue, Jul 03, 2018 at 12:14:19PM -0700, Jacob Trimble wrote: > On Mon, Jul 2, 2018 at 6:07 PM Michael Niedermayer > <michael@niedermayer.cc> wrote: > > > > On Mon, Jun 25, 2018 at 04:03:32PM -0700, Jacob Trimble wrote: > > > Signed-off-by: Jacob Trimble <modmaker@google.com> > > > --- > > > libavformat/avformat.h | 8 ++++++++ > > > libavformat/utils.c | 11 +++++++++++ > > > 2 files changed, 19 insertions(+) > > > > > > > > > > > > > > > > diff --git a/libavformat/avformat.h b/libavformat/avformat.h > > > index fdaffa5bf4..434c88837e 100644 > > > --- a/libavformat/avformat.h > > > +++ b/libavformat/avformat.h > > > @@ -2167,6 +2167,14 @@ AVStream *avformat_new_stream(AVFormatContext *s, const AVCodec *c); > > > int av_stream_add_side_data(AVStream *st, enum AVPacketSideDataType type, > > > uint8_t *data, size_t size); > > > > > > +/** > > > + * Removes any existing side data of the given type. > > > + * > > > + * @param st stream > > > + * @param type side information type > > > + */ > > > +void av_stream_remove_side_data(AVStream *st, enum AVPacketSideDataType type); > > > > What would use this and why ? > > The commit message does not explain this > > > > If side data is changing it probably should be put in AVPackets or AVFrames > > not the stream. > > > > I am using this to removing the side data that contains the > AVEncryptionInitInfo objects once I handle them. Since an MP4 file > can contain multiple pssh atoms, there can be multiple > AVEncryptionInitInfo structs. To make it easier for me, I want to > remove the side data that contain them once I have handled them. This > means that if the AVStream contains the side data, it is because of > new init info I haven't seen. Since the pssh atoms are more "global" > it makes more sense to put them in the AVStream. I dont fully understand but If you intend to remove things while reading the "header" of a mp4 file these things probably should not be in side data to begin with but be internal to the demuxer. otherwise, after the header or outside the demuxer removal seems a "no go" but i may misunderstand what you intend to do. Please explain if iam totally off here with how i interpret this One simple API good vs. bad test btw should be to consider that theres a demuxer connected to a muxer. If this does not work to produce a equivalent file the API is not good for example if you change side data in the AVStream in the middle between outputing packets i would not expect the muxer to see this and thus not be able to reproduce this in the stored file. Also if you mess with the demxuers side data from outside, not only will this result in undefined behavior it also might be that you still need it when for example seeking back to the start again maybe i totally misunderstand what you intend here [...]
On Tue, Jul 3, 2018 at 5:59 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > > On Tue, Jul 03, 2018 at 12:14:19PM -0700, Jacob Trimble wrote: > > On Mon, Jul 2, 2018 at 6:07 PM Michael Niedermayer > > <michael@niedermayer.cc> wrote: > > > > > > On Mon, Jun 25, 2018 at 04:03:32PM -0700, Jacob Trimble wrote: > > > > Signed-off-by: Jacob Trimble <modmaker@google.com> > > > > --- > > > > libavformat/avformat.h | 8 ++++++++ > > > > libavformat/utils.c | 11 +++++++++++ > > > > 2 files changed, 19 insertions(+) > > > > > > > > > > > > > > > > > > > > > > > diff --git a/libavformat/avformat.h b/libavformat/avformat.h > > > > index fdaffa5bf4..434c88837e 100644 > > > > --- a/libavformat/avformat.h > > > > +++ b/libavformat/avformat.h > > > > @@ -2167,6 +2167,14 @@ AVStream *avformat_new_stream(AVFormatContext *s, const AVCodec *c); > > > > int av_stream_add_side_data(AVStream *st, enum AVPacketSideDataType type, > > > > uint8_t *data, size_t size); > > > > > > > > +/** > > > > + * Removes any existing side data of the given type. > > > > + * > > > > + * @param st stream > > > > + * @param type side information type > > > > + */ > > > > +void av_stream_remove_side_data(AVStream *st, enum AVPacketSideDataType type); > > > > > > What would use this and why ? > > > The commit message does not explain this > > > > > > If side data is changing it probably should be put in AVPackets or AVFrames > > > not the stream. > > > > > > > I am using this to removing the side data that contains the > > AVEncryptionInitInfo objects once I handle them. Since an MP4 file > > can contain multiple pssh atoms, there can be multiple > > AVEncryptionInitInfo structs. To make it easier for me, I want to > > remove the side data that contain them once I have handled them. This > > means that if the AVStream contains the side data, it is because of > > new init info I haven't seen. Since the pssh atoms are more "global" > > it makes more sense to put them in the AVStream. > > I dont fully understand but > If you intend to remove things while reading the "header" of a mp4 file > these things probably should not be in side data to begin with but be > internal to the demuxer. > > otherwise, after the header or outside the demuxer removal seems a "no go" > but i may misunderstand what you intend to do. Please explain if iam > totally off here with how i interpret this > > One simple API good vs. bad test btw should be to consider that theres > a demuxer connected to a muxer. > If this does not work to produce a equivalent file the API is not good > for example if you change side data in the AVStream in the middle between > outputing packets i would not expect the muxer to see this and thus not > be able to reproduce this in the stored file. > > Also if you mess with the demxuers side data from outside, not only > will this result in undefined behavior it also might be that you still > need it when for example seeking back to the start > > again maybe i totally misunderstand what you intend here > I would expect the muxer to do what I am doing, it would remove the side data when it handles the data so it doesn't have to keep a copy of all the init data it has seen. For example, consider converting fragmented MP4 into a different fragmented MP4. The pssh atoms can appear inside the fragments, so the muxer should see the new pssh atoms and add them to the current/next fragment while muxing. The best way I see is to check if the side data exists, handle it, and remove the side data. The alternative would be to convert the side data to the AVEncryptionInitInfo struct at every step, then compare each element against a copy the muxer has. This is extremely slow and requires storing the init data several different ways. Another alternative would be to put the side data on the frames, but this doesn't seem right either. The init info is "header" data, so it seems weird to put it on a random frame, and putting the data on every frame would be more duplication and require the muxer/app to compare them for every frame to detect new init info. > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > The worst form of inequality is to try to make unequal things equal. > -- Aristotle > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Mon, Jul 9, 2018 at 9:57 AM Jacob Trimble <modmaker@google.com> wrote: > > On Tue, Jul 3, 2018 at 5:59 PM Michael Niedermayer > <michael@niedermayer.cc> wrote: > > > > On Tue, Jul 03, 2018 at 12:14:19PM -0700, Jacob Trimble wrote: > > > On Mon, Jul 2, 2018 at 6:07 PM Michael Niedermayer > > > <michael@niedermayer.cc> wrote: > > > > > > > > On Mon, Jun 25, 2018 at 04:03:32PM -0700, Jacob Trimble wrote: > > > > > Signed-off-by: Jacob Trimble <modmaker@google.com> > > > > > --- > > > > > libavformat/avformat.h | 8 ++++++++ > > > > > libavformat/utils.c | 11 +++++++++++ > > > > > 2 files changed, 19 insertions(+) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/libavformat/avformat.h b/libavformat/avformat.h > > > > > index fdaffa5bf4..434c88837e 100644 > > > > > --- a/libavformat/avformat.h > > > > > +++ b/libavformat/avformat.h > > > > > @@ -2167,6 +2167,14 @@ AVStream *avformat_new_stream(AVFormatContext *s, const AVCodec *c); > > > > > int av_stream_add_side_data(AVStream *st, enum AVPacketSideDataType type, > > > > > uint8_t *data, size_t size); > > > > > > > > > > +/** > > > > > + * Removes any existing side data of the given type. > > > > > + * > > > > > + * @param st stream > > > > > + * @param type side information type > > > > > + */ > > > > > +void av_stream_remove_side_data(AVStream *st, enum AVPacketSideDataType type); > > > > > > > > What would use this and why ? > > > > The commit message does not explain this > > > > > > > > If side data is changing it probably should be put in AVPackets or AVFrames > > > > not the stream. > > > > > > > > > > I am using this to removing the side data that contains the > > > AVEncryptionInitInfo objects once I handle them. Since an MP4 file > > > can contain multiple pssh atoms, there can be multiple > > > AVEncryptionInitInfo structs. To make it easier for me, I want to > > > remove the side data that contain them once I have handled them. This > > > means that if the AVStream contains the side data, it is because of > > > new init info I haven't seen. Since the pssh atoms are more "global" > > > it makes more sense to put them in the AVStream. > > > > I dont fully understand but > > If you intend to remove things while reading the "header" of a mp4 file > > these things probably should not be in side data to begin with but be > > internal to the demuxer. > > > > otherwise, after the header or outside the demuxer removal seems a "no go" > > but i may misunderstand what you intend to do. Please explain if iam > > totally off here with how i interpret this > > > > One simple API good vs. bad test btw should be to consider that theres > > a demuxer connected to a muxer. > > If this does not work to produce a equivalent file the API is not good > > for example if you change side data in the AVStream in the middle between > > outputing packets i would not expect the muxer to see this and thus not > > be able to reproduce this in the stored file. > > > > Also if you mess with the demxuers side data from outside, not only > > will this result in undefined behavior it also might be that you still > > need it when for example seeking back to the start > > > > again maybe i totally misunderstand what you intend here > > > > I would expect the muxer to do what I am doing, it would remove the > side data when it handles the data so it doesn't have to keep a copy > of all the init data it has seen. > > For example, consider converting fragmented MP4 into a different > fragmented MP4. The pssh atoms can appear inside the fragments, so > the muxer should see the new pssh atoms and add them to the > current/next fragment while muxing. The best way I see is to check if > the side data exists, handle it, and remove the side data. The > alternative would be to convert the side data to the > AVEncryptionInitInfo struct at every step, then compare each element > against a copy the muxer has. This is extremely slow and requires > storing the init data several different ways. > > Another alternative would be to put the side data on the frames, but > this doesn't seem right either. The init info is "header" data, so it > seems weird to put it on a random frame, and putting the data on every > frame would be more duplication and require the muxer/app to compare > them for every frame to detect new init info. > Does that make sense? Is this something that could be merged? > > > > [...] > > -- > > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > > > The worst form of inequality is to try to make unequal things equal. > > -- Aristotle > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Mon, Aug 20, 2018 at 11:41:32AM -0700, Jacob Trimble wrote: > On Mon, Jul 9, 2018 at 9:57 AM Jacob Trimble <modmaker@google.com> wrote: > > > > On Tue, Jul 3, 2018 at 5:59 PM Michael Niedermayer > > <michael@niedermayer.cc> wrote: > > > > > > On Tue, Jul 03, 2018 at 12:14:19PM -0700, Jacob Trimble wrote: > > > > On Mon, Jul 2, 2018 at 6:07 PM Michael Niedermayer > > > > <michael@niedermayer.cc> wrote: > > > > > > > > > > On Mon, Jun 25, 2018 at 04:03:32PM -0700, Jacob Trimble wrote: > > > > > > Signed-off-by: Jacob Trimble <modmaker@google.com> > > > > > > --- > > > > > > libavformat/avformat.h | 8 ++++++++ > > > > > > libavformat/utils.c | 11 +++++++++++ > > > > > > 2 files changed, 19 insertions(+) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > diff --git a/libavformat/avformat.h b/libavformat/avformat.h > > > > > > index fdaffa5bf4..434c88837e 100644 > > > > > > --- a/libavformat/avformat.h > > > > > > +++ b/libavformat/avformat.h > > > > > > @@ -2167,6 +2167,14 @@ AVStream *avformat_new_stream(AVFormatContext *s, const AVCodec *c); > > > > > > int av_stream_add_side_data(AVStream *st, enum AVPacketSideDataType type, > > > > > > uint8_t *data, size_t size); > > > > > > > > > > > > +/** > > > > > > + * Removes any existing side data of the given type. > > > > > > + * > > > > > > + * @param st stream > > > > > > + * @param type side information type > > > > > > + */ > > > > > > +void av_stream_remove_side_data(AVStream *st, enum AVPacketSideDataType type); > > > > > > > > > > What would use this and why ? > > > > > The commit message does not explain this > > > > > > > > > > If side data is changing it probably should be put in AVPackets or AVFrames > > > > > not the stream. > > > > > > > > > > > > > I am using this to removing the side data that contains the > > > > AVEncryptionInitInfo objects once I handle them. Since an MP4 file > > > > can contain multiple pssh atoms, there can be multiple > > > > AVEncryptionInitInfo structs. To make it easier for me, I want to > > > > remove the side data that contain them once I have handled them. This > > > > means that if the AVStream contains the side data, it is because of > > > > new init info I haven't seen. Since the pssh atoms are more "global" > > > > it makes more sense to put them in the AVStream. > > > > > > I dont fully understand but > > > If you intend to remove things while reading the "header" of a mp4 file > > > these things probably should not be in side data to begin with but be > > > internal to the demuxer. > > > > > > otherwise, after the header or outside the demuxer removal seems a "no go" > > > but i may misunderstand what you intend to do. Please explain if iam > > > totally off here with how i interpret this > > > > > > One simple API good vs. bad test btw should be to consider that theres > > > a demuxer connected to a muxer. > > > If this does not work to produce a equivalent file the API is not good > > > for example if you change side data in the AVStream in the middle between > > > outputing packets i would not expect the muxer to see this and thus not > > > be able to reproduce this in the stored file. > > > > > > Also if you mess with the demxuers side data from outside, not only > > > will this result in undefined behavior it also might be that you still > > > need it when for example seeking back to the start > > > > > > again maybe i totally misunderstand what you intend here > > > > > > > I would expect the muxer to do what I am doing, it would remove the > > side data when it handles the data so it doesn't have to keep a copy > > of all the init data it has seen. > > > > For example, consider converting fragmented MP4 into a different > > fragmented MP4. The pssh atoms can appear inside the fragments, so > > the muxer should see the new pssh atoms and add them to the > > current/next fragment while muxing. The best way I see is to check if > > the side data exists, handle it, and remove the side data. The > > alternative would be to convert the side data to the > > AVEncryptionInitInfo struct at every step, then compare each element > > against a copy the muxer has. This is extremely slow and requires > > storing the init data several different ways. > > > > Another alternative would be to put the side data on the frames, but > > this doesn't seem right either. The init info is "header" data, so it > > seems weird to put it on a random frame, and putting the data on every > > frame would be more duplication and require the muxer/app to compare > > them for every frame to detect new init info. > > > > Does that make sense? Is this something that could be merged? how does this work ? in what you describe IIUC there would be a demuxer connected to a muxer its AVPackets would be passed from one to the other, possibly though a FIFO adding arbitrary delay. now if the demxuer updates its stream side data, what will transfer this to the muxer ? they have seperate contexts. And how will this occur at the right time in relation to the packets also consider that the packets from 2 streams may be ordered differently especially if the demxuer doesnt produce packets that are correctly interleaved. i may have misunderstood what you intended, please correct me if that is so also if the muxer removes stream side data elements from its context nothing removes them from the demuxers context where new ones would be added. I am not sure all user apps would handle this the same thanks [...]
diff --git a/libavformat/avformat.h b/libavformat/avformat.h index fdaffa5bf4..434c88837e 100644 --- a/libavformat/avformat.h +++ b/libavformat/avformat.h @@ -2167,6 +2167,14 @@ AVStream *avformat_new_stream(AVFormatContext *s, const AVCodec *c); int av_stream_add_side_data(AVStream *st, enum AVPacketSideDataType type, uint8_t *data, size_t size); +/** + * Removes any existing side data of the given type. + * + * @param st stream + * @param type side information type + */ +void av_stream_remove_side_data(AVStream *st, enum AVPacketSideDataType type); + /** * Allocate new information from stream. * diff --git a/libavformat/utils.c b/libavformat/utils.c index c9cdd2b470..4f7c408d93 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -5491,6 +5491,17 @@ int av_stream_add_side_data(AVStream *st, enum AVPacketSideDataType type, return 0; } +void av_stream_remove_side_data(AVStream *st, enum AVPacketSideDataType type) +{ + for (int i = 0; i < st->nb_side_data; i++) { + if (st->side_data[i].type == type) { + av_freep(&st->side_data[i].data); + st->side_data[i] = st->side_data[st->nb_side_data - 1]; + st->nb_side_data--; + } + } +} + uint8_t *av_stream_new_side_data(AVStream *st, enum AVPacketSideDataType type, int size) {
Signed-off-by: Jacob Trimble <modmaker@google.com> --- libavformat/avformat.h | 8 ++++++++ libavformat/utils.c | 11 +++++++++++ 2 files changed, 19 insertions(+)