Add an OpenH264 decoder wrapper

diff mbox

On Sat, 25 Jun 2016, Anton Khirnov wrote:

> Quoting Martin Storsjö (2016-06-24 23:08:13)
>> On Fri, 24 Jun 2016, Diego Biurrun wrote:
>> 
>> > On Fri, Jun 24, 2016 at 10:00:31PM +0300, Martin Storsjö wrote:
>> >> On Fri, 24 Jun 2016, Diego Biurrun wrote:
>> >> > On Fri, Jun 24, 2016 at 01:07:08AM +0300, Martin Storsjö wrote:
>> >> >> --- /dev/null
>> >> >> +++ b/libavcodec/libopenh264.h
>> >> >> @@ -0,0 +1,39 @@
>> >> >> +
>> >> >> +#ifndef AVCODEC_LIBOPENH264_H
>> >> >> +#define AVCODEC_LIBOPENH264_H
>> >> >> +
>> >> >> +#endif
>> >> >
>> >> > missing #endif comment
>> >> 
>> >> ... and if I fix that?
>> >> 
>> >> A "LGTM otherwise" or similar (or an explicit note saying the opposite), 
>> >> for trivial review comments like these, would be useful, to save one extra 
>> >> round of resending and review...
>> >
>> > That nit just caught my eye, so I mentioned it quickly.
>> >
>> > I have mixed feelings about this beast, so I'm abstaining from the
>> > decision of accepting it or not.  I also haven't looked at it in enough
>> > depth to say whether it's OK or not.  Hence my silence on the patch in
>> > general.
>> 
>> Ok, thanks for saying so explicitly.
>
> For myself, I see no problem with having this in, as long as there is a
> use case for it.
>
> But there should probably be a note somewhere (External libraries
> section in general.texi?) clarifying that it's not really suitable for
> general use.

Amended the patch locally with this:

libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Quoting Martin Storsjö (2016-06-25 12:41:06)
> On Sat, 25 Jun 2016, Anton Khirnov wrote:
> 
> > Quoting Martin Storsjö (2016-06-24 23:08:13)
> >> On Fri, 24 Jun 2016, Diego Biurrun wrote:
> >> 
> >> > On Fri, Jun 24, 2016 at 10:00:31PM +0300, Martin Storsjö wrote:
> >> >> On Fri, 24 Jun 2016, Diego Biurrun wrote:
> >> >> > On Fri, Jun 24, 2016 at 01:07:08AM +0300, Martin Storsjö wrote:
> >> >> >> --- /dev/null
> >> >> >> +++ b/libavcodec/libopenh264.h
> >> >> >> @@ -0,0 +1,39 @@
> >> >> >> +
> >> >> >> +#ifndef AVCODEC_LIBOPENH264_H
> >> >> >> +#define AVCODEC_LIBOPENH264_H
> >> >> >> +
> >> >> >> +#endif
> >> >> >
> >> >> > missing #endif comment
> >> >> 
> >> >> ... and if I fix that?
> >> >> 
> >> >> A "LGTM otherwise" or similar (or an explicit note saying the opposite), 
> >> >> for trivial review comments like these, would be useful, to save one extra 
> >> >> round of resending and review...
> >> >
> >> > That nit just caught my eye, so I mentioned it quickly.
> >> >
> >> > I have mixed feelings about this beast, so I'm abstaining from the
> >> > decision of accepting it or not.  I also haven't looked at it in enough
> >> > depth to say whether it's OK or not.  Hence my silence on the patch in
> >> > general.
> >> 
> >> Ok, thanks for saying so explicitly.
> >
> > For myself, I see no problem with having this in, as long as there is a
> > use case for it.
> >
> > But there should probably be a note somewhere (External libraries
> > section in general.texi?) clarifying that it's not really suitable for
> > general use.
> 
> Amended the patch locally with this:
> 
> --- a/doc/general.texi
> +++ b/doc/general.texi
> @@ -97,12 +97,19 @@ enable it.
> 
>   @section OpenH264
> 
> -Libav can make use of the OpenH264 library for H.264 encoding.
> +Libav can make use of the OpenH264 library for H.264 encoding and decoding.
> 
>   Go to @url{http://www.openh264.org/} and follow the instructions for
>   installing the library. Then pass @code{--enable-libopenh264} to configure to
>   enable it.
> 
> +For decoding, this library is much more limited than the built-in decoder
> +in libavcodec; currently, this library lacks support for decoding B-frames
> +and some other main/high profile features. (It currently only supports
> +constrained baseline profile and cabac.) Using it is mostly useful for

CABAC

> +testing and for taking advantage of Cisco's patent portfolio license
> +(@url{http://www.openh264.org/BINARY_LICENSE.txt}).
> +

Otherwise looks fine.