Re: [FFmpeg-devel] [PATCH 3/5] avcodec/bsf: Add list BSF API

2016-08-05 Thread Jan Sebechlebsky 



On 08/02/2016 05:26 PM, James Almer wrote:

On 8/2/2016 12:14 PM, Nicolas George wrote:

+AVBSFList *av_bsf_list_alloc(void);

This is personal, but for new APIs, I like "int foo_alloc(Foo **rfoo)"
better than "Foo *foo_alloc(void)": that way, the caller can forward the
error code instead of guessing it is ENOMEM.

There is no other error this could generate. It's literally an
av_mallocz wrapper.
Every other similar alloc() function in avcodec.h (with no extra
parameters that could generate EINVAL errors and such) also have
this signature, so lets keep try to keep it consistent.
___

I'll keep it this way then :)

Regards,
Jan

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/5] avcodec/bsf: Add list BSF API

2016-08-05 Thread Jan Sebechlebsky 


Hello Nicolas,

On 08/02/2016 05:14 PM, Nicolas George wrote:



+AVBSFList *av_bsf_list_alloc(void);

This is personal, but for new APIs, I like "int foo_alloc(Foo **rfoo)"
better than "Foo *foo_alloc(void)": that way, the caller can forward the
error code instead of guessing it is ENOMEM.

I left that as it is, since James commented this is more common way.



+
+typedef struct AVBSFList {
+AVBSFContext **bsf_lst;
+int ctx_nr;
+} AVBSFList;

I think it would be possible to use the same structure for AVBSFList and
BSFListContext. Sure, AVBSFList would have extra fields, but that is no
matter, since its only use is to be eventually upgraded into a
BSFListContext.

The benefit would be to avoid a malloc()/copy/free() cycle: just declare
.priv_data_size = 0 to prevent lavc from allocating the private structure,
and set priv_data directly.
I think it is nicer the way it is - I do not like an idea of manually 
assigning private data when priv_data_size will be set to 0, it seems 
like something which can potentially create problem in future. Also the 
overhead is negligible here, since bsf list finalization will be done 
only once or few times per whole transcoding.


I've fixed all other issues you suggested.

Thanks for review!

Regards,
Jan
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/5] avcodec/bsf: Add list BSF API

2016-08-02 Thread Timothy Gu 
Sorry, I seem to have lost the original mail, and as Gmane is dead I had to
review using Nicolas's mail.

On Tue, Aug 2, 2016 at 8:14 AM Nicolas George  wrote:

