| Message ID | 20180326180239.2944-1-jamrial@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Mar 26, 2018 at 03:02:35PM -0300, James Almer wrote: > Based on a patch by Luca Barbato. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > libavformat/internal.h | 35 ++++++++++++++++++++++++++++++++++ > libavformat/utils.c | 51 +++++++++++++++++++++++++++----------------------- > 2 files changed, 63 insertions(+), 23 deletions(-) > > diff --git a/libavformat/internal.h b/libavformat/internal.h > index a020b1b417..7e1b24ebe6 100644 > --- a/libavformat/internal.h > +++ b/libavformat/internal.h > @@ -731,6 +731,41 @@ int ff_unlock_avformat(void); > */ > void ff_format_set_url(AVFormatContext *s, char *url); > > +/** > + * Append an AVPacket to the list. > + * > + * @param head List head > + * @param tail List tail about the terminology Shouldnt this be a single List element or 2 ListEntry elements ? > + * @param pkt The packet being appended > + * @param ref Create a new reference for the packet instead of > + transferring the ownership of the existing one to the > + * list. > + * @return < 0 on failure and 0 on success if its an AVERROR code on failure this should be documented, ATM it just says some unspecifified negative value > + */ > +int ff_packet_list_put(AVPacketList **head, AVPacketList **tail, > + AVPacket *pkt, int ref); ref should not be a int but a enum ff_packet_list_put(,,,1) ff_packet_list_put(,,,0) is alot more confusing to a new developer than with english words the code should be self explanatory not requiring looking up the documentation thx [...]
On 3/27/2018 5:22 PM, Michael Niedermayer wrote: > On Mon, Mar 26, 2018 at 03:02:35PM -0300, James Almer wrote: >> Based on a patch by Luca Barbato. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> libavformat/internal.h | 35 ++++++++++++++++++++++++++++++++++ >> libavformat/utils.c | 51 +++++++++++++++++++++++++++----------------------- >> 2 files changed, 63 insertions(+), 23 deletions(-) >> >> diff --git a/libavformat/internal.h b/libavformat/internal.h >> index a020b1b417..7e1b24ebe6 100644 >> --- a/libavformat/internal.h >> +++ b/libavformat/internal.h >> @@ -731,6 +731,41 @@ int ff_unlock_avformat(void); >> */ >> void ff_format_set_url(AVFormatContext *s, char *url); >> >> +/** >> + * Append an AVPacket to the list. >> + * > >> + * @param head List head >> + * @param tail List tail > > about the terminology > Shouldnt this be a single List element or 2 ListEntry elements ? What do you suggest to write instead? I don't think the current terminology is confusing. It's two AVPacketList arguments, one pointing to the head/first item of the list and the other to the tail/last item. > > >> + * @param pkt The packet being appended >> + * @param ref Create a new reference for the packet instead of >> + transferring the ownership of the existing one to the >> + * list. > >> + * @return < 0 on failure and 0 on success > > if its an AVERROR code on failure this should be documented, ATM it just > says some unspecifified negative value Ok. > > >> + */ >> +int ff_packet_list_put(AVPacketList **head, AVPacketList **tail, >> + AVPacket *pkt, int ref); > > ref should not be a int but a enum > ff_packet_list_put(,,,1) > ff_packet_list_put(,,,0) > > is alot more confusing to a new developer than with english words > the code should be self explanatory not requiring looking up the > documentation Adding an enum like this seems extreme for an internal API, but ok. > > thx > > [...] > > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Tue, Mar 27, 2018 at 06:17:09PM -0300, James Almer wrote: > On 3/27/2018 5:22 PM, Michael Niedermayer wrote: > > On Mon, Mar 26, 2018 at 03:02:35PM -0300, James Almer wrote: > >> Based on a patch by Luca Barbato. > >> > >> Signed-off-by: James Almer <jamrial@gmail.com> > >> --- > >> libavformat/internal.h | 35 ++++++++++++++++++++++++++++++++++ > >> libavformat/utils.c | 51 +++++++++++++++++++++++++++----------------------- > >> 2 files changed, 63 insertions(+), 23 deletions(-) > >> > >> diff --git a/libavformat/internal.h b/libavformat/internal.h > >> index a020b1b417..7e1b24ebe6 100644 > >> --- a/libavformat/internal.h > >> +++ b/libavformat/internal.h > >> @@ -731,6 +731,41 @@ int ff_unlock_avformat(void); > >> */ > >> void ff_format_set_url(AVFormatContext *s, char *url); > >> > >> +/** > >> + * Append an AVPacket to the list. > >> + * > > > >> + * @param head List head > >> + * @param tail List tail > > > > about the terminology > > Shouldnt this be a single List element or 2 ListEntry elements ? > > What do you suggest to write instead? I don't think the current > terminology is confusing. It's two AVPacketList arguments, one pointing > to the head/first item of the list and the other to the tail/last item. hmm i guess the simplest would be @param head List head element. It feels a bit inelegant to have to keep track of 2 elements but changing this is probably too much work [...] > > > > > >> + */ > >> +int ff_packet_list_put(AVPacketList **head, AVPacketList **tail, > >> + AVPacket *pkt, int ref); > > > > ref should not be a int but a enum > > ff_packet_list_put(,,,1) > > ff_packet_list_put(,,,0) > > > > is alot more confusing to a new developer than with english words > > the code should be self explanatory not requiring looking up the > > documentation > > Adding an enum like this seems extreme for an internal API, but ok. yes, i was thinking a bit ahead. For a internal API it is not that important but this API is quite usefull i think theres a good chance it might become public in the future. But even for a internal API named constants seem better IMHO thx [...]
diff --git a/libavformat/internal.h b/libavformat/internal.h index a020b1b417..7e1b24ebe6 100644 --- a/libavformat/internal.h +++ b/libavformat/internal.h @@ -731,6 +731,41 @@ int ff_unlock_avformat(void); */ void ff_format_set_url(AVFormatContext *s, char *url); +/** + * Append an AVPacket to the list. + * + * @param head List head + * @param tail List tail + * @param pkt The packet being appended + * @param ref Create a new reference for the packet instead of + transferring the ownership of the existing one to the + * list. + * @return < 0 on failure and 0 on success + */ +int ff_packet_list_put(AVPacketList **head, AVPacketList **tail, + AVPacket *pkt, int ref); + +/** + * Remove the oldest AVPacket in the list and return it. + * + * @note The pkt will be overwritten completely. The caller owns the + * packet and must unref it by itself. + * + * @param head List head. + * @param tail List tail. + * @param pkt Pointer to an initialized AVPacket struct + */ +int ff_packet_list_get(AVPacketList **head, AVPacketList **tail, + AVPacket *pkt); + +/** + * Wipe the list and unref all the packets in it. + * + * @param head List head. + * @param tail List tail + */ +void ff_packet_list_free(AVPacketList **head, AVPacketList **tail); + #if FF_API_NEXT /** * Register devices in deprecated format linked list. diff --git a/libavformat/utils.c b/libavformat/utils.c index f13c8208b1..cb1ea5b386 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -444,8 +444,9 @@ static int init_input(AVFormatContext *s, const char *filename, s, 0, s->format_probesize); } -static int add_to_pktbuf(AVPacketList **packet_buffer, AVPacket *pkt, - AVPacketList **plast_pktl, int ref) +int ff_packet_list_put(AVPacketList **packet_buffer, + AVPacketList **plast_pktl, + AVPacket *pkt, int ref) { AVPacketList *pktl = av_mallocz(sizeof(AVPacketList)); int ret; @@ -485,9 +486,9 @@ int avformat_queue_attached_pictures(AVFormatContext *s) continue; } - ret = add_to_pktbuf(&s->internal->raw_packet_buffer, - &s->streams[i]->attached_pic, - &s->internal->raw_packet_buffer_end, 1); + ret = ff_packet_list_put(&s->internal->raw_packet_buffer, + &s->internal->raw_packet_buffer_end, + &s->streams[i]->attached_pic, 1); if (ret < 0) return ret; } @@ -913,8 +914,9 @@ int ff_read_packet(AVFormatContext *s, AVPacket *pkt) if (!pktl && st->request_probe <= 0) return ret; - err = add_to_pktbuf(&s->internal->raw_packet_buffer, pkt, - &s->internal->raw_packet_buffer_end, 0); + err = ff_packet_list_put(&s->internal->raw_packet_buffer, + &s->internal->raw_packet_buffer_end, + pkt, 0); if (err) return err; s->internal->raw_packet_buffer_remaining_size -= pkt->size; @@ -1412,7 +1414,7 @@ FF_ENABLE_DEPRECATION_WARNINGS #endif } -static void free_packet_buffer(AVPacketList **pkt_buf, AVPacketList **pkt_buf_end) +void ff_packet_list_free(AVPacketList **pkt_buf, AVPacketList **pkt_buf_end) { while (*pkt_buf) { AVPacketList *pktl = *pkt_buf; @@ -1504,8 +1506,9 @@ static int parse_packet(AVFormatContext *s, AVPacket *pkt, int stream_index) compute_pkt_fields(s, st, st->parser, &out_pkt, next_dts, next_pts); - ret = add_to_pktbuf(&s->internal->parse_queue, &out_pkt, - &s->internal->parse_queue_end, 1); + ret = ff_packet_list_put(&s->internal->parse_queue, + &s->internal->parse_queue_end, + &out_pkt, 1); av_packet_unref(&out_pkt); if (ret < 0) goto fail; @@ -1522,9 +1525,9 @@ fail: return ret; } -static int read_from_packet_buffer(AVPacketList **pkt_buffer, - AVPacketList **pkt_buffer_end, - AVPacket *pkt) +int ff_packet_list_get(AVPacketList **pkt_buffer, + AVPacketList **pkt_buffer_end, + AVPacket *pkt) { AVPacketList *pktl; av_assert0(*pkt_buffer); @@ -1670,7 +1673,7 @@ FF_ENABLE_DEPRECATION_WARNINGS } if (!got_packet && s->internal->parse_queue) - ret = read_from_packet_buffer(&s->internal->parse_queue, &s->internal->parse_queue_end, pkt); + ret = ff_packet_list_get(&s->internal->parse_queue, &s->internal->parse_queue_end, pkt); if (ret >= 0) { AVStream *st = s->streams[pkt->stream_index]; @@ -1749,7 +1752,7 @@ int av_read_frame(AVFormatContext *s, AVPacket *pkt) if (!genpts) { ret = s->internal->packet_buffer - ? read_from_packet_buffer(&s->internal->packet_buffer, + ? ff_packet_list_get(&s->internal->packet_buffer, &s->internal->packet_buffer_end, pkt) : read_frame_internal(s, pkt); if (ret < 0) @@ -1798,7 +1801,7 @@ int av_read_frame(AVFormatContext *s, AVPacket *pkt) st = s->streams[next_pkt->stream_index]; if (!(next_pkt->pts == AV_NOPTS_VALUE && st->discard < AVDISCARD_ALL && next_pkt->dts != AV_NOPTS_VALUE && !eof)) { - ret = read_from_packet_buffer(&s->internal->packet_buffer, + ret = ff_packet_list_get(&s->internal->packet_buffer, &s->internal->packet_buffer_end, pkt); goto return_packet; } @@ -1813,8 +1816,9 @@ int av_read_frame(AVFormatContext *s, AVPacket *pkt) return ret; } - ret = add_to_pktbuf(&s->internal->packet_buffer, pkt, - &s->internal->packet_buffer_end, 1); + ret = ff_packet_list_put(&s->internal->packet_buffer, + &s->internal->packet_buffer_end, + pkt, 1); av_packet_unref(pkt); if (ret < 0) return ret; @@ -1841,9 +1845,9 @@ static void flush_packet_queue(AVFormatContext *s) { if (!s->internal) return; - free_packet_buffer(&s->internal->parse_queue, &s->internal->parse_queue_end); - free_packet_buffer(&s->internal->packet_buffer, &s->internal->packet_buffer_end); - free_packet_buffer(&s->internal->raw_packet_buffer, &s->internal->raw_packet_buffer_end); + ff_packet_list_free(&s->internal->parse_queue, &s->internal->parse_queue_end); + ff_packet_list_free(&s->internal->packet_buffer, &s->internal->packet_buffer_end); + ff_packet_list_free(&s->internal->raw_packet_buffer, &s->internal->raw_packet_buffer_end); s->internal->raw_packet_buffer_remaining_size = RAW_PACKET_BUFFER_SIZE; } @@ -3743,8 +3747,9 @@ FF_ENABLE_DEPRECATION_WARNINGS pkt = &pkt1; if (!(ic->flags & AVFMT_FLAG_NOBUFFER)) { - ret = add_to_pktbuf(&ic->internal->packet_buffer, pkt, - &ic->internal->packet_buffer_end, 0); + ret = ff_packet_list_put(&ic->internal->packet_buffer, + &ic->internal->packet_buffer_end, + pkt, 0); if (ret < 0) goto find_stream_info_err; }
Based on a patch by Luca Barbato. Signed-off-by: James Almer <jamrial@gmail.com> --- libavformat/internal.h | 35 ++++++++++++++++++++++++++++++++++ libavformat/utils.c | 51 +++++++++++++++++++++++++++----------------------- 2 files changed, 63 insertions(+), 23 deletions(-)