Re: [libav-devel] [PATCH 6/8] mpeg: Check memory allocation

2014-12-31 Thread Luca Barbato
On 31/12/14 19:13, Derek Buitenhuis wrote: On 12/31/2014 6:11 PM, Luca Barbato wrote: next_packet, premux_packet and predecode_packet implement a linked list queue in a quite confusing way. Again another item for a later set "clarify the names so people do not get so confused". That wasn't my

Re: [libav-devel] [PATCH 8/8] mpeg: Remove the fifo

2014-12-31 Thread Luca Barbato
On 31/12/14 19:27, Derek Buitenhuis wrote: On 12/31/2014 6:24 PM, Luca Barbato wrote: I can add something in 7 + 8, but I'd rather not talk about the future in a patch. Why not? The motivations behind such changes are very useful when e.g. trying to bisect an issue caused by it. Plenty of our

Re: [libav-devel] [PATCH 3/8] mpeg: Make remove_decoded_packets return void

2014-12-31 Thread Luca Barbato
On 31/12/14 19:25, Derek Buitenhuis wrote: On 12/31/2014 6:23 PM, Luca Barbato wrote: I do agree, shall we error out in the case? (I expect angry mobs since the lack of few audio sample is not as annoying as not being able to mux at all) [...] I'm open to alternatives. We can: 1) Change A

Re: [libav-devel] [PATCH 8/8] mpeg: Remove the fifo

2014-12-31 Thread Derek Buitenhuis
On 12/31/2014 6:24 PM, Luca Barbato wrote: > I can add something in 7 + 8, but I'd rather not talk about the future > in a patch. Why not? The motivations behind such changes are very useful when e.g. trying to bisect an issue caused by it. Plenty of our commits have stuff like "in preparation f

Re: [libav-devel] [PATCH 3/8] mpeg: Make remove_decoded_packets return void

2014-12-31 Thread Derek Buitenhuis
On 12/31/2014 6:23 PM, Luca Barbato wrote: > I do agree, shall we error out in the case? (I expect angry mobs since > the lack of few audio sample is not as annoying as not being able to mux > at all) [...] > I'm open to alternatives. We can: 1) Change AV_LOG_ERROR to AV_LOG_WARNING, and acce

Re: [libav-devel] [PATCH 4/8] mpeg: Refactor output_packet