> Le primidi 11 thermidor, an CCXXIV, sebechlebsky...@gmail.com a écrit :
> > diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> > index 36f7935..39106ee 100644
> > --- a/libavcodec/avcodec.h
> > +++ b/libavcodec/avcodec.h
> > @@ -5949,6 +5949,80 @@ void av_bsf_free(AVBSFContext **ctx);
> >   */
> >  const AVClass *av_bsf_get_class(void);
> >
> > +/**
> > + * Structure for chain/list of bitstream filters.
> > + * Empty list can be allocated by av_bsf_list_alloc().
> > + */
> > +typedef struct AVBSFList AVBSFList;
> > +
> > +/**
> > + * Allocate empty list of bitstream filters.
> > + * The list must be later freed by av_bsf_list_free()
> > + * or finalized by av_bsf_list_finalize().
> > + *
> > + * @return pointer to AVBSFList on success, NULL in case of failure
>

For  Doxygen documentation, capitalize the first letter of the descriptions
in @return's and @param's, and @ref-erence types, structs, and enums. (For
functions, an empty set of parentheses is enough.)

The same rules apply to all the new functions you've added.

@return Pointer to @ref AVBSFList on success, NULL in case of failure.


> > + */
> > +AVBSFList *av_bsf_list_alloc(void);
> > +
> > +/**
> > + * Free list of bitstream filters.
> > + *
> > + * @param lst pointer to pointer returned by av_bsf_list_alloc()
>

@param lst Pointer to pointer returned by av_bsf_list_alloc()

> + */
> > +void av_bsf_list_free(AVBSFList **lst);
> > +
> > +/**
> > + * Append bitstream filter to the list of bitstream filters.
> > + *
>


> > + * @param lst list to append to
>

List


> > + * @param bsf AVBSFContext to be appended
>

@ref AVBSFContext


> > + *
> > + * @return >=0 on success, negative AVERROR in case of failure
> > + */
> > +int av_bsf_list_append(AVBSFList *lst, AVBSFContext *bsf);
> > +
> > +/**
> > + * Finalize list of bitstream filters.
> > + *
> > + * This function will transform AVBSFList to single AVBSFContext,
>

@ref's


> > + * so the whole chain of bitstream filters can be treated as single
> filter
> > + * freshly allocated by av_bsf_alloc().
>


> > + * The AVBSFList structure is destroyed after this call and must not
>

Ditto


> > + * be accessed.
> > + *
> > + * @param lst  AVBSFList structure to be transformed
>


> > + * @param bsf[out] pointer to be set to newly created AVBSFContext
> structure
>

@param[out] bsf

Same applies to rest of the file.


> > + * representing the chain of bitstream filters
> > + *
> > + * @return >=0 on success, negative AVERROR in case of failure
> > + */
> > +int av_bsf_list_finalize(AVBSFList **lst, AVBSFContext **bsf);
> > +
> > +/**
> > + * Parse string describing list of bitstream filters and create single
> > + * AVBSFContext describing the whole chain of bitstream filters.
> > + * Resulting AVBSFContext can be treated as any other AVBSFContext
> freshly
> > + * allocated by av_bsf_alloc().
> > + *
> > + * @param str  string describing chain of bitstream filters in
> format
>


> > + * bsf1[=opt1=val1:opt2=val2][,bsf2]
>

Use Markdown backtick syntax to make it display in monospace font

`bsf[=opt1=val1:opt2=val2][,bsf2]`


> > + * @param bsf[out] pointer to be set to newly created AVBSFContext
> structure
> > + * representing the chain of bitstream filters
> > + *
> > + * @return >=0 on success, negative AVERROR in case of failure
> > + */
> > +int av_bsf_list_parse_str(const char *str, AVBSFContext **bsf);
> > +
> > +/**
> > + * Get null/pass-through bitstream filter.
> > + *

> + * @param bsf[out] pointer to be set to new instance of pass-through
> > + * bitstream filter

> + *
> > + * @return
> > + */
> > +int av_bsf_get_null_filter(AVBSFContext **bsf);
> > +
> >  /* memory */
> >
> >  /**
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/5] avcodec/bsf: Add list BSF API

2016-08-02 Thread James Almer 
On 8/2/2016 12:14 PM, Nicolas George wrote:
>> +AVBSFList *av_bsf_list_alloc(void);
> This is personal, but for new APIs, I like "int foo_alloc(Foo **rfoo)"
> better than "Foo *foo_alloc(void)": that way, the caller can forward the
> error code instead of guessing it is ENOMEM.

There is no other error this could generate. It's literally an
av_mallocz wrapper.
Every other similar alloc() function in avcodec.h (with no extra
parameters that could generate EINVAL errors and such) also have
this signature, so lets keep try to keep it consistent.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 3/5] avcodec/bsf: Add list BSF API

2016-08-02 Thread Nicolas George 
Le primidi 11 thermidor, an CCXXIV, sebechlebsky...@gmail.com a écrit :
> From: Jan Sebechlebsky 
> 
> ---
>  libavcodec/avcodec.h |  74 ++
>  libavcodec/bsf.c | 284 
> +++
>  2 files changed, 358 insertions(+)

Looks rather nice. Comments below.

> 
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 36f7935..39106ee 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -5949,6 +5949,80 @@ void av_bsf_free(AVBSFContext **ctx);
>   */
>  const AVClass *av_bsf_get_class(void);
>  
> +/**
> + * Structure for chain/list of bitstream filters.
> + * Empty list can be allocated by av_bsf_list_alloc().
> + */
> +typedef struct AVBSFList AVBSFList;
> +
> +/**
> + * Allocate empty list of bitstream filters.
> + * The list must be later freed by av_bsf_list_free()
> + * or finalized by av_bsf_list_finalize().
> + *
> + * @return pointer to AVBSFList on success, NULL in case of failure
> + */

> +AVBSFList *av_bsf_list_alloc(void);

This is personal, but for new APIs, I like "int foo_alloc(Foo **rfoo)"
better than "Foo *foo_alloc(void)": that way, the caller can forward the
error code instead of guessing it is ENOMEM.

> +
> +/**
> + * Free list of bitstream filters.
> + *
> + * @param lst pointer to pointer returned by av_bsf_list_alloc()
> + */
> +void av_bsf_list_free(AVBSFList **lst);
> +

> +/**
> + * Append bitstream filter to the list of bitstream filters.
> + *
> + * @param lst list to append to
> + * @param bsf AVBSFContext to be appended
> + *
> + * @return >=0 on success, negative AVERROR in case of failure
> + */
> +int av_bsf_list_append(AVBSFList *lst, AVBSFContext *bsf);

An utility function that combines av_bsf_get_by_name(), av_bsf_alloc() and
this av_bsf_list_append() may be nice to have too.

> +
> +/**
> + * Finalize list of bitstream filters.
> + *
> + * This function will transform AVBSFList to single AVBSFContext,
> + * so the whole chain of bitstream filters can be treated as single filter
> + * freshly allocated by av_bsf_alloc().
> + * The AVBSFList structure is destroyed after this call and must not
> + * be accessed.
> + *

> + * @param lst  AVBSFList structure to be transformed
> + * @param bsf[out] pointer to be set to newly created AVBSFContext structure
> + * representing the chain of bitstream filters

Maybe document that lst no longer belongs to the caller if the call succeeds
and what happens to it if it fails.

> + *
> + * @return >=0 on success, negative AVERROR in case of failure
> + */
> +int av_bsf_list_finalize(AVBSFList **lst, AVBSFContext **bsf);
> +
> +/**
> + * Parse string describing list of bitstream filters and create single
> + * AVBSFContext describing the whole chain of bitstream filters.
> + * Resulting AVBSFContext can be treated as any other AVBSFContext freshly
> + * allocated by av_bsf_alloc().
> + *
> + * @param str  string describing chain of bitstream filters in format
> + * bsf1[=opt1=val1:opt2=val2][,bsf2]
> + * @param bsf[out] pointer to be set to newly created AVBSFContext structure
> + * representing the chain of bitstream filters
> + *
> + * @return >=0 on success, negative AVERROR in case of failure
> + */
> +int av_bsf_list_parse_str(const char *str, AVBSFContext **bsf);
> +
> +/**
> + * Get null/pass-through bitstream filter.
> + *
> + * @param bsf[out] pointer to be set to new instance of pass-through
> + * bitstream filter
> + *
> + * @return
> + */
> +int av_bsf_get_null_filter(AVBSFContext **bsf);
> +
>  /* memory */
>  
>  /**
> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
> index 40fc925..3ae0a2b 100644
> --- a/libavcodec/bsf.c
> +++ b/libavcodec/bsf.c
> @@ -22,6 +22,7 @@
>  #include "libavutil/mem.h"
>  #include "libavutil/opt.h"
>  #include "libavutil/avassert.h"
> +#include "libavutil/avstring.h"
>  
>  #include "avcodec.h"
>  #include "bsf.h"
> @@ -236,3 +237,286 @@ int ff_bsf_get_packet_ref(AVBSFContext *ctx, AVPacket 
> *pkt)
>  
>  return 0;
>  }
> +
> +typedef struct BSFListContext {
> +const AVClass *class;
> +

> +AVBSFContext **bsf_lst;
> +int ctx_nr;

The existing code uses "foo" and "nb_foo", or sometimes "foo" and
"foo_count" in this kind of case.

> +
> +int idx;   // index of currently processed BSF
> +int flushed_idx;   // index of BSF being flushed

Also, probably better make these unsigned.

> +
> +} BSFListContext;
> +
> +
> +static int bsf_list_init(AVBSFContext *bsf)
> +{
> +BSFListContext *lst = bsf->priv_data;
> +int ret, i;
> +const AVCodecParameters *cod_par = bsf->par_in;
> +AVRational tb = bsf->time_base_in;
> +
> +for (i = 0; i < lst->ctx_nr; ++i) {
> +ret = avcodec_parameters_copy(lst->bsf_lst[i]->par_in, cod_par);
> +if (ret < 0)
> +goto fail;
> +
> +lst->bsf_lst[i]->time_base_in = tb;
> +
> +ret =

[FFmpeg-devel] [PATCH 3/5] avcodec/bsf: Add list BSF API

2016-07-28 Thread sebechlebskyjan 
From: Jan Sebechlebsky 

---
 libavcodec/avcodec.h |  74 ++
 libavcodec/bsf.c | 284 +++
 2 files changed, 358 insertions(+)

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 36f7935..39106ee 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -5949,6 +5949,80 @@ void av_bsf_free(AVBSFContext **ctx);
  */
 const AVClass *av_bsf_get_class(void);
 