2014-12-31 Thread Luca Barbato
On 31/12/14 19:22, Derek Buitenhuis wrote: On 12/31/2014 5:30 PM, Luca Barbato wrote: -static int output_packet(AVFormatContext *ctx, int flush) +static int find_best_stream(AVFormatContext *ctx, You're triggering many people's PTSD here, I am sure. [...] I *think* the code seems to be OK. T

[libav-devel] [PATCH] libavcodec: Add an OpenH264 encoder wrapper

2014-12-31 Thread Martin Storsjö
Compared to existing, common opensource H264 encoders, this can be useful since it has got a different license (BSD instead of GPL). Performance- and qualitywise it is comparable to x264 in ultrafast mode. Hooking it up as an encoder in libavcodec also simplifies comparing it against other common

Re: [libav-devel] [PATCH 8/8] mpeg: Remove the fifo

2014-12-31 Thread Luca Barbato
On 31/12/14 19:18, Derek Buitenhuis wrote: On 12/31/2014 6:16 PM, Luca Barbato wrote: Spare a memcpy. What I'm writing right now tries to get access to the actual packets so it stops writing a PES if in the middle there are small I-frames. This fixes some fun situations in which your seek poin

Re: [libav-devel] [PATCH 3/8] mpeg: Make remove_decoded_packets return void

2014-12-31 Thread Luca Barbato
On 31/12/14 19:17, Derek Buitenhuis wrote: On 12/31/2014 6:07 PM, Luca Barbato wrote: I can write down the whole explanation on how this code works, be ready to lose at least 5 points of sanity. Ph'nglui mglw'nafh mpeg*.c Libav wgah'nagl fhtagn. Basically with the current code you might end

Re: [libav-devel] [PATCH 4/8] mpeg: Refactor output_packet

2014-12-31 Thread Derek Buitenhuis
On 12/31/2014 5:30 PM, Luca Barbato wrote: > -static int output_packet(AVFormatContext *ctx, int flush) > +static int find_best_stream(AVFormatContext *ctx, You're triggering many people's PTSD here, I am sure. [...] I *think* the code seems to be OK. The changes aren't super straightforward tho

Re: [libav-devel] [PATCH 5/8] mpeg: Refactor put_system_header

2014-12-31 Thread Derek Buitenhuis
On 12/31/2014 5:30 PM, Luca Barbato wrote: > --- > libavformat/mpegenc.c | 167 > +++--- > 1 file changed, 90 insertions(+), 77 deletions(-) OK. - Derek ___ libav-devel mailing list libav-devel@libav.org htt

Re: [libav-devel] [PATCH 8/8] mpeg: Remove the fifo

2014-12-31 Thread Derek Buitenhuis
On 12/31/2014 6:16 PM, Luca Barbato wrote: > Spare a memcpy. > > What I'm writing right now tries to get access to the actual packets so > it stops writing a PES if in the middle there are small I-frames. > > This fixes some fun situations in which your seek point is in the middle > of a PES pa

Re: [libav-devel] [PATCH 3/8] mpeg: Make remove_decoded_packets return void

2014-12-31 Thread Derek Buitenhuis
On 12/31/2014 6:07 PM, Luca Barbato wrote: > I can write down the whole explanation on how this code works, be ready > to lose at least 5 points of sanity. Ph'nglui mglw'nafh mpeg*.c Libav wgah'nagl fhtagn. > Basically with the current code you might end up muxing data in a weird > way so you e

Re: [libav-devel] [PATCH 8/8] mpeg: Remove the fifo

2014-12-31 Thread Luca Barbato
On 31/12/14 19:12, Derek Buitenhuis wrote: On 12/31/2014 5:30 PM, Luca Barbato wrote: --- libavformat/mpegenc.c | 52 --- 1 file changed, 29 insertions(+), 23 deletions(-) To what end? Spare a memcpy. What I'm writing right now tries to ge

Re: [libav-devel] [PATCH 6/8] mpeg: Check memory allocation

2014-12-31 Thread Derek Buitenhuis
On 12/31/2014 6:11 PM, Luca Barbato wrote: > next_packet, premux_packet and predecode_packet implement a linked list > queue in a quite confusing way. > > Again another item for a later set "clarify the names so people do not > get so confused". That wasn't my point, though. The old code guara

Re: [libav-devel] [PATCH 7/8] mpeg: Keep a reference to the packet

2014-12-31 Thread Luca Barbato
On 31/12/14 19:10, Derek Buitenhuis wrote: On 12/31/2014 5:30 PM, Luca Barbato wrote: --- libavformat/mpegenc.c | 19 ++- 1 file changed, 10 insertions(+), 9 deletions(-) You should probably explain why. It should be squashed with the following as I wrote in the opening e

Re: [libav-devel] [PATCH 8/8] mpeg: Remove the fifo

2014-12-31 Thread Derek Buitenhuis
On 12/31/2014 5:30 PM, Luca Barbato wrote: > --- > libavformat/mpegenc.c | 52 > --- > 1 file changed, 29 insertions(+), 23 deletions(-) To what end? - Derek ___ libav-devel mailing list libav-devel@liba

Re: [libav-devel] [PATCH 6/8] mpeg: Check memory allocation

2014-12-31 Thread Luca Barbato
On 31/12/14 19:07, Derek Buitenhuis wrote: On 12/31/2014 5:30 PM, Luca Barbato wrote: -*stream->next_packet = -pkt_desc = av_mallocz(sizeof(PacketDesc)); +pkt_desc = av_mallocz(sizeof(PacketDesc)); +if (!pkt_desc) +return AVERROR(ENOMEM); Is *stream-

Re: [libav-devel] [PATCH 7/8] mpeg: Keep a reference to the packet

2014-12-31 Thread Derek Buitenhuis
On 12/31/2014 5:30 PM, Luca Barbato wrote: > --- > libavformat/mpegenc.c | 19 ++- > 1 file changed, 10 insertions(+), 9 deletions(-) You should probably explain why. Also, there are some random whitespace changes later on. - Derek ___

Re: [libav-devel] [PATCH 3/8] mpeg: Make remove_decoded_packets return void

2014-12-31 Thread Luca Barbato
On 31/12/14 18:57, Derek Buitenhuis wrote: On 12/31/2014 5:30 PM, Luca Barbato wrote: It always return 0. --- libavformat/mpegenc.c | 12 +--- 1 file changed, 5 insertions(+), 7 deletions(-) Is this actually the correct fix? Possibly I see: https://github.com/libav/libav/blob/ma

Re: [libav-devel] [PATCH 6/8] mpeg: Check memory allocation

2014-12-31 Thread Derek Buitenhuis
On 12/31/2014 5:30 PM, Luca Barbato wrote: > -*stream->next_packet = > -pkt_desc = av_mallocz(sizeof(PacketDesc)); > +pkt_desc = av_mallocz(sizeof(PacketDesc)); > +if (!pkt_desc) > +return AVERROR(ENOMEM); Is *stream->next_packet already guaranteed to be

Re: [libav-devel] [PATCH 3/3] Allow loading the OpenH264 library dynamically

2014-12-31 Thread Luca Barbato
On 31/12/14 18:34, Derek Buitenhuis wrote: On 12/31/2014 5:25 PM, Luca Barbato wrote: I had those in my wishlist since a while (actually something a little more gory) since sometimes I feel the need of it =p I am liekly never going to accept to a patch to libx265.c to dynamically load arbitrar

Re: [libav-devel] [PATCH 2/3] libavcodec: Add an OpenH264 encoder wrapper

2014-12-31 Thread Derek Buitenhuis
On 12/31/2014 5:58 PM, Martin Storsjö wrote: > Ah, yes, that does seem to work. To simplify things a bit further, if > slices is > 1, that's most probably what the user wanted, so then we can > force the slice mode to "fixed number of slices" (SM_FIXEDSLCNUM_SLICE), > instead of the default (acc

Re: [libav-devel] [PATCH 1/8] avconv: Use the current packet API

2014-12-31 Thread Derek Buitenhuis
On 12/31/2014 5:30 PM, Luca Barbato wrote: > -av_free_packet(pkt); > +av_packet_unref(pkt); This is something that confused me as a downstream user. Is this correct? BEcause av_free_packet is not just a wrapper for av_packet_unref, and it's unclear by looking at the code if

Re: [libav-devel] [PATCH 2/3] libavcodec: Add an OpenH264 encoder wrapper

2014-12-31 Thread Martin Storsjö
On Wed, 31 Dec 2014, Derek Buitenhuis wrote: On 12/31/2014 3:21 PM, Martin Storsjö wrote: +param.bEnableBackgroundDetection = 1; +param.bEnableAdaptiveQuant = 1; Is there no equivalent of param_parse()? Seems like hardcoding all these vals is a bit weird. For parsing these fro

Re: [libav-devel] [PATCH 3/8] mpeg: Make remove_decoded_packets return void

2014-12-31 Thread Derek Buitenhuis
On 12/31/2014 5:30 PM, Luca Barbato wrote: > It always return 0. > --- > libavformat/mpegenc.c | 12 +--- > 1 file changed, 5 insertions(+), 7 deletions(-) Is this actually the correct fix? I see: https://github.com/libav/libav/blob/master/libavformat/mpegenc.c#L921-927 That claims to b

Re: [libav-devel] [PATCH 2/8] mpeg: Remove unused field

2014-12-31 Thread Derek Buitenhuis
On 12/31/2014 5:30 PM, Luca Barbato wrote: > --- > libavformat/mpegenc.c | 1 - > 1 file changed, 1 deletion(-) OK. - Derek ___ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 3/3] Allow loading the OpenH264 library dynamically

2014-12-31 Thread Rémi Denis-Courmont
Le 2014-12-31 20:34, Derek Buitenhuis a écrit : On 12/31/2014 5:25 PM, Luca Barbato wrote: I had those in my wishlist since a while (actually something a little more gory) since sometimes I feel the need of it =p I am liekly never going to accept to a patch to libx265.c to dynamically load a

Re: [libav-devel] [PATCH 3/3] Allow loading the OpenH264 library dynamically

2014-12-31 Thread Derek Buitenhuis
On 12/31/2014 5:25 PM, Luca Barbato wrote: > I had those in my wishlist since a while (actually something a little > more gory) since sometimes I feel the need of it =p I am liekly never going to accept to a patch to libx265.c to dynamically load arbitrary libs, and *especially* not to load both

[libav-devel] [PATCH 8/8] mpeg: Remove the fifo

2014-12-31 Thread Luca Barbato
--- libavformat/mpegenc.c | 52 --- 1 file changed, 29 insertions(+), 23 deletions(-) diff --git a/libavformat/mpegenc.c b/libavformat/mpegenc.c index 2f3636b..4b36bcd 100644 --- a/libavformat/mpegenc.c +++ b/libavformat/mpegenc.c @@ -47,7 +47,7 @@

[libav-devel] [PATCH 7/8] mpeg: Keep a reference to the packet

2014-12-31 Thread Luca Barbato
--- libavformat/mpegenc.c | 19 ++- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/libavformat/mpegenc.c b/libavformat/mpegenc.c index f7143c5..2f3636b 100644 --- a/libavformat/mpegenc.c +++ b/libavformat/mpegenc.c @@ -39,9 +39,9 @@ #include typedef struct Pack

[libav-devel] [PATCH 6/8] mpeg: Check memory allocation

2014-12-31 Thread Luca Barbato
--- libavformat/mpegenc.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/libavformat/mpegenc.c b/libavformat/mpegenc.c index 679912f..f7143c5 100644 --- a/libavformat/mpegenc.c +++ b/libavformat/mpegenc.c @@ -1145,12 +1145,16 @@ static int mpeg_mux_write_packet(AVForma

[libav-devel] [PATCH 5/8] mpeg: Refactor put_system_header

2014-12-31 Thread Luca Barbato
--- libavformat/mpegenc.c | 167 +++--- 1 file changed, 90 insertions(+), 77 deletions(-) diff --git a/libavformat/mpegenc.c b/libavformat/mpegenc.c index ee0c218..679912f 100644 --- a/libavformat/mpegenc.c +++ b/libavformat/mpegenc.c @@ -124,11 +124,98

[libav-devel] [PATCH 4/8] mpeg: Refactor output_packet

2014-12-31 Thread Luca Barbato
Make simpler to follow what it does. --- libavformat/mpegenc.c | 141 +++--- 1 file changed, 89 insertions(+), 52 deletions(-) diff --git a/libavformat/mpegenc.c b/libavformat/mpegenc.c index a243d2c..ee0c218 100644 --- a/libavformat/mpegenc.c +++ b/lib

Re: [libav-devel] [PATCH 1/3] libavcodec: Make the slice_count option settable for video encoders

2014-12-31 Thread Luca Barbato
On 31/12/14 18:26, Derek Buitenhuis wrote: On 12/31/2014 5:25 PM, Luca Barbato wrote: Shall we fix it? Should probably find out why first. That's the start of fixing it =) lu ___ libav-devel mailing list libav-devel@libav.org https://lists.libav

[libav-devel] [PATCH 3/8] mpeg: Make remove_decoded_packets return void

2014-12-31 Thread Luca Barbato
It always return 0. --- libavformat/mpegenc.c | 12 +--- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/libavformat/mpegenc.c b/libavformat/mpegenc.c index 1c68a8f..a243d2c 100644 --- a/libavformat/mpegenc.c +++ b/libavformat/mpegenc.c @@ -906,7 +906,7 @@ static void put_vcd

[libav-devel] [PATCH 2/8] mpeg: Remove unused field

2014-12-31 Thread Luca Barbato
--- libavformat/mpegenc.c | 1 - 1 file changed, 1 deletion(-) diff --git a/libavformat/mpegenc.c b/libavformat/mpegenc.c index 9640893..1c68a8f 100644 --- a/libavformat/mpegenc.c +++ b/libavformat/mpegenc.c @@ -43,7 +43,6 @@ typedef struct PacketDesc { int64_t dts; int size; int u

[libav-devel] [PATCH 1/8] avconv: Use the current packet API

2014-12-31 Thread Luca Barbato
--- avconv.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/avconv.c b/avconv.c index 8e76863..18a49f4 100644 --- a/avconv.c +++ b/avconv.c @@ -330,7 +330,7 @@ static void write_frame(AVFormatContext *s, AVPacket *pkt, OutputStream *ost) */ if (!(avctx->

[libav-devel] mpeg ps enc small cleanup

2014-12-31 Thread Luca Barbato
[PATCH 1/8] avconv: Use the current packet API Something I had pending since a while, sort of used by this set. [PATCH 2/8] mpeg: Remove unused field [PATCH 3/8] mpeg: Make remove_decoded_packets return void [PATCH 4/8] mpeg: Refactor output_packet [PATCH 5/8] mpeg: Refactor put_system_header [PAT

Re: [libav-devel] [PATCH 1/3] libavcodec: Make the slice_count option settable for video encoders

2014-12-31 Thread Derek Buitenhuis
On 12/31/2014 5:25 PM, Luca Barbato wrote: > Shall we fix it? Should probably find out why first. - Derek ___ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 1/3] libavcodec: Make the slice_count option settable for video encoders

2014-12-31 Thread Luca Barbato
On 31/12/14 18:10, Derek Buitenhuis wrote: On 12/31/2014 5:07 PM, Martin Storsjö wrote: (Why do we have two separate fields for almost the same thing?) I'm sure there's some silly historical reason. Shall we fix it? lu ___ libav-devel mailing li

Re: [libav-devel] [PATCH 3/3] Allow loading the OpenH264 library dynamically

2014-12-31 Thread Luca Barbato
On 31/12/14 17:51, Derek Buitenhuis wrote: On 12/31/2014 3:21 PM, Martin Storsjö wrote: I'm not sure if this is worthwhile doing though. It does make the code (and configure) quite a bit more messy. I am sort of inclined to agree. It's not that this patch is bad per se, but it kinda sets the

Re: [libav-devel] [PATCH 1/3] libavcodec: Make the slice_count option settable for video encoders

2014-12-31 Thread Derek Buitenhuis
On 12/31/2014 5:07 PM, Martin Storsjö wrote: > (Why do we have two separate fields for almost the same thing?) I'm sure there's some silly historical reason. - Derek ___ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/list

Re: [libav-devel] [PATCH 3/3] Allow loading the OpenH264 library dynamically

2014-12-31 Thread Timothy Gu
On Wed, Dec 31, 2014 at 9:03 AM, Martin Storsjö wrote: > It would probably indeed make sense to add a separate enable-item (so you > don't need to check individually for dlopen and LoadLibrary, but just for > the generic dynamic-loading feature) and a separate header that does this. +1. Wanted t

Re: [libav-devel] [PATCH 1/3] libavcodec: Make the slice_count option settable for video encoders

2014-12-31 Thread Martin Storsjö
On Wed, 31 Dec 2014, Derek Buitenhuis wrote: On 12/31/2014 3:21 PM, Martin Storsjö wrote: --- libavcodec/options_table.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Didn't we use avctx->slices before? See: https://github.com/libav/libav/blob/master/libavcodec/utvideoenc.c#L137-146

Re: [libav-devel] [PATCH 1/3] libavcodec: Make the slice_count option settable for video encoders

2014-12-31 Thread Derek Buitenhuis
On 12/31/2014 3:21 PM, Martin Storsjö wrote: > --- > libavcodec/options_table.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Didn't we use avctx->slices before? See: https://github.com/libav/libav/blob/master/libavcodec/utvideoenc.c#L137-146 - Derek __

Re: [libav-devel] [PATCH 3/3] Allow loading the OpenH264 library dynamically

2014-12-31 Thread Martin Storsjö
On Wed, 31 Dec 2014, Derek Buitenhuis wrote: On 12/31/2014 3:21 PM, Martin Storsjö wrote: I'm not sure if this is worthwhile doing though. It does make the code (and configure) quite a bit more messy. I am sort of inclined to agree. It's not that this patch is bad per se, but it kinda sets t

Re: [libav-devel] [PATCH 2/3] libavcodec: Add an OpenH264 encoder wrapper

2014-12-31 Thread Derek Buitenhuis
On 12/31/2014 3:21 PM, Martin Storsjö wrote: > +param.bEnableBackgroundDetection = 1; > +param.bEnableAdaptiveQuant = 1; Is there no equivalent of param_parse()? Seems like hardcoding all these vals is a bit weird. > +if (avctx->thread_count != 1) { > +SSliceConfig *cfg

Re: [libav-devel] [PATCH 3/3] Allow loading the OpenH264 library dynamically

2014-12-31 Thread Derek Buitenhuis
On 12/31/2014 3:21 PM, Martin Storsjö wrote: > I'm not sure if this is worthwhile doing though. It does make the > code (and configure) quite a bit more messy. I am sort of inclined to agree. It's not that this patch is bad per se, but it kinda sets the precedent for much worse things like dynami

Re: [libav-devel] [PATCH 1/3] libavcodec: Make the slice_count option settable for video encoders

2014-12-31 Thread Luca Barbato
On 31/12/14 16:21, Martin Storsjö wrote: --- libavcodec/options_table.h | 2 +- Ok. ___ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 3/3] Allow loading the OpenH264 library dynamically

2014-12-31 Thread Luca Barbato
On 31/12/14 16:21, Martin Storsjö wrote: This simplifies use of the patent license, simplifying use with a library that has been downloaded at runtime (making it possible to actually load and run libavcodec before the corresponding OpenH264 library exists). I'm not sure if this is worthwhile doi

Re: [libav-devel] [PATCH 2/3] libavcodec: Add an OpenH264 encoder wrapper

2014-12-31 Thread Luca Barbato
On 31/12/14 16:21, Martin Storsjö wrote: Compared to existing, common opensource H264 encoders, this can be useful since it has got a different license (BSD instead of GPL). Performance- and qualitywise it is comparable to x264 in ultrafast mode. Hooking it up as an encoder in libavcodec also s

[libav-devel] [PATCH 3/3] Allow loading the OpenH264 library dynamically

2014-12-31 Thread Martin Storsjö
This simplifies use of the patent license, simplifying use with a library that has been downloaded at runtime (making it possible to actually load and run libavcodec before the corresponding OpenH264 library exists). I'm not sure if this is worthwhile doing though. It does make the code (and confi

[libav-devel] [PATCH 1/3] libavcodec: Make the slice_count option settable for video encoders

2014-12-31 Thread Martin Storsjö
--- libavcodec/options_table.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h index 91d7ff9..83bfb47 100644 --- a/libavcodec/options_table.h +++ b/libavcodec/options_table.h @@ -223,7 +223,7 @@ static const AVOption avcod

[libav-devel] [PATCH 2/3] libavcodec: Add an OpenH264 encoder wrapper

2014-12-31 Thread Martin Storsjö
Compared to existing, common opensource H264 encoders, this can be useful since it has got a different license (BSD instead of GPL). Performance- and qualitywise it is comparable to x264 in ultrafast mode. Hooking it up as an encoder in libavcodec also simplifies comparing it against other common