+/**
+ * Structure for chain/list of bitstream filters.
+ * Empty list can be allocated by av_bsf_list_alloc().
+ */
+typedef struct AVBSFList AVBSFList;
+
+/**
+ * Allocate empty list of bitstream filters.
+ * The list must be later freed by av_bsf_list_free()
+ * or finalized by av_bsf_list_finalize().
+ *
+ * @return pointer to AVBSFList on success, NULL in case of failure
+ */
+AVBSFList *av_bsf_list_alloc(void);
+
+/**
+ * Free list of bitstream filters.
+ *
+ * @param lst pointer to pointer returned by av_bsf_list_alloc()
+ */
+void av_bsf_list_free(AVBSFList **lst);
+
+/**
+ * Append bitstream filter to the list of bitstream filters.
+ *
+ * @param lst list to append to
+ * @param bsf AVBSFContext to be appended
+ *
+ * @return >=0 on success, negative AVERROR in case of failure
+ */
+int av_bsf_list_append(AVBSFList *lst, AVBSFContext *bsf);
+
+/**
+ * Finalize list of bitstream filters.
+ *
+ * This function will transform AVBSFList to single AVBSFContext,
+ * so the whole chain of bitstream filters can be treated as single filter
+ * freshly allocated by av_bsf_alloc().
+ * The AVBSFList structure is destroyed after this call and must not
+ * be accessed.
+ *
+ * @param lst  AVBSFList structure to be transformed
+ * @param bsf[out] pointer to be set to newly created AVBSFContext structure
+ * representing the chain of bitstream filters
+ *
+ * @return >=0 on success, negative AVERROR in case of failure
+ */
+int av_bsf_list_finalize(AVBSFList **lst, AVBSFContext **bsf);
+
+/**
+ * Parse string describing list of bitstream filters and create single
+ * AVBSFContext describing the whole chain of bitstream filters.
+ * Resulting AVBSFContext can be treated as any other AVBSFContext freshly
+ * allocated by av_bsf_alloc().
+ *
+ * @param str  string describing chain of bitstream filters in format
+ * bsf1[=opt1=val1:opt2=val2][,bsf2]
+ * @param bsf[out] pointer to be set to newly created AVBSFContext structure
+ * representing the chain of bitstream filters
+ *
+ * @return >=0 on success, negative AVERROR in case of failure
+ */
+int av_bsf_list_parse_str(const char *str, AVBSFContext **bsf);
+
+/**
+ * Get null/pass-through bitstream filter.
+ *
+ * @param bsf[out] pointer to be set to new instance of pass-through
+ * bitstream filter
+ *
+ * @return
+ */
+int av_bsf_get_null_filter(AVBSFContext **bsf);
+
 /* memory */
 
 /**
diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c
index 40fc925..3ae0a2b 100644
--- a/libavcodec/bsf.c
+++ b/libavcodec/bsf.c
@@ -22,6 +22,7 @@
 #include "libavutil/mem.h"
 #include "libavutil/opt.h"
 #include "libavutil/avassert.h"
+#include "libavutil/avstring.h"
 
 #include "avcodec.h"
 #include "bsf.h"
@@ -236,3 +237,286 @@ int ff_bsf_get_packet_ref(AVBSFContext *ctx, AVPacket 
*pkt)
 
 return 0;
 }
+
+typedef struct BSFListContext {
+const AVClass *class;
+
+AVBSFContext **bsf_lst;
+int ctx_nr;
+
+int idx;   // index of currently processed BSF
+int flushed_idx;   // index of BSF being flushed
+
+} BSFListContext;
+
+
+static int bsf_list_init(AVBSFContext *bsf)
+{
+BSFListContext *lst = bsf->priv_data;
+int ret, i;
+const AVCodecParameters *cod_par = bsf->par_in;
+AVRational tb = bsf->time_base_in;
+
+for (i = 0; i < lst->ctx_nr; ++i) {
+ret = avcodec_parameters_copy(lst->bsf_lst[i]->par_in, cod_par);
+if (ret < 0)
+goto fail;
+
+lst->bsf_lst[i]->time_base_in = tb;
+
+ret = av_bsf_init(lst->bsf_lst[i]);
+if (ret < 0)
+goto fail;
+
+cod_par = lst->bsf_lst[i]->par_out;
+tb = lst->bsf_lst[i]->time_base_out;
+}
+
+bsf->time_base_out = tb;
+ret = avcodec_parameters_copy(bsf->par_out, cod_par);
+
+fail:
+return ret;
+}
+
+static int bsf_list_flush(AVBSFContext *bsf, AVPacket *out)
+{
+BSFListContext *lst = bsf->priv_data;
+int ret;
+
+if (lst->flushed_idx == lst->ctx_nr)
+return AVERROR_EOF;
+
+while (1) {
+if (lst->idx > lst->flushed_idx) {
+ret = av_bsf_receive_packet(lst->bsf_lst[lst->idx-1], out);
+if (ret == AVERROR(EAGAIN)) {
+ret = 0;
+lst->idx--;
+continue;
+} else if (ret == AVERROR_EOF) {
+/* filter idx-1 is done, let's flush filter idx */
+lst->flushed_idx = lst->idx;
+continue;
+} else if (ret < 0) {
+/*