Re: [FFmpeg-devel] [PATCH] matroskaenc: Add support for writing video projection.

[James Almer]

On 1/27/2017 11:21 PM, Aaron Colwell wrote:
> On Fri, Jan 27, 2017 at 5:46 PM James Almer  wrote:
> 
>> On 1/27/2017 5:12 PM, Aaron Colwell wrote:
>>> Adding support for writing spherical metadata in Matroska.
>>>
>>>
>>> 0001-matroskaenc-Add-support-for-writing-video-projection.patch
>>>
>>>
>>> From 5a9cf56bf3114186412bb5572b153f886edb6ddb Mon Sep 17 00:00:00 2001
>>> From: Aaron Colwell 
>>> Date: Fri, 27 Jan 2017 12:07:25 -0800
>>> Subject: [PATCH] matroskaenc: Add support for writing video projection
>>>  element.
>>>
>>> Adding support for writing spherical metadata in Matroska.
>>> ---
>>>  libavformat/matroskaenc.c | 64
>> +++
>>>  1 file changed, 64 insertions(+)
>>>
>>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>>> index f731b678b9..1b186db223 100644
>>> --- a/libavformat/matroskaenc.c
>>> +++ b/libavformat/matroskaenc.c
>>> @@ -1038,6 +1038,67 @@ static int mkv_write_stereo_mode(AVFormatContext
>> *s, AVIOContext *pb,
>>>  return ret;
>>>  }
>>>
>>> +static int mkv_write_video_projection(AVIOContext *pb, AVStream *st)
>>> +{
>>> +AVSphericalMapping *spherical_mapping =
>> (AVSphericalMapping*)av_stream_get_side_data(st, AV_PKT_DATA_SPHERICAL,
>> NULL);
>>> +ebml_master projection;
>>> +int projection_type = 0;
>>> +
>>> +AVIOContext *dyn_cp;
>>> +uint8_t *projection_private_ptr = NULL;
>>> +int ret, projection_private_size;
>>> +
>>> +if (!spherical_mapping)
>>> +return 0;
>>> +
>>> +if (spherical_mapping->projection != AV_SPHERICAL_EQUIRECTANGULAR &&
>>> +spherical_mapping->projection != AV_SPHERICAL_CUBEMAP) {
>>> +av_log(pb, AV_LOG_WARNING, "Unsupported projection %d.
>> projection not written.\n", spherical_mapping->projection);
>>> +return 0;
>>> +}
>>> +
>>> +ret = avio_open_dyn_buf(_cp);
>>> +if (ret < 0)
>>> +return ret;
>>> +
>>> +switch (spherical_mapping->projection) {
>>> +case AV_SPHERICAL_EQUIRECTANGULAR:
>>> +projection_type = 1;
>>> +
>>> +/* Create ProjectionPrivate contents */
>>> +avio_wb32(dyn_cp, 0); /* version = 0 & flags = 0 */
>>> +avio_wb32(dyn_cp, 0); /* projection_bounds_top */
>>> +avio_wb32(dyn_cp, 0); /* projection_bounds_bottom */
>>> +avio_wb32(dyn_cp, 0); /* projection_bounds_left */
>>> +avio_wb32(dyn_cp, 0); /* projection_bounds_right */
>>
>> You could instead use a local 20 byte array, fill it using AV_WB32() and
>> then write it with put_ebml_binary().
>>
> 
> I was mainly using this form since that is what the code would have to look
> like once AVSphericalMapping actually contained this data.

I know. I meant doing something like

uint8_t private[20];

AV_WB32(private + 0, 0);
AV_WB32(private + 4, projection_bounds_top);
AV_WB32(private + 8, projection_bounds_bottom);
AV_WB32(private + 12, projection_bounds_left);
AV_WB32(private + 16, projection_bounds_right);

put_ebml_binary(dyn_cp, MATROSKA_ID_VIDEOPROJECTIONPRIVATE, private, 
sizeof(private));

Then the same for Cubemap, but it's mostly a nit.

> 
> 
>>
>> Also, the latest change to the spec draft mentions ProjectionPrivate is
>> optional for Equirectangular projections if the contents are going to be
>> all zero.
>>
> 
> True. I could just drop this if that is preferred.
> 
> 
>>
>>> +break;
>>> +case AV_SPHERICAL_CUBEMAP:
>>> +projection_type = 2;
>>> +
>>> +/* Create ProjectionPrivate contents */
>>> +avio_wb32(dyn_cp, 0); /* version = 0 & flags = 0 */
>>> +avio_wb32(dyn_cp, 0); /* layout */
>>> +avio_wb32(dyn_cp, 0); /* padding */
>>> +break;
>>> +}
>>
>> Same, 12 byte array.
>>
>> What if the user is trying to remux a matroska file that has a
>> ProjectionPrivate element or an mp4 with an equi box filled with non zero
>> values, for that matter?
>>
> 
> yeah. I'm a little confused why the demuxing code didn't implement this to
> begin with.

Vittorio (The one that implemented AVSphericalMapping) tried to add this at
first, but then removed it because he wasn't sure if he was doing it right.

> 
> 
>> I know you're the one behind the spec in question, but wouldn't it be a
>> better idea to wait until AVSphericalMapping gets a way to propagate this
>> kind of information before adding support for muxing video projection
>> elements? Or maybe try to implement it right now...
>>
> 
> I'm happy to implement support for the projection specific info. What would
> be the best way to proceed. It seems like I could just place a union with
> projection specific structs in AVSphericalMapping. I'd also like some

I'm CCing Vittorio so he can chim in. I personally have no real preference.

> advice around how to sequence the set of changes. My preference would be to
> add the union & fields to AVSphericalMapping and update at least one
> demuxer to at least 

Re: [FFmpeg-devel] [PATCH 5/9] nistspheredec: prevent overflow during block alignment calculation

[Michael Niedermayer]

On Sat, Jan 28, 2017 at 01:26:40AM +0100, Andreas Cadhalpun wrote:
> On 27.01.2017 02:56, Marton Balint wrote:
> > I see 3 problems (wm4 explicitly named them, but I also had them in mind)
> > - Little benefit, yet
> > - Makes the code less clean, more cluttered
> > - Increases binary size
> > 
> > The ideas I proposed (use macros, use common / factorized checks for common
> > validatons and errors) might be a good compromise IMHO. Fuzzing thereforce
> > can be done with "debug" builds.
> > 
> > Anyway, I am not blocking the patch, just expressing what I would prefer in
> > the long run.
> 
> How about the following macro?
> #define FF_RETURN_ERROR(condition, error, log_ctx, ...) {   \
> if (condition) {\
> if (!CONFIG_SMALL && log_ctx)   \
> av_log(log_ctx, AV_LOG_ERROR, __VA_ARGS__); \
> return error;   \
> }   \
> }
> 
> That could be used with error message:
> FF_RETURN_ERROR(st->codecpar->channels > FF_SANE_NB_CHANNELS, 
> AVERROR(ENOSYS),
> s, "Too many channels %d > %d\n", st->codecpar->channels, 
> FF_SANE_NB_CHANNELS)
> 
> Or without:
> FF_RETURN_ERROR(st->codecpar->channels > FF_SANE_NB_CHANNELS, 
> AVERROR(ENOSYS), NULL)

I suggest
if (st->codecpar->channels > FF_SANE_NB_CHANNELS) {
ff_elog(s, "Too many channels %d > %d\n", st->codecpar->channels, 
FF_SANE_NB_CHANNELS);
return AVERROR(ENOSYS);
}

or for the 2nd example:

if (st->codecpar->channels > FF_SANE_NB_CHANNELS)
return AVERROR(ENOSYS);


ff_elog() can then be defined to nothing based on CONFIG_SMALL

the difference between my suggestion and yours is that mine has
new-lines seperating the condition, message and return and that its
self explanatory C code.

What you do is removing newlines and standard C keywords with a custom
macro that people not working on FFmpeg on a regular basis will have
difficulty understanding

The macro is similar to writing (minus the C keywords)
if (st->codecpar->channels > FF_SANE_NB_CHANNELS) { ff_elog(
s, "Too many channels %d > %d\n", st->codecpar->channels, 
FF_SANE_NB_CHANNELS); return AVERROR(ENOSYS); }

we dont do that becuause its just bad code
why does this suddenly become desirable when its in a macro?

Or maybe the question should be, does code become less cluttered and
cleaner when newlines and C keywords are removed ?
if not why is that done here ?
if yes why is it done just here ?

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you think the mosad wants you dead since a long time then you are either
wrong or dead since a long time.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] matroskaenc: Add support for writing video projection.

[Aaron Colwell]

On Fri, Jan 27, 2017 at 5:46 PM James Almer  wrote:

> On 1/27/2017 5:12 PM, Aaron Colwell wrote:
> > Adding support for writing spherical metadata in Matroska.
> >
> >
> > 0001-matroskaenc-Add-support-for-writing-video-projection.patch
> >
> >
> > From 5a9cf56bf3114186412bb5572b153f886edb6ddb Mon Sep 17 00:00:00 2001
> > From: Aaron Colwell 
> > Date: Fri, 27 Jan 2017 12:07:25 -0800
> > Subject: [PATCH] matroskaenc: Add support for writing video projection
> >  element.
> >
> > Adding support for writing spherical metadata in Matroska.
> > ---
> >  libavformat/matroskaenc.c | 64
> +++
> >  1 file changed, 64 insertions(+)
> >
> > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> > index f731b678b9..1b186db223 100644
> > --- a/libavformat/matroskaenc.c
> > +++ b/libavformat/matroskaenc.c
> > @@ -1038,6 +1038,67 @@ static int mkv_write_stereo_mode(AVFormatContext
> *s, AVIOContext *pb,
> >  return ret;
> >  }
> >
> > +static int mkv_write_video_projection(AVIOContext *pb, AVStream *st)
> > +{
> > +AVSphericalMapping *spherical_mapping =
> (AVSphericalMapping*)av_stream_get_side_data(st, AV_PKT_DATA_SPHERICAL,
> NULL);
> > +ebml_master projection;
> > +int projection_type = 0;
> > +
> > +AVIOContext *dyn_cp;
> > +uint8_t *projection_private_ptr = NULL;
> > +int ret, projection_private_size;
> > +
> > +if (!spherical_mapping)
> > +return 0;
> > +
> > +if (spherical_mapping->projection != AV_SPHERICAL_EQUIRECTANGULAR &&
> > +spherical_mapping->projection != AV_SPHERICAL_CUBEMAP) {
> > +av_log(pb, AV_LOG_WARNING, "Unsupported projection %d.
> projection not written.\n", spherical_mapping->projection);
> > +return 0;
> > +}
> > +
> > +ret = avio_open_dyn_buf(_cp);
> > +if (ret < 0)
> > +return ret;
> > +
> > +switch (spherical_mapping->projection) {
> > +case AV_SPHERICAL_EQUIRECTANGULAR:
> > +projection_type = 1;
> > +
> > +/* Create ProjectionPrivate contents */
> > +avio_wb32(dyn_cp, 0); /* version = 0 & flags = 0 */
> > +avio_wb32(dyn_cp, 0); /* projection_bounds_top */
> > +avio_wb32(dyn_cp, 0); /* projection_bounds_bottom */
> > +avio_wb32(dyn_cp, 0); /* projection_bounds_left */
> > +avio_wb32(dyn_cp, 0); /* projection_bounds_right */
>
> You could instead use a local 20 byte array, fill it using AV_WB32() and
> then write it with put_ebml_binary().
>

I was mainly using this form since that is what the code would have to look
like once AVSphericalMapping actually contained this data.


>
> Also, the latest change to the spec draft mentions ProjectionPrivate is
> optional for Equirectangular projections if the contents are going to be
> all zero.
>

True. I could just drop this if that is preferred.


>
> > +break;
> > +case AV_SPHERICAL_CUBEMAP:
> > +projection_type = 2;
> > +
> > +/* Create ProjectionPrivate contents */
> > +avio_wb32(dyn_cp, 0); /* version = 0 & flags = 0 */
> > +avio_wb32(dyn_cp, 0); /* layout */
> > +avio_wb32(dyn_cp, 0); /* padding */
> > +break;
> > +}
>
> Same, 12 byte array.
>
> What if the user is trying to remux a matroska file that has a
> ProjectionPrivate element or an mp4 with an equi box filled with non zero
> values, for that matter?
>

yeah. I'm a little confused why the demuxing code didn't implement this to
begin with.


> I know you're the one behind the spec in question, but wouldn't it be a
> better idea to wait until AVSphericalMapping gets a way to propagate this
> kind of information before adding support for muxing video projection
> elements? Or maybe try to implement it right now...
>

I'm happy to implement support for the projection specific info. What would
be the best way to proceed. It seems like I could just place a union with
projection specific structs in AVSphericalMapping. I'd also like some
advice around how to sequence the set of changes. My preference would be to
add the union & fields to AVSphericalMapping and update at least one
demuxer to at least justify the presence of the fields in a single patch.
Not sure if that is the preferred way to go about this though.


>
> This also applies to the mp4 patch.
>

Understood and makes sense.


>
> > +
> > +projection_private_size = avio_close_dyn_buf(dyn_cp,
> _private_ptr);
>
> The dynbuf should ideally contain the whole Projection master, so you can
> then pass its size to start_ebml_master() instead of zero.
> See mkv_write_video_color().
>
>
I'm a little confused about what you mean by passing the size to
start_ebml_master() it looks like both the calls I see in
mkv_write_video_color() pass in zero.


> > +
> > +projection = start_ebml_master(pb, MATROSKA_ID_VIDEOPROJECTION, 0);
> > +put_ebml_uint(pb, MATROSKA_ID_VIDEOPROJECTIONTYPE, projection_type);
> > +if 

Re: [FFmpeg-devel] [PATCH] mov: Fix spherical metadata_source parsing.

[James Almer]

On 1/27/2017 2:44 PM, Aaron Colwell wrote:
> The metadata_source field is a null-terminated string, like other ISOBMFF
> strings, not an 8-bit length followed by string characters. This patch
> fixes the parsing code so it rejects svhd boxes that are too small and
> skips to the end of the svhd box since we don't actually care about the
> contents of the
> metadata_source field.
> 
> 
> 0001-mov-Fix-spherical-metadata_source-parsing.patch
> 
> 
> From f63f65135e7059376acff3acc0e5268a8861d21d Mon Sep 17 00:00:00 2001
> From: Aaron Colwell 
> Date: Fri, 27 Jan 2017 09:33:29 -0800
> Subject: [PATCH] mov: Fix spherical metadata_source parsing.
> 
> The metadata_source field is a null-terminated string, like other ISOBMFF 
> strings,
> not an 8-bit length followed by string characters. This patch fixes the 
> parsing
> code so it rejects svhd boxes that are too small and skips to the end of the 
> svhd
> box since we don't actually care about the contents of the
> metadata_source field.
> ---
>  libavformat/mov.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 7dc550eb99..b1bfa0a35f 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -4566,7 +4566,7 @@ static int mov_read_sv3d(MOVContext *c, AVIOContext 
> *pb, MOVAtom atom)
>  }
>  
>  size = avio_rb32(pb);
> -if (size > atom.size)
> +if (size <= 12 || size > atom.size)
>  return AVERROR_INVALIDDATA;
>  
>  tag = avio_rl32(pb);
> @@ -4575,7 +4575,7 @@ static int mov_read_sv3d(MOVContext *c, AVIOContext 
> *pb, MOVAtom atom)
>  return 0;
>  }
>  avio_skip(pb, 4); /*  version + flags */
> -avio_skip(pb, avio_r8(pb)); /* metadata_source */
> +avio_skip(pb, size - 12); /* metadata_source */
>  
>  size = avio_rb32(pb);
>  if (size > atom.size)
> -- 2.11.0.483.g087da7b7c-goog

Pushed, thanks.

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

Re: [FFmpeg-devel] [PATCH] matroskaenc: Add support for writing video projection.

[James Almer]

On 1/27/2017 5:12 PM, Aaron Colwell wrote:
> Adding support for writing spherical metadata in Matroska.
> 
> 
> 0001-matroskaenc-Add-support-for-writing-video-projection.patch
> 
> 
> From 5a9cf56bf3114186412bb5572b153f886edb6ddb Mon Sep 17 00:00:00 2001
> From: Aaron Colwell 
> Date: Fri, 27 Jan 2017 12:07:25 -0800
> Subject: [PATCH] matroskaenc: Add support for writing video projection
>  element.
> 
> Adding support for writing spherical metadata in Matroska.
> ---
>  libavformat/matroskaenc.c | 64 
> +++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index f731b678b9..1b186db223 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -1038,6 +1038,67 @@ static int mkv_write_stereo_mode(AVFormatContext *s, 
> AVIOContext *pb,
>  return ret;
>  }
>  
> +static int mkv_write_video_projection(AVIOContext *pb, AVStream *st)
> +{
> +AVSphericalMapping *spherical_mapping = 
> (AVSphericalMapping*)av_stream_get_side_data(st, AV_PKT_DATA_SPHERICAL, NULL);
> +ebml_master projection;
> +int projection_type = 0;
> +
> +AVIOContext *dyn_cp;
> +uint8_t *projection_private_ptr = NULL;
> +int ret, projection_private_size;
> +
> +if (!spherical_mapping)
> +return 0;
> +
> +if (spherical_mapping->projection != AV_SPHERICAL_EQUIRECTANGULAR &&
> +spherical_mapping->projection != AV_SPHERICAL_CUBEMAP) {
> +av_log(pb, AV_LOG_WARNING, "Unsupported projection %d. projection 
> not written.\n", spherical_mapping->projection);
> +return 0;
> +}
> +
> +ret = avio_open_dyn_buf(_cp);
> +if (ret < 0)
> +return ret;
> +
> +switch (spherical_mapping->projection) {
> +case AV_SPHERICAL_EQUIRECTANGULAR:
> +projection_type = 1;
> +
> +/* Create ProjectionPrivate contents */
> +avio_wb32(dyn_cp, 0); /* version = 0 & flags = 0 */
> +avio_wb32(dyn_cp, 0); /* projection_bounds_top */
> +avio_wb32(dyn_cp, 0); /* projection_bounds_bottom */
> +avio_wb32(dyn_cp, 0); /* projection_bounds_left */
> +avio_wb32(dyn_cp, 0); /* projection_bounds_right */

You could instead use a local 20 byte array, fill it using AV_WB32() and
then write it with put_ebml_binary().

Also, the latest change to the spec draft mentions ProjectionPrivate is
optional for Equirectangular projections if the contents are going to be
all zero.

> +break;
> +case AV_SPHERICAL_CUBEMAP:
> +projection_type = 2;
> +
> +/* Create ProjectionPrivate contents */
> +avio_wb32(dyn_cp, 0); /* version = 0 & flags = 0 */
> +avio_wb32(dyn_cp, 0); /* layout */
> +avio_wb32(dyn_cp, 0); /* padding */
> +break;
> +}

Same, 12 byte array.

What if the user is trying to remux a matroska file that has a
ProjectionPrivate element or an mp4 with an equi box filled with non zero
values, for that matter?
I know you're the one behind the spec in question, but wouldn't it be a
better idea to wait until AVSphericalMapping gets a way to propagate this
kind of information before adding support for muxing video projection
elements? Or maybe try to implement it right now...

This also applies to the mp4 patch.

> +
> +projection_private_size = avio_close_dyn_buf(dyn_cp, 
> _private_ptr);

The dynbuf should ideally contain the whole Projection master, so you can
then pass its size to start_ebml_master() instead of zero.
See mkv_write_video_color().

> +
> +projection = start_ebml_master(pb, MATROSKA_ID_VIDEOPROJECTION, 0);
> +put_ebml_uint(pb, MATROSKA_ID_VIDEOPROJECTIONTYPE, projection_type);
> +if (projection_private_size)
> +put_ebml_binary(pb, MATROSKA_ID_VIDEOPROJECTIONPRIVATE, 
> projection_private_ptr, projection_private_size);
> +
> +put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEYAW, 
> (float)(spherical_mapping->yaw) / (1 << 16));
> +put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEPITCH, 
> (float)(spherical_mapping->pitch) / (1 << 16));
> +put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEROLL, 
> (float)(spherical_mapping->roll) / (1 << 16));
> +end_ebml_master(pb, projection);
> +
> +av_free(projection_private_ptr);
> +
> +return 0;
> +}
> +
>  static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv,
> int i, AVIOContext *pb, int default_stream_exists)
>  {
> @@ -1251,6 +1312,9 @@ static int mkv_write_track(AVFormatContext *s, 
> MatroskaMuxContext *mkv,
>  ret = mkv_write_video_color(pb, par, st);
>  if (ret < 0)
>  return ret;
> +ret = mkv_write_video_projection(pb, st);

This needs to be inside a check for strict experimental compliance
nonetheless.

> +if (ret < 0)
> +return ret;
>  end_ebml_master(pb, subinfo);
>  break;
>  
> -- 2.11.0.483.g087da7b7c-goog
> 
> 
> 

Re: [FFmpeg-devel] [PATCH] lavc/mjpegdec: hint next marker position in ff_mjpeg_find_marker

[Michael Niedermayer]

On Fri, Jan 27, 2017 at 09:47:18AM +0100, Matthieu Bouron wrote:
> On Fri, Jan 27, 2017 at 02:30:40AM +0100, Michael Niedermayer wrote:
> > On Thu, Jan 26, 2017 at 05:47:51PM +0100, Matthieu Bouron wrote:
> > > Speeds up next marker search when a SOS marker is found.
> > > 
> > > ff_mjpeg_find_marker goes from 54% to 30% under perf record ffmpeg -i
> > > sample_2992x4000.jpg
> > > ---
> > >  libavcodec/mjpegdec.c | 31 +--
> > >  libavcodec/mjpegdec.h |  2 +-
> > >  libavcodec/mxpegdec.c |  5 +++--
> > >  3 files changed, 29 insertions(+), 9 deletions(-)
> > 
> > this breaks decoding multiple jpeg files
> > for example tickets//856/nint.jpg
> 
> New patch attached fixing the issue.
> 
> [...]

>  mjpegdec.c |   31 +--
>  mjpegdec.h |2 +-
>  mxpegdec.c |5 +++--
>  3 files changed, 29 insertions(+), 9 deletions(-)
> 6ba49d73c189b197bdc1a983e26ccd49713c617c  
> 0001-lavc-mjpegdec-hint-next-marker-position-in-ff_mjpeg_.patch
> From ebef63ebdc75872d52529d5b74b6d05254d4c0fd Mon Sep 17 00:00:00 2001
> From: Matthieu Bouron 
> Date: Thu, 26 Jan 2017 15:17:36 +0100
> Subject: [PATCH] lavc/mjpegdec: hint next marker position in
>  ff_mjpeg_find_marker
> 
> Speeds up next marker search when a SOS marker is found.
> 
> ff_mjpeg_find_marker goes from 54% to 30% under perf record ffmpeg -i
> sample_2992x4000.jpg

the patch seems to work but
why is the "buf_ptr += (get_bits_count(>gb) + 7) / 8;" not
already skiping all that what the buf_hint does ?
is the difference the unescaping ?
maybe the buf_hint stuff can be simplified by adjusting buf_ptr instead
?

or maybe i misunderstand

thx

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 5/9] nistspheredec: prevent overflow during block alignment calculation

[Andreas Cadhalpun]

On 27.01.2017 02:56, Marton Balint wrote:
> I see 3 problems (wm4 explicitly named them, but I also had them in mind)
> - Little benefit, yet
> - Makes the code less clean, more cluttered
> - Increases binary size
> 
> The ideas I proposed (use macros, use common / factorized checks for common
> validatons and errors) might be a good compromise IMHO. Fuzzing thereforce
> can be done with "debug" builds.
> 
> Anyway, I am not blocking the patch, just expressing what I would prefer in
> the long run.

How about the following macro?
#define FF_RETURN_ERROR(condition, error, log_ctx, ...) {   \
if (condition) {\
if (!CONFIG_SMALL && log_ctx)   \
av_log(log_ctx, AV_LOG_ERROR, __VA_ARGS__); \
return error;   \
}   \
}

That could be used with error message:
FF_RETURN_ERROR(st->codecpar->channels > FF_SANE_NB_CHANNELS, 
AVERROR(ENOSYS),
s, "Too many channels %d > %d\n", st->codecpar->channels, 
FF_SANE_NB_CHANNELS)

Or without:
FF_RETURN_ERROR(st->codecpar->channels > FF_SANE_NB_CHANNELS, 
AVERROR(ENOSYS), NULL)

Best regards,
Andreas
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/3] mpeg12dec: validate color space

[Andreas Cadhalpun]

On 26.01.2017 13:59, Ronald S. Bultje wrote:
> On Wed, Jan 25, 2017 at 8:56 PM, Andreas Cadhalpun <
> andreas.cadhal...@googlemail.com> wrote:
>> On 26.01.2017 02:26, Ronald S. Bultje wrote:
>>> The question is not whether changing A or B towards the other makes the
>>> combination consistent. The question is which form of consistency is
>>> better...
>>
>> As new values don't get added that often, I don't see a problem with
>> requiring
>> to explicitly add them to libavutil before being able to use them.
> 
> 
> That is because your use case for libavcodec is constrained, you use it
> only for decoding videos and fuzzing.

Please refrain from making such presumptuous, false claims in the future.

> There are others in this community
> that do a lot more than that with libavcodec.
> 
> Look, Andreas, I appreciate the work you're doing, I really do, but you
> always pick a fight with every review I put out on your code.

That's a grossly distorted picture of reality [1][2][3].

> I don't
> understand why. My reviews are not difficult to address, they really are
> not. My reviews are actionable, and if the action is taken, I'm happy and
> the code can be committed.

I appreciate your reviews, as long as they remain technical.

> Why pick a fight? What is the point? Do you
> think I'm an idiot that has no clue what he's doing and should be shot down
> because of that?

Such polemic phrases are inappropriate in any technical discussion.

> Please appreciate that I do have some clue of what I'm
> doing, and I am looking out for the health of the project, just like you,
> but with a slightly different perspective to some things. If I'm wrong,
> please point it out, I make mistakes also, but in cases like these, it can
> also help to just drop it and move on. Resolving an issue is not losing, it
> is winning.

Technical issues should be resolved with convincing arguments and not by
asking anyone who disagrees with you to move on.

For example, you could explain why you think that allowing unknown values
here is important, even though new ones are added only rarely.

Best regards,
Andreas


1: https://ffmpeg.org/pipermail/ffmpeg-devel/2017-January/205192.html
2: https://ffmpeg.org/pipermail/ffmpeg-devel/2017-January/205406.html
3: https://ffmpeg.org/pipermail/ffmpeg-devel/2017-January/205414.html

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

Re: [FFmpeg-devel] [PATCH] lavf/tls_openssl: Support building with LibreSSL

[Mark Thompson]

On 27/01/17 21:42, Marek Behún wrote:
> In configure, check if the function BIO_meth_new is defined in the
> corresponding OpenSSL/LibreSSL library, and if yes, define
> HAVE_OPENSSL_BIO_METH_CALLS variable to 1 in config.h, or to 0
> otherwise.
> 
> Change the heuristics preprocessor check
>   #if OPENSSL_VERSION_NUMBER >= 0x101fL
> to
>   #if HAVE_OPENSSL_BIO_METH_CALLS
> 
> This makes it possible to use LibreSSL, which defines
> OPENSSL_VERSION_NUMBER to >= 0x200fL, but does not support the
> BIO_meth_* calls from OpenSSL 1.1.0+.
> 
> Signed-off-by: Marek Behun 
> ---
>  configure |  9 -
>  libavformat/tls_openssl.c | 12 ++--
>  2 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/configure b/configure
> index 7154142..f2bf7b4 100755
> --- a/configure
> +++ b/configure
> @@ -2060,6 +2060,7 @@ HAVE_LIST="
>  makeinfo
>  makeinfo_html
>  MMAL_PARAMETER_VIDEO_MAX_NUM_CALLBACKS
> +openssl_bio_meth_calls
>  perl
>  pod2man
>  sdl2
> @@ -5883,7 +5884,13 @@ enabled openssl   && { use_pkg_config openssl 
> openssl/ssl.h OPENSSL_init
> check_lib openssl/ssl.h SSL_library_init 
> -lssl -lcrypto ||
> check_lib openssl/ssl.h SSL_library_init 
> -lssl32 -leay32 ||
> check_lib openssl/ssl.h SSL_library_init 
> -lssl -lcrypto -lws2_32 -lgdi32 ||
> -   die "ERROR: openssl not found"; }
> +   die "ERROR: openssl not found"; } &&
> + { { check_pkg_config openssl openssl/bio.h 
> BIO_meth_new ||
> + check_func BIO_meth_new -lssl -lcrypto ||
> + check_func BIO_meth_new -lssl32 -leay32 ||
> + check_func BIO_meth_new -lssl -lcrypto 
> -lws2_32 -lgdi32; } &&
> +   enable openssl_bio_meth_calls || disable 
> openssl_bio_meth_calls
> + }

You don't need to duplicate the whole list of tests.  Once one of the first set 
has succeeded then the necessary libraries have already been added to the 
accumulated flags.

That is:

@@ -5883,7 +5884,8 @@ enabled openssl   && { use_pkg_config openssl 
openssl/ssl.h OPENSSL_init
check_lib openssl/ssl.h SSL_library_init -lssl 
-lcrypto ||
check_lib openssl/ssl.h SSL_library_init 
-lssl32 -leay32 ||
check_lib openssl/ssl.h SSL_library_init -lssl 
-lcrypto -lws2_32 -lgdi32 ||
-   die "ERROR: openssl not found"; }
+   die "ERROR: openssl not found"; } &&
+ { check_func BIO_meth_new && enable 
openssl_bio_meth_calls; }
 enabled qtkit_indev  && { check_header_objcc QTKit/QTKit.h || disable 
qtkit_indev; }

>  enabled qtkit_indev  && { check_header_objcc QTKit/QTKit.h || disable 
> qtkit_indev; }
>  
>  # libdc1394 check
> diff --git a/libavformat/tls_openssl.c b/libavformat/tls_openssl.c
> index 3d9768a..cede0b6 100644
> --- a/libavformat/tls_openssl.c
> +++ b/libavformat/tls_openssl.c
> @@ -43,7 +43,7 @@ typedef struct TLSContext {
>  TLSShared tls_shared;
>  SSL_CTX *ctx;
>  SSL *ssl;
> -#if OPENSSL_VERSION_NUMBER >= 0x101fL
> +#if HAVE_OPENSSL_BIO_METH_CALLS
>  BIO_METHOD* url_bio_method;
>  #endif
>  } TLSContext;
> @@ -68,7 +68,7 @@ static unsigned long openssl_thread_id(void)
>  
>  static int url_bio_create(BIO *b)
>  {
> -#if OPENSSL_VERSION_NUMBER >= 0x101fL
> +#if HAVE_OPENSSL_BIO_METH_CALLS
>  BIO_set_init(b, 1);
>  BIO_set_data(b, NULL);
>  BIO_set_flags(b, 0);
> @@ -85,7 +85,7 @@ static int url_bio_destroy(BIO *b)
>  return 1;
>  }
>  
> -#if OPENSSL_VERSION_NUMBER >= 0x101fL
> +#if HAVE_OPENSSL_BIO_METH_CALLS
>  #define GET_BIO_DATA(x) BIO_get_data(x);
>  #else
>  #define GET_BIO_DATA(x) (x)->ptr;
> @@ -133,7 +133,7 @@ static int url_bio_bputs(BIO *b, const char *str)
>  return url_bio_bwrite(b, str, strlen(str));
>  }
>  
> -#if OPENSSL_VERSION_NUMBER < 0x101fL
> +#if !HAVE_OPENSSL_BIO_METH_CALLS
>  static BIO_METHOD url_bio_method = {
>  .type = BIO_TYPE_SOURCE_SINK,
>  .name = "urlprotocol bio",
> @@ -212,7 +212,7 @@ static int tls_close(URLContext *h)
>  SSL_CTX_free(c->ctx);
>  if (c->tls_shared.tcp)
>  ffurl_close(c->tls_shared.tcp);
> -#if OPENSSL_VERSION_NUMBER >= 0x101fL
> +#if HAVE_OPENSSL_BIO_METH_CALLS
>  if (c->url_bio_method)
>  BIO_meth_free(c->url_bio_method);
>  #endif
> @@ -270,7 +270,7 @@ static int tls_open(URLContext *h, const char *uri, int 
> flags, AVDictionary **op
>  ret = AVERROR(EIO);
>  goto fail;
>  }
> -#if OPENSSL_VERSION_NUMBER >= 0x101fL
> +#if HAVE_OPENSSL_BIO_METH_CALLS
>  p->url_bio_method = 

Re: [FFmpeg-devel] [PATCH]lavf/mov: Export vendor metadata

[Michael Niedermayer]

On Mon, Dec 12, 2016 at 12:12:37PM +0100, Carl Eugen Hoyos wrote:
> Hi!
> 
> I saw only after writing this patch that a Google employee has sent 
> a more complicated variant of this patch in November (searching for 
> a description of the vendor field showed the patch).
> 
> Please comment, Carl Eugen

It seems some developers dislike exporting 4 byte vendor tags as
metadata.
Maybe using the table from:
http://www.sno.phy.queensu.ca/~phil/exiftool/TagNames/QuickTime.html#VendorID

And instead exporting the full vendor name as metadata from the demuxer
and converting it back to a 4byte value using the same table in the
muxer would be an option ?

If this doesnt result in a resolution of the disagreements then feel
free to ignore my suggestion, its not my intent to complicate this
by adding additional requests

thx

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you fake or manipulate statistics in a paper in physics you will never
get a job again.
If you fake or manipulate statistics in a paper in medicin you will get
a job for life at the pharma industry.


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] lavf/tls_openssl: Support building with LibreSSL

[Marek Behún]

In configure, check if the function BIO_meth_new is defined in the
corresponding OpenSSL/LibreSSL library, and if yes, define
HAVE_OPENSSL_BIO_METH_CALLS variable to 1 in config.h, or to 0
otherwise.

Change the heuristics preprocessor check
  #if OPENSSL_VERSION_NUMBER >= 0x101fL
to
  #if HAVE_OPENSSL_BIO_METH_CALLS

This makes it possible to use LibreSSL, which defines
OPENSSL_VERSION_NUMBER to >= 0x200fL, but does not support the
BIO_meth_* calls from OpenSSL 1.1.0+.

Signed-off-by: Marek Behun 
---
 configure |  9 -
 libavformat/tls_openssl.c | 12 ++--
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/configure b/configure
index 7154142..f2bf7b4 100755
--- a/configure
+++ b/configure
@@ -2060,6 +2060,7 @@ HAVE_LIST="
 makeinfo
 makeinfo_html
 MMAL_PARAMETER_VIDEO_MAX_NUM_CALLBACKS
+openssl_bio_meth_calls
 perl
 pod2man
 sdl2
@@ -5883,7 +5884,13 @@ enabled openssl   && { use_pkg_config openssl 
openssl/ssl.h OPENSSL_init
check_lib openssl/ssl.h SSL_library_init -lssl 
-lcrypto ||
check_lib openssl/ssl.h SSL_library_init 
-lssl32 -leay32 ||
check_lib openssl/ssl.h SSL_library_init -lssl 
-lcrypto -lws2_32 -lgdi32 ||
-   die "ERROR: openssl not found"; }
+   die "ERROR: openssl not found"; } &&
+ { { check_pkg_config openssl openssl/bio.h 
BIO_meth_new ||
+ check_func BIO_meth_new -lssl -lcrypto ||
+ check_func BIO_meth_new -lssl32 -leay32 ||
+ check_func BIO_meth_new -lssl -lcrypto 
-lws2_32 -lgdi32; } &&
+   enable openssl_bio_meth_calls || disable 
openssl_bio_meth_calls
+ }
 enabled qtkit_indev  && { check_header_objcc QTKit/QTKit.h || disable 
qtkit_indev; }
 
 # libdc1394 check
diff --git a/libavformat/tls_openssl.c b/libavformat/tls_openssl.c
index 3d9768a..cede0b6 100644
--- a/libavformat/tls_openssl.c
+++ b/libavformat/tls_openssl.c
@@ -43,7 +43,7 @@ typedef struct TLSContext {
 TLSShared tls_shared;
 SSL_CTX *ctx;
 SSL *ssl;
-#if OPENSSL_VERSION_NUMBER >= 0x101fL
+#if HAVE_OPENSSL_BIO_METH_CALLS
 BIO_METHOD* url_bio_method;
 #endif
 } TLSContext;
@@ -68,7 +68,7 @@ static unsigned long openssl_thread_id(void)
 
 static int url_bio_create(BIO *b)
 {
-#if OPENSSL_VERSION_NUMBER >= 0x101fL
+#if HAVE_OPENSSL_BIO_METH_CALLS
 BIO_set_init(b, 1);
 BIO_set_data(b, NULL);
 BIO_set_flags(b, 0);
@@ -85,7 +85,7 @@ static int url_bio_destroy(BIO *b)
 return 1;
 }
 
-#if OPENSSL_VERSION_NUMBER >= 0x101fL
+#if HAVE_OPENSSL_BIO_METH_CALLS
 #define GET_BIO_DATA(x) BIO_get_data(x);
 #else
 #define GET_BIO_DATA(x) (x)->ptr;
@@ -133,7 +133,7 @@ static int url_bio_bputs(BIO *b, const char *str)
 return url_bio_bwrite(b, str, strlen(str));
 }
 
-#if OPENSSL_VERSION_NUMBER < 0x101fL
+#if !HAVE_OPENSSL_BIO_METH_CALLS
 static BIO_METHOD url_bio_method = {
 .type = BIO_TYPE_SOURCE_SINK,
 .name = "urlprotocol bio",
@@ -212,7 +212,7 @@ static int tls_close(URLContext *h)
 SSL_CTX_free(c->ctx);
 if (c->tls_shared.tcp)
 ffurl_close(c->tls_shared.tcp);
-#if OPENSSL_VERSION_NUMBER >= 0x101fL
+#if HAVE_OPENSSL_BIO_METH_CALLS
 if (c->url_bio_method)
 BIO_meth_free(c->url_bio_method);
 #endif
@@ -270,7 +270,7 @@ static int tls_open(URLContext *h, const char *uri, int 
flags, AVDictionary **op
 ret = AVERROR(EIO);
 goto fail;
 }
-#if OPENSSL_VERSION_NUMBER >= 0x101fL
+#if HAVE_OPENSSL_BIO_METH_CALLS
 p->url_bio_method = BIO_meth_new(BIO_TYPE_SOURCE_SINK, "urlprotocol bio");
 BIO_meth_set_write(p->url_bio_method, url_bio_bwrite);
 BIO_meth_set_read(p->url_bio_method, url_bio_bread);
-- 
2.10.2

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

[FFmpeg-devel] [PATCH] avformat: fix ID3v2 parser for v2.2 comment frames

[Christopher Snowhill]

From: Chris Moeller 

---
 libavformat/id3v2.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
index 9969d7a..f7fa3ef 100644
--- a/libavformat/id3v2.c
+++ b/libavformat/id3v2.c
@@ -823,6 +823,7 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary 
**metadata,
 const ID3v2EMFunc *extra_func = NULL;
 unsigned char *uncompressed_buffer = NULL;
 av_unused int uncompressed_buffer_size = 0;
+const char *comm_frame;
 
 av_log(s, AV_LOG_DEBUG, "id3v2 ver:%d flags:%02X len:%d\n", version, 
flags, len);
 
@@ -834,12 +835,14 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary 
**metadata,
 }
 isv34 = 0;
 taghdrlen = 6;
+comm_frame = "COM";
 break;
 
 case 3:
 case 4:
 isv34 = 1;
 taghdrlen = 10;
+comm_frame = "COMM";
 break;
 
 default:
@@ -950,7 +953,7 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary 
**metadata,
 /* check for text tag or supported special meta tag */
 } else if (tag[0] == 'T' ||
!memcmp(tag, "USLT", 4) ||
-   !memcmp(tag, "COMM", 4) ||
+   !strcmp(tag, comm_frame) ||
(extra_meta &&
 (extra_func = get_extra_meta_func(tag, isv34 {
 pbx = pb;
@@ -1018,7 +1021,7 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary 
**metadata,
 read_ttag(s, pbx, tlen, metadata, tag);
 else if (!memcmp(tag, "USLT", 4))
 read_uslt(s, pbx, tlen, metadata);
-else if (!memcmp(tag, "COMM", 4))
+else if (!strcmp(tag, comm_frame))
 read_comment(s, pbx, tlen, metadata);
 else
 /* parse special meta tag */
-- 
2.10.1 (Apple Git-78)

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

[FFmpeg-devel] [PATCH] avformat: fix ID3v2 parser for v2.2 comment frames

[Christopher Snowhill]

It needs this patch first, but I have some example MP3 files that
decode gaplessly with these patches applied. Encoded using the latest
iTunes as of this writing (12.5.5.5) on macOS Sierra. Attaching the
MP3 files, as well as source FLAC files, after this patch goes
through.


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

Re: [FFmpeg-devel] [PATCH] lavf/matroskaenc.c: Free dyn bufs in mkv_free. Fixes memory leaks when muxing fails.

[Michael Niedermayer]

On Thu, Jan 26, 2017 at 11:26:46AM -0800, Sasi Inguva wrote:
> Signed-off-by: Sasi Inguva 
> ---
>  libavformat/matroskaenc.c | 17 +
>  1 file changed, 17 insertions(+)

applied

thx

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have often repented speaking, but never of holding my tongue.
-- Xenocrates


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] ffmpeg.c: Add output file index and stream index to vstats file.

[Michael Niedermayer]

On Wed, Jan 25, 2017 at 04:41:44PM -0800, Sasi Inguva wrote:
> Signed-off-by: Sasi Inguva 
> ---
>  doc/ffmpeg.texi | 8 +++-
>  ffmpeg.c| 9 +++--
>  ffmpeg_opt.c| 2 +-
>  3 files changed, 15 insertions(+), 4 deletions(-)

applied

thx

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] matroskaenc: Add support for writing video projection.

[Aaron Colwell]

Adding support for writing spherical metadata in Matroska.
From 5a9cf56bf3114186412bb5572b153f886edb6ddb Mon Sep 17 00:00:00 2001
From: Aaron Colwell 
Date: Fri, 27 Jan 2017 12:07:25 -0800
Subject: [PATCH] matroskaenc: Add support for writing video projection
 element.

Adding support for writing spherical metadata in Matroska.
---
 libavformat/matroskaenc.c | 64 +++
 1 file changed, 64 insertions(+)

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index f731b678b9..1b186db223 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -1038,6 +1038,67 @@ static int mkv_write_stereo_mode(AVFormatContext *s, AVIOContext *pb,
 return ret;
 }
 
+static int mkv_write_video_projection(AVIOContext *pb, AVStream *st)
+{
+AVSphericalMapping *spherical_mapping = (AVSphericalMapping*)av_stream_get_side_data(st, AV_PKT_DATA_SPHERICAL, NULL);
+ebml_master projection;
+int projection_type = 0;
+
+AVIOContext *dyn_cp;
+uint8_t *projection_private_ptr = NULL;
+int ret, projection_private_size;
+
+if (!spherical_mapping)
+return 0;
+
+if (spherical_mapping->projection != AV_SPHERICAL_EQUIRECTANGULAR &&
+spherical_mapping->projection != AV_SPHERICAL_CUBEMAP) {
+av_log(pb, AV_LOG_WARNING, "Unsupported projection %d. projection not written.\n", spherical_mapping->projection);
+return 0;
+}
+
+ret = avio_open_dyn_buf(_cp);
+if (ret < 0)
+return ret;
+
+switch (spherical_mapping->projection) {
+case AV_SPHERICAL_EQUIRECTANGULAR:
+projection_type = 1;
+
+/* Create ProjectionPrivate contents */
+avio_wb32(dyn_cp, 0); /* version = 0 & flags = 0 */
+avio_wb32(dyn_cp, 0); /* projection_bounds_top */
+avio_wb32(dyn_cp, 0); /* projection_bounds_bottom */
+avio_wb32(dyn_cp, 0); /* projection_bounds_left */
+avio_wb32(dyn_cp, 0); /* projection_bounds_right */
+break;
+case AV_SPHERICAL_CUBEMAP:
+projection_type = 2;
+
+/* Create ProjectionPrivate contents */
+avio_wb32(dyn_cp, 0); /* version = 0 & flags = 0 */
+avio_wb32(dyn_cp, 0); /* layout */
+avio_wb32(dyn_cp, 0); /* padding */
+break;
+}
+
+projection_private_size = avio_close_dyn_buf(dyn_cp, _private_ptr);
+
+projection = start_ebml_master(pb, MATROSKA_ID_VIDEOPROJECTION, 0);
+put_ebml_uint(pb, MATROSKA_ID_VIDEOPROJECTIONTYPE, projection_type);
+if (projection_private_size)
+put_ebml_binary(pb, MATROSKA_ID_VIDEOPROJECTIONPRIVATE, projection_private_ptr, projection_private_size);
+
+put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEYAW, (float)(spherical_mapping->yaw) / (1 << 16));
+put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEPITCH, (float)(spherical_mapping->pitch) / (1 << 16));
+put_ebml_float(pb, MATROSKA_ID_VIDEOPROJECTIONPOSEROLL, (float)(spherical_mapping->roll) / (1 << 16));
+end_ebml_master(pb, projection);
+
+av_free(projection_private_ptr);
+
+return 0;
+}
+
 static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv,
int i, AVIOContext *pb, int default_stream_exists)
 {
@@ -1251,6 +1312,9 @@ static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv,
 ret = mkv_write_video_color(pb, par, st);
 if (ret < 0)
 return ret;
+ret = mkv_write_video_projection(pb, st);
+if (ret < 0)
+return ret;
 end_ebml_master(pb, subinfo);
 break;
 
-- 
2.11.0.483.g087da7b7c-goog

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

Re: [FFmpeg-devel] [PATCH] lavf/tls_openssl: Support building with LibreSSL

[Mark Thompson]

On 27/01/17 19:15, Marek Behun wrote:
> On Fri, 27 Jan 2017 18:41:09 +
> Mark Thompson  wrote:
> 
>> On 27/01/17 17:31, Marek Behún wrote:
>>> Use the LIBRESSL_VERSION_NUMBER macro to determine if building with
>>> LibreSSL instead of OpenSSL. This is pretty straightforward, since
>>> it is enough to add this check to existing #if macros.
>>>
>>> Signed-off-by: Marek Behun 
>>> ---
>>>  libavformat/tls_openssl.c | 12 ++--
>>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/libavformat/tls_openssl.c b/libavformat/tls_openssl.c
>>> index 3d9768a..cf1a62e 100644
>>> --- a/libavformat/tls_openssl.c
>>> +++ b/libavformat/tls_openssl.c
>>> @@ -43,7 +43,7 @@ typedef struct TLSContext {
>>>  TLSShared tls_shared;
>>>  SSL_CTX *ctx;
>>>  SSL *ssl;
>>> -#if OPENSSL_VERSION_NUMBER >= 0x101fL
>>> +#if OPENSSL_VERSION_NUMBER >= 0x101fL
>>> && !defined(LIBRESSL_VERSION_NUMBER)  
>>
>> I don't understand what this is trying to do.
>>
>> Does LibreSSL support the OpenSSL 1.1.0 API:
>>
>> If yes, why would the additional check be needed?
>>
>> If no, isn't this doing nothing because the first check would be
>> false?
> 
> LibreSSL defines OPENSSL_VERSION_NUMBER to >=0x200, thus
> OPENSSL_VERSION_NUMBER is always greater than 0x101, but LibreSSL
> does not support 1.1.0 API.

Er, right, so it just lies and leaves it to user programs to sort it out.  How 
nice.

Looking back, I can see this has been discussed before:



That (beyond the disapprobation towards libressl for being naughty) looks like 
people would prefer the test to be in configure rather than copying the 
nontrivial #if condition everywhere?

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

Re: [FFmpeg-devel] [PATCH] avfilter/vf_gblur: Increase supported pixel count from 31bit to 32bit in filter_postscale()

[Michael Niedermayer]

On Sat, Jan 21, 2017 at 11:01:50PM +0100, Michael Niedermayer wrote:
> Fixes CID1396252
> 
> RFC: Is it preferred to use 64bit ?
> 
> Signed-off-by: Michael Niedermayer 
> ---
>  libavfilter/vf_gblur.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)

applied

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Good people do not need laws to tell them to act responsibly, while bad
people will find a way around the laws. -- Plato


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avfilter: do not write to unwritable frame

[Nicolas George]

L'octidi 8 pluviôse, an CCXXV, Muhammad Faiz a écrit :
> affect filters that set partial_buf_size
> test-case
> ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp, asplit 
> [a][b];
> [a] firequalizer=fixed=on, showcqt=s=1280x360 [a1];
> [b] firequalizer=fixed=on, showcqt=s=1280x360 [b1];
> [a1][b1] vstack'
> 
> Signed-off-by: Muhammad Faiz 
> ---
>  libavfilter/avfilter.c | 17 +
>  1 file changed, 17 insertions(+)

Maybe this can be of use:

https://git.ffmpeg.org/gitweb/ffmpeg.git/commit/28c62df672865890cbb13e5f0e94bde29c8fbacd

Regards,

-- 
  Nicolas George


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavf/tls_openssl: Support building with LibreSSL

[Marek Behun]

On Fri, 27 Jan 2017 18:41:09 +
Mark Thompson  wrote:

> On 27/01/17 17:31, Marek Behún wrote:
> > Use the LIBRESSL_VERSION_NUMBER macro to determine if building with
> > LibreSSL instead of OpenSSL. This is pretty straightforward, since
> > it is enough to add this check to existing #if macros.
> > 
> > Signed-off-by: Marek Behun 
> > ---
> >  libavformat/tls_openssl.c | 12 ++--
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/libavformat/tls_openssl.c b/libavformat/tls_openssl.c
> > index 3d9768a..cf1a62e 100644
> > --- a/libavformat/tls_openssl.c
> > +++ b/libavformat/tls_openssl.c
> > @@ -43,7 +43,7 @@ typedef struct TLSContext {
> >  TLSShared tls_shared;
> >  SSL_CTX *ctx;
> >  SSL *ssl;
> > -#if OPENSSL_VERSION_NUMBER >= 0x101fL
> > +#if OPENSSL_VERSION_NUMBER >= 0x101fL
> > && !defined(LIBRESSL_VERSION_NUMBER)  
> 
> I don't understand what this is trying to do.
> 
> Does LibreSSL support the OpenSSL 1.1.0 API:
> 
> If yes, why would the additional check be needed?
> 
> If no, isn't this doing nothing because the first check would be
> false?

LibreSSL defines OPENSSL_VERSION_NUMBER to >=0x200, thus
OPENSSL_VERSION_NUMBER is always greater than 0x101, but LibreSSL
does not support 1.1.0 API.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] avfilter/showwavespic : case of empty audio stream, fixes ticket 6107

[Александр Слободенюк]

0001-avfilter-showwavespic-case-of-empty-audio-stream-fix.patch
Description: Binary data
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] avfilter: do not write to unwritable frame

[Muhammad Faiz]

affect filters that set partial_buf_size
test-case
ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp, asplit 
[a][b];
[a] firequalizer=fixed=on, showcqt=s=1280x360 [a1];
[b] firequalizer=fixed=on, showcqt=s=1280x360 [b1];
[a1][b1] vstack'

Signed-off-by: Muhammad Faiz 
---
 libavfilter/avfilter.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
index c12d491..9b28842 100644
--- a/libavfilter/avfilter.c
+++ b/libavfilter/avfilter.c
@@ -1235,6 +1235,22 @@ static int take_samples(AVFilterLink *link, unsigned 
min, unsigned max,
 frame = ff_framequeue_peek(>fifo, 0);
 av_samples_copy(buf->extended_data, frame->extended_data, p, 0, n,
 link->channels, link->format);
+
+if (!av_frame_is_writable(frame)) {
+AVFrame *new = ff_get_audio_buffer(link, frame->nb_samples);
+if (!new)
+return AVERROR(ENOMEM);
+ret = av_frame_copy_props(new, frame);
+if (ret < 0) {
+av_frame_free();
+return ret;
+}
+av_frame_copy(new, frame);
+av_frame_unref(frame);
+av_frame_move_ref(frame, new);
+av_frame_free();
+}
+
 frame->nb_samples -= n;
 av_samples_copy(frame->extended_data, frame->extended_data, 0, n,
 frame->nb_samples, link->channels, link->format);
-- 
2.5.0

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

Re: [FFmpeg-devel] [PATCH] avfilter: do not write to unwritable frame

[Muhammad Faiz]

On 1/27/17, wm4  wrote:
> On Fri, 27 Jan 2017 22:04:50 +0700
> Muhammad Faiz  wrote:
>
>> On 1/27/17, wm4  wrote:
>> > On Fri, 27 Jan 2017 18:42:03 +0700
>> > Muhammad Faiz  wrote:
>> >
>> >> affect filters that set partial_buf_size
>> >> test-case
>> >> ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp,
>> >> asplit
>> >> [a][b];
>> >> [a] firequalizer=fixed=on, showcqt=s=1280x360 [a1];
>> >> [b] firequalizer=fixed=on, showcqt=s=1280x360 [b1];
>> >> [a1][b1] vstack'
>> >>
>> >> Signed-off-by: Muhammad Faiz 
>> >> ---
>> >>  libavfilter/avfilter.c | 17 +
>> >>  1 file changed, 17 insertions(+)
>> >>
>> >> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
>> >> index c12d491..7e7e83c 100644
>> >> --- a/libavfilter/avfilter.c
>> >> +++ b/libavfilter/avfilter.c
>> >> @@ -1235,6 +1235,23 @@ static int take_samples(AVFilterLink *link,
>> >> unsigned min, unsigned max,
>> >>  frame = ff_framequeue_peek(>fifo, 0);
>> >>  av_samples_copy(buf->extended_data, frame->extended_data, p,
>> >> 0,
>> >> n,
>> >>  link->channels, link->format);
>> >> +
>> >> +if (!av_frame_is_writable(frame)) {
>> >> +AVFrame *new = ff_get_audio_buffer(link,
>> >> frame->nb_samples);
>> >> +if (!new)
>> >> +return AVERROR(ENOMEM);
>> >> +ret = av_frame_copy_props(new, frame);
>> >> +if (ret < 0)
>> >> +return ret;
>> >> +av_samples_copy(new->extended_data, frame->extended_data,
>> >> +0, 0, frame->nb_samples,
>> >> +av_frame_get_channels(frame),
>> >> +frame->format);
>> >> +av_frame_unref(frame);
>> >> +av_frame_move_ref(frame, new);
>> >> +av_frame_free();
>> >> +}
>> >> +
>> >>  frame->nb_samples -= n;
>> >>  av_samples_copy(frame->extended_data, frame->extended_data, 0,
>> >> n,
>> >>  frame->nb_samples, link->channels,
>> >> link->format);
>> >
>> > There's a function that does this automatically:
>> > av_frame_make_writable()
>>
>> OK, I have known this.
>>
>> >
>> > Might not be fully equivalent though due to ff_get_audio_buffer()
>>
>> If it is permitted to not use ff_get_audio_buffer(), I will use
>> av_frame_make_writable().
>
> I don't know about the details. Actually it looks like the ff_ function
> does something non-trivial (like using a buffer pool or allocating a
> buffer from the next filter).
>
> So it looks like your code is actually slightly more correct than a
> solution using av_frame_make_writable(). Although it's sad that most of
> the av_frame_make_writable() function has to be sort-of duplicated.
>
> So, patch is ok by me. Sorry for the additional confusion.
>
> (Maybe av_frame_copy() could be used instead of av_samples_copy().)

Changed to use av_frame_copy, also fix memleak
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] mov: Fix spherical metadata_source parsing.

[Aaron Colwell]

The metadata_source field is a null-terminated string, like other ISOBMFF
strings, not an 8-bit length followed by string characters. This patch
fixes the parsing code so it rejects svhd boxes that are too small and
skips to the end of the svhd box since we don't actually care about the
contents of the
metadata_source field.
From f63f65135e7059376acff3acc0e5268a8861d21d Mon Sep 17 00:00:00 2001
From: Aaron Colwell 
Date: Fri, 27 Jan 2017 09:33:29 -0800
Subject: [PATCH] mov: Fix spherical metadata_source parsing.

The metadata_source field is a null-terminated string, like other ISOBMFF strings,
not an 8-bit length followed by string characters. This patch fixes the parsing
code so it rejects svhd boxes that are too small and skips to the end of the svhd
box since we don't actually care about the contents of the
metadata_source field.
---
 libavformat/mov.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 7dc550eb99..b1bfa0a35f 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -4566,7 +4566,7 @@ static int mov_read_sv3d(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 }
 
 size = avio_rb32(pb);
-if (size > atom.size)
+if (size <= 12 || size > atom.size)
 return AVERROR_INVALIDDATA;
 
 tag = avio_rl32(pb);
@@ -4575,7 +4575,7 @@ static int mov_read_sv3d(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 return 0;
 }
 avio_skip(pb, 4); /*  version + flags */
-avio_skip(pb, avio_r8(pb)); /* metadata_source */
+avio_skip(pb, size - 12); /* metadata_source */
 
 size = avio_rb32(pb);
 if (size > atom.size)
-- 
2.11.0.483.g087da7b7c-goog

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

[FFmpeg-devel] [PATCH] lavf/tls_openssl: Support building with LibreSSL

[Marek Behún]

Use the LIBRESSL_VERSION_NUMBER macro to determine if building with
LibreSSL instead of OpenSSL. This is pretty straightforward, since
it is enough to add this check to existing #if macros.

Signed-off-by: Marek Behun 
---
 libavformat/tls_openssl.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/libavformat/tls_openssl.c b/libavformat/tls_openssl.c
index 3d9768a..cf1a62e 100644
--- a/libavformat/tls_openssl.c
+++ b/libavformat/tls_openssl.c
@@ -43,7 +43,7 @@ typedef struct TLSContext {
 TLSShared tls_shared;
 SSL_CTX *ctx;
 SSL *ssl;
-#if OPENSSL_VERSION_NUMBER >= 0x101fL
+#if OPENSSL_VERSION_NUMBER >= 0x101fL && !defined(LIBRESSL_VERSION_NUMBER)
 BIO_METHOD* url_bio_method;
 #endif
 } TLSContext;
@@ -68,7 +68,7 @@ static unsigned long openssl_thread_id(void)
 
 static int url_bio_create(BIO *b)
 {
-#if OPENSSL_VERSION_NUMBER >= 0x101fL
+#if OPENSSL_VERSION_NUMBER >= 0x101fL && !defined(LIBRESSL_VERSION_NUMBER)
 BIO_set_init(b, 1);
 BIO_set_data(b, NULL);
 BIO_set_flags(b, 0);
@@ -85,7 +85,7 @@ static int url_bio_destroy(BIO *b)
 return 1;
 }
 
-#if OPENSSL_VERSION_NUMBER >= 0x101fL
+#if OPENSSL_VERSION_NUMBER >= 0x101fL && !defined(LIBRESSL_VERSION_NUMBER)
 #define GET_BIO_DATA(x) BIO_get_data(x);
 #else
 #define GET_BIO_DATA(x) (x)->ptr;
@@ -133,7 +133,7 @@ static int url_bio_bputs(BIO *b, const char *str)
 return url_bio_bwrite(b, str, strlen(str));
 }
 
-#if OPENSSL_VERSION_NUMBER < 0x101fL
+#if OPENSSL_VERSION_NUMBER < 0x101fL || defined(LIBRESSL_VERSION_NUMBER)
 static BIO_METHOD url_bio_method = {
 .type = BIO_TYPE_SOURCE_SINK,
 .name = "urlprotocol bio",
@@ -212,7 +212,7 @@ static int tls_close(URLContext *h)
 SSL_CTX_free(c->ctx);
 if (c->tls_shared.tcp)
 ffurl_close(c->tls_shared.tcp);
-#if OPENSSL_VERSION_NUMBER >= 0x101fL
+#if OPENSSL_VERSION_NUMBER >= 0x101fL && !defined(LIBRESSL_VERSION_NUMBER)
 if (c->url_bio_method)
 BIO_meth_free(c->url_bio_method);
 #endif
@@ -270,7 +270,7 @@ static int tls_open(URLContext *h, const char *uri, int 
flags, AVDictionary **op
 ret = AVERROR(EIO);
 goto fail;
 }
-#if OPENSSL_VERSION_NUMBER >= 0x101fL
+#if OPENSSL_VERSION_NUMBER >= 0x101fL && !defined(LIBRESSL_VERSION_NUMBER)
 p->url_bio_method = BIO_meth_new(BIO_TYPE_SOURCE_SINK, "urlprotocol bio");
 BIO_meth_set_write(p->url_bio_method, url_bio_bwrite);
 BIO_meth_set_read(p->url_bio_method, url_bio_bread);
-- 
2.10.2

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

Re: [FFmpeg-devel] [PATCH] Ignore duplicate ID3 tags if vorbis tags exist

[wm4]

On Fri, 27 Jan 2017 17:32:08 +0100
Paul Arzelier  wrote:

> From 227d7d1f4b5665f824900cf2b14863621180dd1c Mon Sep 17 00:00:00 2001
> From: Polochon-street 
> Date: Fri, 27 Jan 2017 16:49:41 +0100
> Subject: [PATCH] Ignore ID3 tags if other tags are present (e.g. vorbis)
> Originally-by: Ben Boeckel 
> ---
>  libavformat/internal.h |  5 +
>  libavformat/mp3dec.c   |  5 +
>  libavformat/options.c  |  1 +
>  libavformat/utils.c| 16 +++-
>  4 files changed, 26 insertions(+), 1 deletion(-)
> 

Looks largely ok to me, just a few minor nits.

> diff --git a/libavformat/internal.h b/libavformat/internal.h
> index 9d614fb12c..63a1724cfa 100644
> --- a/libavformat/internal.h
> +++ b/libavformat/internal.h
> @@ -140,6 +140,11 @@ struct AVFormatInternal {
>   * Whether or not avformat_init_output fully initialized streams
>   */
>  int streams_initialized;
> +
> +/**
> + * ID3v2 tag useful for MP3 demuxing

The tag is used for many file formats (unfortunately), not just mp3. So
I'm not sure if the comment is correct.

> + */
> +AVDictionary *id3v2_meta;
>  };
>  
>  struct AVStreamInternal {
> diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> index 099ca57d24..8540e45e4f 100644
> --- a/libavformat/mp3dec.c
> +++ b/libavformat/mp3dec.c
> @@ -349,6 +349,11 @@ static int mp3_read_header(AVFormatContext *s)
>  int ret;
>  int i;
>  
> +if (s->internal->id3v2_meta)
> +s->metadata = s->internal->id3v2_meta;

Slightly odd that it checks for id3v2_meta being NULL. Seems redundant?

> +
> +s->internal->id3v2_meta = NULL;
> +
>  st = avformat_new_stream(s, NULL);
>  if (!st)
>  return AVERROR(ENOMEM);
> diff --git a/libavformat/options.c b/libavformat/options.c
> index 25a506eef8..d8aca472c5 100644
> --- a/libavformat/options.c
> +++ b/libavformat/options.c
> @@ -144,6 +144,7 @@ AVFormatContext *avformat_alloc_context(void)
>  ic->internal->offset = AV_NOPTS_VALUE;
>  ic->internal->raw_packet_buffer_remaining_size = RAW_PACKET_BUFFER_SIZE;
>  ic->internal->shortest_end = AV_NOPTS_VALUE;
> +ic->internal->id3v2_meta = NULL;

Not necessary. All fields are initialized to 0 by default, because the
struct is allocated by av_mallocz().

>  
>  return ic;
>  }
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index d5dfca7dec..b44d688213 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -588,12 +588,25 @@ int avformat_open_input(AVFormatContext **ps, const 
> char *filename,
>  
>  /* e.g. AVFMT_NOFILE formats will not have a AVIOContext */
>  if (s->pb)
> -ff_id3v2_read(s, ID3v2_DEFAULT_MAGIC, _extra_meta, 0);
> +ff_id3v2_read_dict(s->pb, >internal->id3v2_meta, 
> ID3v2_DEFAULT_MAGIC, _extra_meta);
>  
>  if (!(s->flags_FLAG_PRIV_OPT) && s->iformat->read_header)
>  if ((ret = s->iformat->read_header(s)) < 0)
>  goto fail;
>  
> +if (!s->metadata) {
> +s->metadata = s->internal->id3v2_meta;
> +s->internal->id3v2_meta = NULL;
> +} else if (s->internal->id3v2_meta) {
> +int level = AV_LOG_WARNING;
> +if (s->error_recognition & AV_EF_COMPLIANT)
> +level = AV_LOG_ERROR;
> +av_log(s, level, "Discarding ID3 tags because more suitable tags 
> were found.\n");
> +av_dict_free(>internal->id3v2_meta);
> +if (s->error_recognition & AV_EF_EXPLODE)
> +return AVERROR_INVALIDDATA;
> +}
> +
>  if (id3v2_extra_meta) {
>  if (!strcmp(s->iformat->name, "mp3") || !strcmp(s->iformat->name, 
> "aac") ||
>  !strcmp(s->iformat->name, "tta")) {
> @@ -627,6 +640,7 @@ int avformat_open_input(AVFormatContext **ps, const char 
> *filename,
>  fail:
>  ff_id3v2_free_extra_meta(_extra_meta);
>  av_dict_free();
> +av_dict_free(>internal->id3v2_meta);
>  if (s->pb && !(s->flags & AVFMT_FLAG_CUSTOM_IO))
>  avio_closep(>pb);
>  avformat_free_context(s);

You could free the tag in avformat_free_context(), which might be
slightly simpler (don't need to care about success vs. error path), but
it doesn't actually matter.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avfilter: do not write to unwritable frame

[wm4]

On Fri, 27 Jan 2017 22:04:50 +0700
Muhammad Faiz  wrote:

> On 1/27/17, wm4  wrote:
> > On Fri, 27 Jan 2017 18:42:03 +0700
> > Muhammad Faiz  wrote:
> >  
> >> affect filters that set partial_buf_size
> >> test-case
> >> ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp, asplit
> >> [a][b];
> >> [a] firequalizer=fixed=on, showcqt=s=1280x360 [a1];
> >> [b] firequalizer=fixed=on, showcqt=s=1280x360 [b1];
> >> [a1][b1] vstack'
> >>
> >> Signed-off-by: Muhammad Faiz 
> >> ---
> >>  libavfilter/avfilter.c | 17 +
> >>  1 file changed, 17 insertions(+)
> >>
> >> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> >> index c12d491..7e7e83c 100644
> >> --- a/libavfilter/avfilter.c
> >> +++ b/libavfilter/avfilter.c
> >> @@ -1235,6 +1235,23 @@ static int take_samples(AVFilterLink *link,
> >> unsigned min, unsigned max,
> >>  frame = ff_framequeue_peek(>fifo, 0);
> >>  av_samples_copy(buf->extended_data, frame->extended_data, p, 0,
> >> n,
> >>  link->channels, link->format);
> >> +
> >> +if (!av_frame_is_writable(frame)) {
> >> +AVFrame *new = ff_get_audio_buffer(link, frame->nb_samples);
> >> +if (!new)
> >> +return AVERROR(ENOMEM);
> >> +ret = av_frame_copy_props(new, frame);
> >> +if (ret < 0)
> >> +return ret;
> >> +av_samples_copy(new->extended_data, frame->extended_data,
> >> +0, 0, frame->nb_samples,
> >> +av_frame_get_channels(frame),
> >> +frame->format);
> >> +av_frame_unref(frame);
> >> +av_frame_move_ref(frame, new);
> >> +av_frame_free();
> >> +}
> >> +
> >>  frame->nb_samples -= n;
> >>  av_samples_copy(frame->extended_data, frame->extended_data, 0, n,
> >>  frame->nb_samples, link->channels, link->format); 
> >>  
> >
> > There's a function that does this automatically:
> > av_frame_make_writable()  
> 
> OK, I have known this.
> 
> >
> > Might not be fully equivalent though due to ff_get_audio_buffer()  
> 
> If it is permitted to not use ff_get_audio_buffer(), I will use
> av_frame_make_writable().

I don't know about the details. Actually it looks like the ff_ function
does something non-trivial (like using a buffer pool or allocating a
buffer from the next filter).

So it looks like your code is actually slightly more correct than a
solution using av_frame_make_writable(). Although it's sad that most of
the av_frame_make_writable() function has to be sort-of duplicated.

So, patch is ok by me. Sorry for the additional confusion.

(Maybe av_frame_copy() could be used instead of av_samples_copy().)
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] Ignore duplicate ID3 tags if vorbis tags exist

[Paul Arzelier]

I've added an id3v2_meta AVDictionary for the AVInternalFormat structure 
and edited mp3dec.c as wm4 suggested.


It solves the issue Michael noticed, and passes the fate tests, but 
maybe it still need some work; tell me!



Le 27/01/2017 à 07:02, wm4 a écrit :

On Fri, 27 Jan 2017 03:03:03 +0100
Michael Niedermayer  wrote:


On Thu, Jan 26, 2017 at 02:29:21PM +0100, Paul Arzelier wrote:

Alright, attached is the last version (I hope!)

Paul


Le 26/01/2017 à 13:43, wm4 a écrit :

On Thu, 26 Jan 2017 13:32:08 +0100
Paul Arzelier  wrote:
  

 From d84648e1990ad3a12462dfb76990dc7036f5f082 Mon Sep 17 00:00:00 2001
From: Polochon-street 
Date: Thu, 26 Jan 2017 13:25:22 +0100
Subject: [PATCH] Ignore ID3 tags if other tags are present (e.g. vorbis
  comments)
Originally-by: Ben Boeckel 
---
  libavformat/utils.c | 17 -
  1 file changed, 16 insertions(+), 1 deletion(-)
  

Patch looks generally great to me now.
  

diff --git a/libavformat/utils.c b/libavformat/utils.c
index d5dfca7dec..ce24244135 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -513,6 +513,7 @@ int avformat_open_input(AVFormatContext **ps, const char 
*filename,
  AVFormatContext *s = *ps;
  int i, ret = 0;
  AVDictionary *tmp = NULL;
+AVDictionary *id3_meta = NULL;
  ID3v2ExtraMeta *id3v2_extra_meta = NULL;
  if (!s && !(s = avformat_alloc_context()))
@@ -588,12 +589,25 @@ int avformat_open_input(AVFormatContext **ps, const char 
*filename,
  /* e.g. AVFMT_NOFILE formats will not have a AVIOContext */
  if (s->pb)
-ff_id3v2_read(s, ID3v2_DEFAULT_MAGIC, _extra_meta, 0);
+ff_id3v2_read_dict(s->pb, _meta, ID3v2_DEFAULT_MAGIC, 
_extra_meta);
  if (!(s->flags_FLAG_PRIV_OPT) && s->iformat->read_header)
  if ((ret = s->iformat->read_header(s)) < 0)
  goto fail;
+if (!s->metadata) {
+av_dict_copy(>metadata, id3_meta, AV_DICT_DONT_OVERWRITE);
+av_dict_free(_meta);

Strictly speaking, this line is redundant, but it doesn't harm either.
If you feel like it you could remove it, but it's not necessary.
  

+}
+else if (id3_meta) {

If you make another patch, you could merge the } with the next line too
(I'm not sure, but I think that's preferred coding-style wise).
  

+int level = AV_LOG_WARNING;
+if (s->error_recognition & AV_EF_COMPLIANT)
+level = AV_LOG_ERROR;
+av_log(s, level, "Discarding ID3 tag because more suitable tags were 
found.\n");
+if (s->error_recognition & AV_EF_EXPLODE)
+return AVERROR_INVALIDDATA;
+}
+
  if (id3v2_extra_meta) {
  if (!strcmp(s->iformat->name, "mp3") || !strcmp(s->iformat->name, 
"aac") ||
  !strcmp(s->iformat->name, "tta")) {
@@ -627,6 +641,7 @@ int avformat_open_input(AVFormatContext **ps, const char 
*filename,
  fail:
  ff_id3v2_free_extra_meta(_extra_meta);
  av_dict_free();
+   av_dict_free(_meta);
  if (s->pb && !(s->flags & AVFMT_FLAG_CUSTOM_IO))
  avio_closep(>pb);
  avformat_free_context(s);

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

--
Paul ARZELIER
Élève ingénieur à l'École Centrale de Lille
2014-2017
   
  utils.c |   17 -

  1 file changed, 16 insertions(+), 1 deletion(-)
477d4f87058a098464e2c1dbfe7e7ff806d2c85f  
0001-Ignore-ID3-tags-if-other-tags-are-present-e.g.-vorbi.patch
 From d84648e1990ad3a12462dfb76990dc7036f5f082 Mon Sep 17 00:00:00 2001
From: Polochon-street 
Date: Thu, 26 Jan 2017 13:25:22 +0100
Subject: [PATCH] Ignore ID3 tags if other tags are present (e.g. vorbis
  comments)
Originally-by: Ben Boeckel 
---
  libavformat/utils.c | 17 -
  1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/libavformat/utils.c b/libavformat/utils.c
index d5dfca7dec..ce24244135 100644
--- a/libavformat/utils.c
+++ b/libavformat/utils.c
@@ -513,6 +513,7 @@ int avformat_open_input(AVFormatContext **ps, const char 
*filename,
  AVFormatContext *s = *ps;
  int i, ret = 0;
  AVDictionary *tmp = NULL;
+AVDictionary *id3_meta = NULL;
  ID3v2ExtraMeta *id3v2_extra_meta = NULL;
  
  if (!s && !(s = avformat_alloc_context()))

@@ -588,12 +589,25 @@ int avformat_open_input(AVFormatContext **ps, const char 
*filename,
  
  /* e.g. AVFMT_NOFILE formats will not have a AVIOContext */

  if (s->pb)
-ff_id3v2_read(s, ID3v2_DEFAULT_MAGIC, _extra_meta, 0);
+ff_id3v2_read_dict(s->pb, _meta, ID3v2_DEFAULT_MAGIC, 
_extra_meta);
  
  if (!(s->flags_FLAG_PRIV_OPT) && s->iformat->read_header)

  if ((ret = s->iformat->read_header(s)) < 0)
  goto fail;
  
+if (!s->metadata) {

+s->metadata = id3_meta;
+id3_meta = NULL;
+} else if (id3_meta) {

Re: [FFmpeg-devel] [PATCH] avfilter: do not write to unwritable frame

[Muhammad Faiz]

On 1/27/17, wm4  wrote:
> On Fri, 27 Jan 2017 18:42:03 +0700
> Muhammad Faiz  wrote:
>
>> affect filters that set partial_buf_size
>> test-case
>> ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp, asplit
>> [a][b];
>> [a] firequalizer=fixed=on, showcqt=s=1280x360 [a1];
>> [b] firequalizer=fixed=on, showcqt=s=1280x360 [b1];
>> [a1][b1] vstack'
>>
>> Signed-off-by: Muhammad Faiz 
>> ---
>>  libavfilter/avfilter.c | 17 +
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
>> index c12d491..7e7e83c 100644
>> --- a/libavfilter/avfilter.c
>> +++ b/libavfilter/avfilter.c
>> @@ -1235,6 +1235,23 @@ static int take_samples(AVFilterLink *link,
>> unsigned min, unsigned max,
>>  frame = ff_framequeue_peek(>fifo, 0);
>>  av_samples_copy(buf->extended_data, frame->extended_data, p, 0,
>> n,
>>  link->channels, link->format);
>> +
>> +if (!av_frame_is_writable(frame)) {
>> +AVFrame *new = ff_get_audio_buffer(link, frame->nb_samples);
>> +if (!new)
>> +return AVERROR(ENOMEM);
>> +ret = av_frame_copy_props(new, frame);
>> +if (ret < 0)
>> +return ret;
>> +av_samples_copy(new->extended_data, frame->extended_data,
>> +0, 0, frame->nb_samples,
>> +av_frame_get_channels(frame),
>> +frame->format);
>> +av_frame_unref(frame);
>> +av_frame_move_ref(frame, new);
>> +av_frame_free();
>> +}
>> +
>>  frame->nb_samples -= n;
>>  av_samples_copy(frame->extended_data, frame->extended_data, 0, n,
>>  frame->nb_samples, link->channels, link->format);
>
> There's a function that does this automatically:
> av_frame_make_writable()

OK, I have known this.

>
> Might not be fully equivalent though due to ff_get_audio_buffer()

If it is permitted to not use ff_get_audio_buffer(), I will use
av_frame_make_writable().

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

Re: [FFmpeg-devel] patch: the fastest video decoder ever? :)

[u-9iep]

Here comes a slightly better version, with rounding to nearest instead
of rounding down at color bits truncation. I was unable to reliably measure
any speed difference compared to the simplistic former version.

The output quality now fully corresponds to the capabilities of rgb565
and the decoding is 12 - 17% faster than to rgb24.

The most remarkable gain of course is by skipping any extra format
conversion when on an rgb565 screen.

Posting the patch for perusal of possible interested parties.

A runtime output format option might be worth to be added,
and possibly support for more output formats as well?

Cheers,
Rune

On Fri, Jan 06, 2017 at 02:13:41PM +0100, u-9...@aetey.se wrote:
> Hello,
> 
> On slow hardware a 16-bit framebuffer depth makes a remarkable difference
> in performance and still provides a good look.
> 
> When videos are to be played on slow terminals there is a single
> best speed performer, cinepak (tell me if you know a better codec in this
> respect, the only comparable one I know of is VQA-15, but there is no
> reasonable encoder for it, nor a safe decoder).
> 
> Unfortunately cinepak as present in ffmpeg yields rgb24 which has to be
> repacked depending on the framebuffer format.
> 
> The attached patch makes cinepak to provide rgb565 in native endianness.
> This cuts in half (!) the mplayer CPU consumption on i686 with Xorg
> in the corresponding mode/depth, compared to decoding to rgb24.
> 
> The optimization is possible because cinepak is a VQ codec and pixel
> format packing at codebook fill is much more efficient than at a later
> stage. (Thanks Michael for the suggestion, long ago).
> 
> Of course the picture quality is slightly affected if this decoder version
> is used with larger framebuffer depths. A run-time switch would be to
> prefer, if this optimization against all odds would be welcome upstream :)
> 
> I am posting this as a proof of concept, lacking run-time format
> switching. As said, this patch yields half CPU consumption or double
> speed at decoding for rgb565 terminals.
--- libavcodec/cinepak.c.rgb24  2017-01-27 11:54:31.316799141 +0100
+++ libavcodec/cinepak.c2017-01-27 12:25:33.674095602 +0100
@@ -31,6 +31,8 @@
  *
  * Cinepak colorspace support (c) 2013 Rl, Aetey Global Technologies AB
  * @author Cinepak colorspace, Rl, Aetey Global Technologies AB
+ * Cinepak rgb565 format (c) 2017 Rl, Aetey Global Technologies AB
+ * @author Cinepak rgb565, Rl, Aetey Global Technologies AB
  */
 
 #include 
@@ -42,8 +44,12 @@
 #include "avcodec.h"
 #include "internal.h"
 
+/* feel free to replace with a better mapping implementation
+ * (keeping in mind slow, not very "intelligent" hardware)
+ */
+#define PACK_RGB_RGB565(r,g,b) 
(((av_clip_uint8((r)+4)>>3)<<11)|((av_clip_uint8((g)+2)>>2)<<5)|(av_clip_uint8((b)+4)>>3))
 
-typedef uint8_t cvid_codebook[12];
+typedef uint16_t cvid_codebook[4]; /* in the native endian rgb565 format */
 
 #define MAX_STRIPS  32
 
@@ -73,13 +79,13 @@
 uint32_t pal[256];
 } CinepakContext;
 
-static void cinepak_decode_codebook (cvid_codebook *codebook,
+static void cinepak_decode_codebook (cvid_codebook *codebook, int 
palette_video,
  int chunk_id, int size, const uint8_t 
*data)
 {
 const uint8_t *eod = (data + size);
 uint32_t flag, mask;
 int  i, n;
-uint8_t *p;
+uint16_t *p;
 
 /* check if this chunk contains 4- or 6-element vectors */
 n= (chunk_id & 0x04) ? 4 : 6;
@@ -98,33 +104,36 @@
 }
 
 if (!(chunk_id & 0x01) || (flag & mask)) {
-int k, kk;
+int k;
 
 if ((data + n) > eod)
 break;
 
-for (k = 0; k < 4; ++k) {
-int r = *data++;
-for (kk = 0; kk < 3; ++kk)
-*p++ = r;
-}
-if (n == 6) {
-int r, g, b, u, v;
+if (n == 4)
+/* 8-bit palette indexes or otherwise grayscale values,
+ * they need different handling */
+if (palette_video)
+for (k = 0; k < 4; ++k)
+*p++ = *data++;
+else
+for (k = 0; k < 4; ++k) {
+int r = *data++;
+*p++ = PACK_RGB_RGB565(r,r,r);
+}
+else {
+int y[4], u, v;
+for (k = 0; k < 4; ++k)
+/* 8-bit luminance values */
+y[k] = *data++;
 u = *(int8_t *)data++;
 v = *(int8_t *)data++;
-p -= 12;
-for(k=0; k<4; ++k) {
-r = *p++ + v*2;
-g = *p++ - (u/2) - v;
-b = *p   + u*2;
-p -= 2;
-*p++ = av_clip_uint8(r);
-*p++ = av_clip_uint8(g);
-*p++ = av_clip_uint8(b);
-}
+for(k=0; k<4; ++k)
+  

Re: [FFmpeg-devel] [PATCH] avformat: parse iTunes gapless information

[wm4]

On Thu, 26 Jan 2017 16:47:12 -0800
Christopher Snowhill  wrote:

> Signed-off-by: Christopher Snowhill 
> ---
>  libavformat/mp3dec.c | 77 
> +++-
>  1 file changed, 76 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/mp3dec.c b/libavformat/mp3dec.c
> index 099ca57..b895e37 100644
> --- a/libavformat/mp3dec.c
> +++ b/libavformat/mp3dec.c
> @@ -295,6 +295,66 @@ static void mp3_parse_vbri_tag(AVFormatContext *s, 
> AVStream *st, int64_t base)
>  }
>  }
>  
> +static void mp3_parse_itunes_tag(AVFormatContext *s, AVStream *st, 
> MPADecodeHeader *c, int64_t base, int vbrtag_size, unsigned int *size, 
> uint64_t *duration)
> +{
> +uint32_t v;
> +AVDictionaryEntry *de;
> +MP3DecContext *mp3 = s->priv_data;
> +size_t length;
> +uint32_t zero, start_pad, end_pad;
> +uint64_t last_eight_frames_offset;
> +int64_t temp_duration, file_size;
> +int i;
> +
> +if (!s->metadata || !(de = av_dict_get(s->metadata, "iTunSMPB", NULL, 
> 0)))
> +return;
> +
> +length = strlen(de->value);
> +
> +/* Minimum length is one digit per field plus the whitespace, maximum 
> length should depend on field type
> + * There are four fields we need in the first six, the rest are 
> currently zero padding */
> +if (length < (12 + 11) || length > (10 * 8 + 2 * 16 + 11))
> +return;
> +
> +file_size = avio_size(s->pb);
> +if (file_size < 0)
> +file_size = 0;
> +
> +if (sscanf(de->value, "%"PRIx32" %"PRIx32" %"PRIx32" %"PRIx64" %"PRIx32" 
> %"PRIx64, , _pad, _pad, _duration, , 
> _eight_frames_offset) < 6 ||
> +temp_duration < 0 ||
> +start_pad > (576 * 2 * 32) ||
> +end_pad > (576 * 2 * 64) ||
> +(file_size && (last_eight_frames_offset >= (file_size - base - 
> vbrtag_size {
> +*duration = 0;
> +return;
> +}
> +
> +*duration = temp_duration;
> +
> +mp3->start_pad = start_pad;
> +mp3->end_pad = end_pad;
> +if (end_pad >= 528 + 1)
> +mp3->end_pad = end_pad - (528 + 1);
> +st->start_skip_samples = mp3->start_pad + 528 + 1;
> +av_log(s, AV_LOG_DEBUG, "pad %d %d\n", mp3->start_pad, mp3->end_pad);
> +if (!s->pb->seekable)
> +return;
> +
> +*size = (unsigned int) last_eight_frames_offset;
> +if (avio_seek(s->pb, base + vbrtag_size + last_eight_frames_offset, 
> SEEK_SET) < 0)
> +return;
> +
> +for (i = 0; i < 8; i++) {
> +v = avio_rb32(s->pb);
> +if (ff_mpa_check_header(v) < 0)
> +return;
> +if (avpriv_mpegaudio_decode_header(c, v) != 0)
> +break;
> +*size += c->frame_size;
> +avio_skip(s->pb, c->frame_size - 4);
> +}
> +}
> +
>  /**
>   * Try to find Xing/Info/VBRI tags and compute duration from info therein
>   */
> @@ -303,8 +363,10 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, 
> AVStream *st, int64_t base)
>  uint32_t v, spf;
>  MPADecodeHeader c;
>  int vbrtag_size = 0;
> +unsigned int size = 0;
>  MP3DecContext *mp3 = s->priv_data;
>  int ret;
> +uint64_t duration = 0;
>  
>  ffio_init_checksum(s->pb, ff_crcA001_update, 0);
>  
> @@ -327,16 +389,29 @@ static int mp3_parse_vbr_tags(AVFormatContext *s, 
> AVStream *st, int64_t base)
>  mp3_parse_vbri_tag(s, st, base);
>  
>  if (!mp3->frames && !mp3->header_filesize)
> +vbrtag_size = 0;
> +
> +mp3_parse_itunes_tag(s, st, , base, vbrtag_size, , );
> +
> +if (!mp3->frames && !size && !duration)
>  return -1;
>  
>  /* Skip the vbr tag frame */
>  avio_seek(s->pb, base + vbrtag_size, SEEK_SET);
>  
> -if (mp3->frames)
> +if (duration)
> +st->duration = av_rescale_q(duration, (AVRational){1, 
> c.sample_rate}, st->time_base);
> +else if (mp3->frames)
>  st->duration = av_rescale_q(mp3->frames, (AVRational){spf, 
> c.sample_rate},
>  st->time_base);
>  if (mp3->header_filesize && mp3->frames && !mp3->is_cbr)
>  st->codecpar->bit_rate = av_rescale(mp3->header_filesize, 8 * 
> c.sample_rate, mp3->frames * (int64_t)spf);
> +if (size) {
> +if (duration)
> +st->codecpar->bit_rate = av_rescale(size, 8 * c.sample_rate, 
> duration);
> +else if (mp3->frames)
> +st->codecpar->bit_rate = av_rescale(size, 8 * c.sample_rate, 
> mp3->frames * (int64_t)spf);
> +}
>  
>  return 0;
>  }

If we add this, can we add a fate sample as well? The test should be
similar to the other gapless tests.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 5/9] nistspheredec: prevent overflow during block alignment calculation

[wm4]

On Thu, 26 Jan 2017 17:02:37 +0100
Michael Niedermayer  wrote:

> On Thu, Jan 26, 2017 at 06:43:45AM +0100, wm4 wrote:
> > On Thu, 26 Jan 2017 03:20:02 +0100
> > Michael Niedermayer  wrote:
> >   
> > > On Thu, Jan 26, 2017 at 02:58:07AM +0100, Andreas Cadhalpun wrote:  
> > > > On 26.01.2017 02:29, Ronald S. Bultje wrote:
> > > > > On Wed, Jan 25, 2017 at 8:12 PM, Andreas Cadhalpun <
> > > > > andreas.cadhal...@googlemail.com> wrote:
> > > > > 
> > > > >> Signed-off-by: Andreas Cadhalpun 
> > > > >> ---
> > > > >>  libavformat/nistspheredec.c | 11 +++
> > > > >>  1 file changed, 11 insertions(+)
> > > > >>
> > > > >> diff --git a/libavformat/nistspheredec.c 
> > > > >> b/libavformat/nistspheredec.c
> > > > >> index 782d1dfbfb..3386497682 100644
> > > > >> --- a/libavformat/nistspheredec.c
> > > > >> +++ b/libavformat/nistspheredec.c
> > > > >> @@ -21,6 +21,7 @@
> > > > >>
> > > > >>  #include "libavutil/avstring.h"
> > > > >>  #include "libavutil/intreadwrite.h"
> > > > >> +#include "libavcodec/internal.h"
> > > > >>  #include "avformat.h"
> > > > >>  #include "internal.h"
> > > > >>  #include "pcm.h"
> > > > >> @@ -90,6 +91,11 @@ static int nist_read_header(AVFormatContext *s)
> > > > >>  return 0;
> > > > >>  } else if (!memcmp(buffer, "channel_count", 13)) {
> > > > >>  sscanf(buffer, "%*s %*s %"SCNd32, 
> > > > >> >codecpar->channels);
> > > > >> +if (st->codecpar->channels > FF_SANE_NB_CHANNELS) {
> > > > >> +av_log(s, AV_LOG_ERROR, "Too many channels %d > 
> > > > >> %d\n",
> > > > >> +   st->codecpar->channels, FF_SANE_NB_CHANNELS);
> > > > >> +return AVERROR(ENOSYS);
> > > > >> +}
> > > > > 
> > > > > 
> > > > > I've said this before, but again - please don't add useless log 
> > > > > messages.
> > > > 
> > > > I disagree that these log messages are useless and I'm not the only one 
> > > > [1].
> > > 
> > > +1
> > > 
> > > Log messages make debuging the code easier, as a developer i like to
> > > know why something fails having a hard failure but no clear and easy
> > > findable error message is bad
> > > 
> > > 
> > > [...]  
> > 
> > -1
> > 
> > This kind of things bloat the code with rare corner cases. One point
> > would be that this increases binary size (why do we even still have the
> > NULL_IF_CONFIG_SMALL macro? I'm not saying we should use it here, but
> > the current use of the macro will barely even make a dent into the
> > number of bytes "wasted" for optional strings, yet we bother).
> > 
> > Another point is that code becomes unreadable if obscure corner cases
> > take up most of the code. I think that's w worrisome direction we're
> > taking here.  
> 
> While it doesnt apply to this patch here but,
> Error messages in obscure checks that describe the error condition
> in the source would make at least some checks easier to understand
> than just a generic
> "return AVERROR_INVALIDDATA"

In my opinion, there's no choice but to debug such cases manually
anyway. Whether you think that log messages help you better than
breakpoints in a source debugger is a different question.

> > 
> > When I debug FFmpeg API use, the thing that annoys me most is that I
> > can't know where an error happens. I have to trace the origin manually.
> > Error codes are almost always completely useless. This patchset adds a
> > lot of NOSYS, which is used 254 times in FFmpeg and which is about 99%
> > meaningless and resolves to an even worse error message when using
> > av_err2str(). Adding an av_log to error error point would "work" (at
> > least you could use grep), but isn't a good solution.  
> 
> I think the idea about something more informative than a int32 error
> code did come up previously.
> I certainly would be in favor of having an error value that could be
> used to pinpoint the error location(s) and or function(s) it passed
> through, this would be usefull for debguging in general

We could probably return a struct that contains the __FILE__ etc. value
of the error site, but this would probably be too over-engineered and
intrusive for a real solution.

Back to this patch and the core issue: I think there's no ideal
solution. So balance is required. Don't add av_logs to every error, but
add them to cases that are interesting and would be otherwise hard to
find.

I think fine grained error logging for read_header in a really obscure
and tiny demuxer isn't an interesting/useful case.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]lavf/mov: Export vendor metadata

[wm4]

On Fri, 27 Jan 2017 12:35:25 +0100
Carl Eugen Hoyos  wrote:

> 2017-01-27 11:55 GMT+01:00 wm4 :
> > On Fri, 27 Jan 2017 11:21:08 +0100
> > Carl Eugen Hoyos  wrote:
> >  
> >> 2017-01-27 10:42 GMT+01:00 wm4 :  
> >> > On Fri, 27 Jan 2017 10:38:23 +0100
> >> > Carl Eugen Hoyos  wrote:
> >> >  
> >> >> 2017-01-27 10:29 GMT+01:00 wm4 :  
> >> >> > On Fri, 27 Jan 2017 10:19:32 +0100
> >> >> > Carl Eugen Hoyos  wrote:
> >> >> >  
> >> >> >> 2017-01-27 10:09 GMT+01:00 wm4 :  
> >> >> >> > On Fri, 27 Jan 2017 09:39:03 +0100
> >> >> >> > Carl Eugen Hoyos  wrote:
> >> >> >> >  
> >> >> >> >> 2017-01-27 9:17 GMT+01:00 wm4 :
> >> >> >> >>  
> >> >> >> >> > You're completely misunderstanding.  
> >> >> >> >>
> >> >> >> >> Would you mind to elaborate?  
> >> >> >> >
> >> >> >> > FFmpeg shouldn't mux codec-specific tags into a different
> >> >> >> > container.  
> >> >> >>
> >> >> >> This is not codec-specific, at least not in the sense that
> >> >> >> would make a difference for other containers.
> >> >> >>  
> >> >> >> > Your ffmpeg.c patch works for transcoding only, not remuxing.  
> >> >> >>
> >> >> >> That's because it makes sense to remux "vendor" metadata.  
> >> >> >
> >> >> > No, technical metadata that is "hidden" is different from user-visible
> >> >> > tags that contain non-technical information about the media.  
> >> >>
> >> >> This is non-technical information that should be user-visible.  
> >> >
> >> > The vendor tag in your fate change has the value "appl". How should
> >> > that be user-visible?  
> >>
> >> You mean the information is encoded in such a difficult manner
> >> that no user will be able to decipher it?  
> >
> > That too. It's essentially a FourCC. Most people don't know what to do
> > with FourCC. It's something for developers or otherwise technically
> > inclined people.  
> 
> I disagree.
> (And so do other developers it seems.)

Which other developers?

> > While this strengthens my argument, it's not really the main point.
> >  
> >> I really wish you were a little more serious when trying to slow down
> >> FFmpeg development.  
> >
> > Please refrain from personal attacks.  
> 
> I do not consider this a personal attack: People you seem to respect
> have often requested (publically and in private) that FFmpeg development
> has to slow down - I strongly disagree with them but you have said in the
> past that you agree with them.

Where did they say this? Do you have a link or an example?

> > I'm not trying to slow down
> > development, I'm trying to improve the general quality of FFmpeg. In
> > fact, that's the main point of patch reviews: point out weak spots,
> > request improvements, and so on.  
> 
> But that is not how you started this thread, or was it?

Did you?

> And that's apart from the fact that you have threatened contributors
> away in the past.

How many people have you chased away from the bug tracker?

> > In this specific case, you're exporting a technical details as
> > user tags. This is a practice that should stop. Also, writing
> > essentially random garbage as tags or writing it to files in a manner
> > that doesn't make sense is not what I associate with quality. What
> > sense does it make to write a stringified FourCC as user-tag to, say,
> > a mkv file?  
> 
> I do not understand in which way the vendor tag "doesn't make sense"
> in any container.

See the paragraph you quoted.

Let me ask you again: how does an Apple-specific FourCC make sense in
mkv (except maybe the Qt muxing support)?

> > Last but not least, it's questionable whether this is a meaningful
> > feature at all. Why not use a tool that's more suited for the job, like
> > a mp4-specific file dumper? (Why did you not do the same for audio?)  
> 
> I would really, really prefer if all necessary debug information would be
> printed by FFmpeg, not a third-party toold.

Adding user-visible changes "for debugging" is usually a bad argument.

You could output it with an av_log with debug log level if you must.

> > To put this into perspective, mediainfo -f doesn't seem to export this
> > value.  
> 
> This does not sound like a useful argument to me.

It does to me.

> >> > You can't seriously tell me that this should show
> >> > up in a non-technical UI view.  
> >>
> >> Since you are blocking much more important patches, I should
> >> probably not spend so much time discussing this one. But the
> >> fact that I needed this patch for debugging and that I found  out
> >> afterwards that other developers need it too sufficiently indicates
> >> to me showing, exporting and remuxing the vendor tag makes sense.  
> >
> > For debugging you probably want to use a tool like boxdumper.  
> 
> See above.

See above.

> > FFmpeg can't and won't be able to do "everything" a more suited tool
> > 

Re: [FFmpeg-devel] [PATCH] avfilter: do not write to unwritable frame

[wm4]

On Fri, 27 Jan 2017 18:42:03 +0700
Muhammad Faiz  wrote:

> affect filters that set partial_buf_size
> test-case
> ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp, asplit 
> [a][b];
> [a] firequalizer=fixed=on, showcqt=s=1280x360 [a1];
> [b] firequalizer=fixed=on, showcqt=s=1280x360 [b1];
> [a1][b1] vstack'
> 
> Signed-off-by: Muhammad Faiz 
> ---
>  libavfilter/avfilter.c | 17 +
>  1 file changed, 17 insertions(+)
> 
> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> index c12d491..7e7e83c 100644
> --- a/libavfilter/avfilter.c
> +++ b/libavfilter/avfilter.c
> @@ -1235,6 +1235,23 @@ static int take_samples(AVFilterLink *link, unsigned 
> min, unsigned max,
>  frame = ff_framequeue_peek(>fifo, 0);
>  av_samples_copy(buf->extended_data, frame->extended_data, p, 0, n,
>  link->channels, link->format);
> +
> +if (!av_frame_is_writable(frame)) {
> +AVFrame *new = ff_get_audio_buffer(link, frame->nb_samples);
> +if (!new)
> +return AVERROR(ENOMEM);
> +ret = av_frame_copy_props(new, frame);
> +if (ret < 0)
> +return ret;
> +av_samples_copy(new->extended_data, frame->extended_data,
> +0, 0, frame->nb_samples,
> +av_frame_get_channels(frame),
> +frame->format);
> +av_frame_unref(frame);
> +av_frame_move_ref(frame, new);
> +av_frame_free();
> +}
> +
>  frame->nb_samples -= n;
>  av_samples_copy(frame->extended_data, frame->extended_data, 0, n,
>  frame->nb_samples, link->channels, link->format);

There's a function that does this automatically:
av_frame_make_writable()

Might not be fully equivalent though due to ff_get_audio_buffer()
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] avfilter: do not write to unwritable frame

[Muhammad Faiz]

affect filters that set partial_buf_size
test-case
ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp, asplit 
[a][b];
[a] firequalizer=fixed=on, showcqt=s=1280x360 [a1];
[b] firequalizer=fixed=on, showcqt=s=1280x360 [b1];
[a1][b1] vstack'

Signed-off-by: Muhammad Faiz 
---
 libavfilter/avfilter.c | 17 +
 1 file changed, 17 insertions(+)

diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
index c12d491..7e7e83c 100644
--- a/libavfilter/avfilter.c
+++ b/libavfilter/avfilter.c
@@ -1235,6 +1235,23 @@ static int take_samples(AVFilterLink *link, unsigned 
min, unsigned max,
 frame = ff_framequeue_peek(>fifo, 0);
 av_samples_copy(buf->extended_data, frame->extended_data, p, 0, n,
 link->channels, link->format);
+
+if (!av_frame_is_writable(frame)) {
+AVFrame *new = ff_get_audio_buffer(link, frame->nb_samples);
+if (!new)
+return AVERROR(ENOMEM);
+ret = av_frame_copy_props(new, frame);
+if (ret < 0)
+return ret;
+av_samples_copy(new->extended_data, frame->extended_data,
+0, 0, frame->nb_samples,
+av_frame_get_channels(frame),
+frame->format);
+av_frame_unref(frame);
+av_frame_move_ref(frame, new);
+av_frame_free();
+}
+
 frame->nb_samples -= n;
 av_samples_copy(frame->extended_data, frame->extended_data, 0, n,
 frame->nb_samples, link->channels, link->format);
-- 
2.5.0

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

Re: [FFmpeg-devel] [PATCH]lavf/mov: Export vendor metadata

[Carl Eugen Hoyos]

2017-01-27 11:55 GMT+01:00 wm4 :
> On Fri, 27 Jan 2017 11:21:08 +0100
> Carl Eugen Hoyos  wrote:
>
>> 2017-01-27 10:42 GMT+01:00 wm4 :
>> > On Fri, 27 Jan 2017 10:38:23 +0100
>> > Carl Eugen Hoyos  wrote:
>> >
>> >> 2017-01-27 10:29 GMT+01:00 wm4 :
>> >> > On Fri, 27 Jan 2017 10:19:32 +0100
>> >> > Carl Eugen Hoyos  wrote:
>> >> >
>> >> >> 2017-01-27 10:09 GMT+01:00 wm4 :
>> >> >> > On Fri, 27 Jan 2017 09:39:03 +0100
>> >> >> > Carl Eugen Hoyos  wrote:
>> >> >> >
>> >> >> >> 2017-01-27 9:17 GMT+01:00 wm4 :
>> >> >> >>
>> >> >> >> > You're completely misunderstanding.
>> >> >> >>
>> >> >> >> Would you mind to elaborate?
>> >> >> >
>> >> >> > FFmpeg shouldn't mux codec-specific tags into a different
>> >> >> > container.
>> >> >>
>> >> >> This is not codec-specific, at least not in the sense that
>> >> >> would make a difference for other containers.
>> >> >>
>> >> >> > Your ffmpeg.c patch works for transcoding only, not remuxing.
>> >> >>
>> >> >> That's because it makes sense to remux "vendor" metadata.
>> >> >
>> >> > No, technical metadata that is "hidden" is different from user-visible
>> >> > tags that contain non-technical information about the media.
>> >>
>> >> This is non-technical information that should be user-visible.
>> >
>> > The vendor tag in your fate change has the value "appl". How should
>> > that be user-visible?
>>
>> You mean the information is encoded in such a difficult manner
>> that no user will be able to decipher it?
>
> That too. It's essentially a FourCC. Most people don't know what to do
> with FourCC. It's something for developers or otherwise technically
> inclined people.

I disagree.
(And so do other developers it seems.)

> While this strengthens my argument, it's not really the main point.
>
>> I really wish you were a little more serious when trying to slow down
>> FFmpeg development.
>
> Please refrain from personal attacks.

I do not consider this a personal attack: People you seem to respect
have often requested (publically and in private) that FFmpeg development
has to slow down - I strongly disagree with them but you have said in the
past that you agree with them.

> I'm not trying to slow down
> development, I'm trying to improve the general quality of FFmpeg. In
> fact, that's the main point of patch reviews: point out weak spots,
> request improvements, and so on.

But that is not how you started this thread, or was it?

And that's apart from the fact that you have threatened contributors
away in the past.

> In this specific case, you're exporting a technical details as
> user tags. This is a practice that should stop. Also, writing
> essentially random garbage as tags or writing it to files in a manner
> that doesn't make sense is not what I associate with quality. What
> sense does it make to write a stringified FourCC as user-tag to, say,
> a mkv file?

I do not understand in which way the vendor tag "doesn't make sense"
in any container.

> Last but not least, it's questionable whether this is a meaningful
> feature at all. Why not use a tool that's more suited for the job, like
> a mp4-specific file dumper? (Why did you not do the same for audio?)

I would really, really prefer if all necessary debug information would be
printed by FFmpeg, not a third-party toold.

> To put this into perspective, mediainfo -f doesn't seem to export this
> value.

This does not sound like a useful argument to me.

>> > You can't seriously tell me that this should show
>> > up in a non-technical UI view.
>>
>> Since you are blocking much more important patches, I should
>> probably not spend so much time discussing this one. But the
>> fact that I needed this patch for debugging and that I found  out
>> afterwards that other developers need it too sufficiently indicates
>> to me showing, exporting and remuxing the vendor tag makes sense.
>
> For debugging you probably want to use a tool like boxdumper.

See above.

> FFmpeg can't and won't be able to do "everything" a more suited tool
> can do, because it's simply out of scope, and would flood us with
> thousands of normally useless details.

How is exporting properties of a media file out-of-scope for FFmpeg?

> I'm also sad to see that you consider discussing patches an annoyance.

I wish you would have commented earlier, the way you did it gave me
the impression you were only interested in annoying me.

> Besides, aren't you one of the main offenders when it comes to
> stubbornly blocking certain changes?

Compared to you?

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

Re: [FFmpeg-devel] [PATCH]lavf/mov: Export vendor metadata

[wm4]

On Fri, 27 Jan 2017 11:21:08 +0100
Carl Eugen Hoyos  wrote:

> 2017-01-27 10:42 GMT+01:00 wm4 :
> > On Fri, 27 Jan 2017 10:38:23 +0100
> > Carl Eugen Hoyos  wrote:
> >  
> >> 2017-01-27 10:29 GMT+01:00 wm4 :  
> >> > On Fri, 27 Jan 2017 10:19:32 +0100
> >> > Carl Eugen Hoyos  wrote:
> >> >  
> >> >> 2017-01-27 10:09 GMT+01:00 wm4 :  
> >> >> > On Fri, 27 Jan 2017 09:39:03 +0100
> >> >> > Carl Eugen Hoyos  wrote:
> >> >> >  
> >> >> >> 2017-01-27 9:17 GMT+01:00 wm4 :
> >> >> >>  
> >> >> >> > You're completely misunderstanding.  
> >> >> >>
> >> >> >> Would you mind to elaborate?  
> >> >> >
> >> >> > FFmpeg shouldn't mux codec-specific tags into a different
> >> >> > container.  
> >> >>
> >> >> This is not codec-specific, at least not in the sense that
> >> >> would make a difference for other containers.
> >> >>  
> >> >> > Your ffmpeg.c patch works for transcoding only, not remuxing.  
> >> >>
> >> >> That's because it makes sense to remux "vendor" metadata.  
> >> >
> >> > No, technical metadata that is "hidden" is different from user-visible
> >> > tags that contain non-technical information about the media.  
> >>
> >> This is non-technical information that should be user-visible.  
> >
> > The vendor tag in your fate change has the value "appl". How should
> > that be user-visible?  
> 
> You mean the information is encoded in such a difficult manner
> that no user will be able to decipher it?

That too. It's essentially a FourCC. Most people don't know what to do
with FourCC. It's something for developers or otherwise technically
inclined people.

While this strengthens my argument, it's not really the main point.

> I really wish you were a little more serious when trying to slow down
> FFmpeg development.

Please refrain from personal attacks. I'm not trying to slow down
development, I'm trying to improve the general quality of FFmpeg. In
fact, that's the main point of patch reviews: point out weak spots,
request improvements, and so on.

In this specific case, you're exporting a technical details as
user tags. This is a practice that should stop. Also, writing
essentially random garbage as tags or writing it to files in a manner
that doesn't make sense is not what I associate with quality. What
sense does it make to write a stringified FourCC as user-tag to, say,
a mkv file?

Last but not least, it's questionable whether this is a meaningful
feature at all. Why not use a tool that's more suited for the job, like
a mp4-specific file dumper? (Why did you not do the same for audio?)

To put this into perspective, mediainfo -f doesn't seem to export this
value.

> > You can't seriously tell me that this should show
> > up in a non-technical UI view.  
> 
> Since you are blocking much more important patches, I should
> probably not spend so much time discussing this one. But the
> fact that I needed this patch for debugging and that I found  out
> afterwards that other developers need it too sufficiently indicates
> to me showing, exporting and remuxing the vendor tag makes sense.

For debugging you probably want to use a tool like boxdumper. FFmpeg
can't and won't be able to do "everything" a more suited tool can do,
because it's simply out of scope, and would flood us with thousands of
normally useless details.

I'm also sad to see that you consider discussing patches an annoyance.

Besides, aren't you one of the main offenders when it comes to
stubbornly blocking certain changes?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] fate: add SCC test

[Paul B Mahol]

On 1/26/17, Michael Niedermayer  wrote:
> On Wed, Jan 25, 2017 at 10:30:06PM +0100, Paul B Mahol wrote:
>> Signed-off-by: Paul B Mahol 
>> ---
>>  tests/fate/subtitles.mak |   3 +
>>  tests/ref/fate/sub-scc   | 519
>> +++
>>  2 files changed, 522 insertions(+)
>>  create mode 100644 tests/ref/fate/sub-scc
>
> Tested on linux32/64 x86, mingw32/64, mips/arm qemu linux
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Republics decline into democracies and democracies degenerate into
> despotisms. -- Aristotle
>

I can not push this because it aborts:

remote: -Info-  Update is fast-forward
remote: -Deny-  In c0cad837f922bcf2eb6220fd129c02188d26002b:
remote: -Deny-  Trailing whitespace found in tests/ref/fate/sub-scc.
remote: -Deny-  Commit aborted, fix the issue and try again.
remote: error: hook declined to update refs/heads/master
To source.ffmpeg.org:ffmpeg.git
 ! [remote rejected] scc -> master (hook declined)
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]lavf/mov: Export vendor metadata

[Carl Eugen Hoyos]

2017-01-27 10:42 GMT+01:00 wm4 :
> On Fri, 27 Jan 2017 10:38:23 +0100
> Carl Eugen Hoyos  wrote:
>
>> 2017-01-27 10:29 GMT+01:00 wm4 :
>> > On Fri, 27 Jan 2017 10:19:32 +0100
>> > Carl Eugen Hoyos  wrote:
>> >
>> >> 2017-01-27 10:09 GMT+01:00 wm4 :
>> >> > On Fri, 27 Jan 2017 09:39:03 +0100
>> >> > Carl Eugen Hoyos  wrote:
>> >> >
>> >> >> 2017-01-27 9:17 GMT+01:00 wm4 :
>> >> >>
>> >> >> > You're completely misunderstanding.
>> >> >>
>> >> >> Would you mind to elaborate?
>> >> >
>> >> > FFmpeg shouldn't mux codec-specific tags into a different
>> >> > container.
>> >>
>> >> This is not codec-specific, at least not in the sense that
>> >> would make a difference for other containers.
>> >>
>> >> > Your ffmpeg.c patch works for transcoding only, not remuxing.
>> >>
>> >> That's because it makes sense to remux "vendor" metadata.
>> >
>> > No, technical metadata that is "hidden" is different from user-visible
>> > tags that contain non-technical information about the media.
>>
>> This is non-technical information that should be user-visible.
>
> The vendor tag in your fate change has the value "appl". How should
> that be user-visible?

You mean the information is encoded in such a difficult manner
that no user will be able to decipher it?
I really wish you were a little more serious when trying to slow down
FFmpeg development.

> You can't seriously tell me that this should show
> up in a non-technical UI view.

Since you are blocking much more important patches, I should
probably not spend so much time discussing this one. But the
fact that I needed this patch for debugging and that I found  out
afterwards that other developers need it too sufficiently indicates
to me showing, exporting and remuxing the vendor tag makes sense.

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

Re: [FFmpeg-devel] [PATCH]lavf/mov: Export vendor metadata

[Paul B Mahol]

On 12/12/16, Carl Eugen Hoyos  wrote:
> Hi!
>
> I saw only after writing this patch that a Google employee has sent
> a more complicated variant of this patch in November (searching for
> a description of the vendor field showed the patch).
>
> Please comment, Carl Eugen
>

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

Re: [FFmpeg-devel] [PATCH]lavf/mov: Export vendor metadata

[wm4]

On Fri, 27 Jan 2017 10:38:23 +0100
Carl Eugen Hoyos  wrote:

> 2017-01-27 10:29 GMT+01:00 wm4 :
> > On Fri, 27 Jan 2017 10:19:32 +0100
> > Carl Eugen Hoyos  wrote:
> >  
> >> 2017-01-27 10:09 GMT+01:00 wm4 :  
> >> > On Fri, 27 Jan 2017 09:39:03 +0100
> >> > Carl Eugen Hoyos  wrote:
> >> >  
> >> >> 2017-01-27 9:17 GMT+01:00 wm4 :
> >> >>  
> >> >> > You're completely misunderstanding.  
> >> >>
> >> >> Would you mind to elaborate?  
> >> >
> >> > FFmpeg shouldn't mux codec-specific tags into a different
> >> > container.  
> >>
> >> This is not codec-specific, at least not in the sense that
> >> would make a difference for other containers.
> >>  
> >> > Your ffmpeg.c patch works for transcoding only, not remuxing.  
> >>
> >> That's because it makes sense to remux "vendor" metadata.  
> >
> > No, technical metadata that is "hidden" is different from user-visible
> > tags that contain non-technical information about the media.  
> 
> This is non-technical information that should be user-visible.

The vendor tag in your fate change has the value "appl". How should
that be user-visible? You can't seriously tell me that this should show
up in a non-technical UI view.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]lavf/mov: Export vendor metadata

[Carl Eugen Hoyos]

2017-01-27 10:29 GMT+01:00 wm4 :
> On Fri, 27 Jan 2017 10:19:32 +0100
> Carl Eugen Hoyos  wrote:
>
>> 2017-01-27 10:09 GMT+01:00 wm4 :
>> > On Fri, 27 Jan 2017 09:39:03 +0100
>> > Carl Eugen Hoyos  wrote:
>> >
>> >> 2017-01-27 9:17 GMT+01:00 wm4 :
>> >>
>> >> > You're completely misunderstanding.
>> >>
>> >> Would you mind to elaborate?
>> >
>> > FFmpeg shouldn't mux codec-specific tags into a different
>> > container.
>>
>> This is not codec-specific, at least not in the sense that
>> would make a difference for other containers.
>>
>> > Your ffmpeg.c patch works for transcoding only, not remuxing.
>>
>> That's because it makes sense to remux "vendor" metadata.
>
> No, technical metadata that is "hidden" is different from user-visible
> tags that contain non-technical information about the media.

This is non-technical information that should be user-visible.

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

Re: [FFmpeg-devel] [PATCH]lavf/mov: Export vendor metadata

[wm4]

On Fri, 27 Jan 2017 10:19:32 +0100
Carl Eugen Hoyos  wrote:

> 2017-01-27 10:09 GMT+01:00 wm4 :
> > On Fri, 27 Jan 2017 09:39:03 +0100
> > Carl Eugen Hoyos  wrote:
> >  
> >> 2017-01-27 9:17 GMT+01:00 wm4 :
> >>  
> >> > You're completely misunderstanding.  
> >>
> >> Would you mind to elaborate?  
> >
> > FFmpeg shouldn't mux codec-specific tags into a different
> > container.  
> 
> This is not codec-specific, at least not in the sense that
> would make a difference for other containers.
> 
> > Your ffmpeg.c patch works for transcoding only, not remuxing.  
> 
> That's because it makes sense to remux "vendor" metadata.

No, technical metadata that is "hidden" is different from user-visible
tags that contain non-technical information about the media. Don't mix
them. FFmpeg does, and this is bad.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]lavf/mov: Export vendor metadata

[Carl Eugen Hoyos]

2017-01-27 10:09 GMT+01:00 wm4 :
> On Fri, 27 Jan 2017 09:39:03 +0100
> Carl Eugen Hoyos  wrote:
>
>> 2017-01-27 9:17 GMT+01:00 wm4 :
>>
>> > You're completely misunderstanding.
>>
>> Would you mind to elaborate?
>
> FFmpeg shouldn't mux codec-specific tags into a different
> container.

This is not codec-specific, at least not in the sense that
would make a difference for other containers.

> Your ffmpeg.c patch works for transcoding only, not remuxing.

That's because it makes sense to remux "vendor" metadata.

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

Re: [FFmpeg-devel] [PATCH]lavf/mov: Export vendor metadata

[wm4]

On Fri, 27 Jan 2017 09:39:03 +0100
Carl Eugen Hoyos  wrote:

> 2017-01-27 9:17 GMT+01:00 wm4 :
> 
> > You're completely misunderstanding.  
> 
> Would you mind to elaborate?

FFmpeg shouldn't mux codec-specific tags into a different container.

Your ffmpeg.c patch works for transcoding only, not remuxing. Also, an
API user can't distinguish user tags and container-specific metadata
added by libavformat, which is a problem.

In theory, you have to add this information as side-data. Adding a new
side-data type is a bit roundabout, though.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] lavc/mjpegdec: hint next marker position in ff_mjpeg_find_marker

[Matthieu Bouron]

On Fri, Jan 27, 2017 at 02:30:40AM +0100, Michael Niedermayer wrote:
> On Thu, Jan 26, 2017 at 05:47:51PM +0100, Matthieu Bouron wrote:
> > Speeds up next marker search when a SOS marker is found.
> > 
> > ff_mjpeg_find_marker goes from 54% to 30% under perf record ffmpeg -i
> > sample_2992x4000.jpg
> > ---
> >  libavcodec/mjpegdec.c | 31 +--
> >  libavcodec/mjpegdec.h |  2 +-
> >  libavcodec/mxpegdec.c |  5 +++--
> >  3 files changed, 29 insertions(+), 9 deletions(-)
> 
> this breaks decoding multiple jpeg files
> for example tickets//856/nint.jpg

New patch attached fixing the issue.

[...]
>From ebef63ebdc75872d52529d5b74b6d05254d4c0fd Mon Sep 17 00:00:00 2001
From: Matthieu Bouron 
Date: Thu, 26 Jan 2017 15:17:36 +0100
Subject: [PATCH] lavc/mjpegdec: hint next marker position in
 ff_mjpeg_find_marker

Speeds up next marker search when a SOS marker is found.

ff_mjpeg_find_marker goes from 54% to 30% under perf record ffmpeg -i
sample_2992x4000.jpg
---
 libavcodec/mjpegdec.c | 31 +--
 libavcodec/mjpegdec.h |  2 +-
 libavcodec/mxpegdec.c |  5 +++--
 3 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
index 7d17e3dfcb..347c363da0 100644
--- a/libavcodec/mjpegdec.c
+++ b/libavcodec/mjpegdec.c
@@ -1921,13 +1921,23 @@ static int mjpeg_decode_com(MJpegDecodeContext *s)
 
 /* return the 8 bit start code value and update the search
state. Return -1 if no start code found */
-static int find_marker(const uint8_t **pbuf_ptr, const uint8_t *buf_end)
+static int find_marker(const uint8_t **pbuf_ptr, const uint8_t *buf_hint, const uint8_t *buf_end)
 {
 const uint8_t *buf_ptr;
 unsigned int v, v2;
 int val;
 int skipped = 0;
 
+buf_ptr = buf_hint;
+if (buf_ptr && (buf_end - buf_ptr > 1)) {
+v = *buf_ptr++;
+v2 = *buf_ptr;
+if ((v == 0xff) && (v2 >= 0xc0) && (v2 <= 0xfe) && buf_ptr < buf_end) {
+val = *buf_ptr++;
+goto found;
+}
+}
+
 buf_ptr = *pbuf_ptr;
 while (buf_end - buf_ptr > 1) {
 v  = *buf_ptr++;
@@ -1947,12 +1957,15 @@ found:
 }
 
 int ff_mjpeg_find_marker(MJpegDecodeContext *s,
- const uint8_t **buf_ptr, const uint8_t *buf_end,
+ const uint8_t **buf_ptr,
+ const uint8_t **buf_hint,
+ const uint8_t *buf_end,
  const uint8_t **unescaped_buf_ptr,
  int *unescaped_buf_size)
 {
 int start_code;
-start_code = find_marker(buf_ptr, buf_end);
+start_code = find_marker(buf_ptr, *buf_hint, buf_end);
+*buf_hint = NULL;
 
 av_fast_padded_malloc(>buffer, >buffer_size, buf_end - *buf_ptr);
 if (!s->buffer)
@@ -1963,6 +1976,7 @@ int ff_mjpeg_find_marker(MJpegDecodeContext *s,
 const uint8_t *src = *buf_ptr;
 const uint8_t *ptr = src;
 uint8_t *dst = s->buffer;
+const uint8_t *next_marker = NULL;
 
 #define copy_data_segment(skip) do {   \
 ptrdiff_t length = (ptr - src) - (skip);  \
@@ -1999,8 +2013,10 @@ int ff_mjpeg_find_marker(MJpegDecodeContext *s,
 
 if (x < 0xd0 || x > 0xd7) {
 copy_data_segment(1);
-if (x)
+if (x) {
+next_marker = ptr - 2;
 break;
+}
 }
 }
 }
@@ -2009,6 +2025,8 @@ int ff_mjpeg_find_marker(MJpegDecodeContext *s,
 }
 #undef copy_data_segment
 
+if (next_marker)
+*buf_hint = next_marker;
 *unescaped_buf_ptr  = s->buffer;
 *unescaped_buf_size = dst - s->buffer;
 memset(s->buffer + *unescaped_buf_size, 0,
@@ -2073,7 +2091,7 @@ int ff_mjpeg_decode_frame(AVCodecContext *avctx, void *data, int *got_frame,
 const uint8_t *buf = avpkt->data;
 int buf_size   = avpkt->size;
 MJpegDecodeContext *s = avctx->priv_data;
-const uint8_t *buf_end, *buf_ptr;
+const uint8_t *buf_end, *buf_hint, *buf_ptr;
 const uint8_t *unescaped_buf_ptr;
 int hshift, vshift;
 int unescaped_buf_size;
@@ -2087,10 +2105,11 @@ int ff_mjpeg_decode_frame(AVCodecContext *avctx, void *data, int *got_frame,
 s->adobe_transform = -1;
 
 buf_ptr = buf;
+buf_hint = NULL;
 buf_end = buf + buf_size;
 while (buf_ptr < buf_end) {
 /* find start next marker */
-start_code = ff_mjpeg_find_marker(s, _ptr, buf_end,
+start_code = ff_mjpeg_find_marker(s, _ptr, _hint, buf_end,
   _buf_ptr,
   _buf_size);
 /* EOF */
diff --git a/libavcodec/mjpegdec.h b/libavcodec/mjpegdec.h
index fb811294a1..c9d289187b 100644
--- a/libavcodec/mjpegdec.h
+++ 

Re: [FFmpeg-devel] [PATCH]lavf/mov: Export vendor metadata

[Carl Eugen Hoyos]

2017-01-27 9:17 GMT+01:00 wm4 :

> You're completely misunderstanding.

Would you mind to elaborate?

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

Re: [FFmpeg-devel] [PATCH]lavf/mov: Export vendor metadata

[wm4]

On Fri, 27 Jan 2017 09:05:15 +0100
Carl Eugen Hoyos  wrote:

> 2017-01-27 8:56 GMT+01:00 wm4 :
> > On Fri, 27 Jan 2017 08:26:26 +0100
> > Carl Eugen Hoyos  wrote:
> >  
> >> 2017-01-27 7:04 GMT+01:00 wm4 :  
> >> > On Thu, 26 Jan 2017 18:06:39 +0100
> >> > Carl Eugen Hoyos  wrote:
> >> >  
> >> >> 2017-01-26 9:26 GMT+01:00 wm4 :  
> >> >> > On Thu, 26 Jan 2017 09:16:00 +0100
> >> >> > Carl Eugen Hoyos  wrote:
> >> >> >  
> >> >> >> 2017-01-26 9:07 GMT+01:00 wm4 :
> >> >> >>  
> >> >> >> >> >> > Any metadata you export can and will get copied to a new 
> >> >> >> >> >> > file when
> >> >> >> >> >> > remuxing, therefor exporting arbitrary info that isn't 
> >> >> >> >> >> > actual stream
> >> >> >> >> >> > metadata tags in metadata is problematic - it carries over 
> >> >> >> >> >> > to the
> >> >> >> >> >> > destination file, in which it would be entirely meaningless. 
> >> >> >> >> >> >  
> >> >> >> >> >>
> >> >> >> >> >> Sorry, I don't understand:
> >> >> >> >> >> Which application do you mean?  
> >> >> >> >> >
> >> >> >> >> > ffmpeg  
> >> >> >> >>
> >> >> >> >> Didn't I ping yesterday a patch that avoids this?
> >> >> >> >> And wasn't this the mail you answered?  
> >> >> >> >
> >> >> >> > Sorry, I couldn't find it just now. What was the subject line?  
> >> >> >>
> >> >> >> My mailer (gmail) shows it as the first mail of this thread, so
> >> >> >> it (hopefully) uses the same subject.  
> >> >> >
> >> >> > Sorry, I missed that. But this code is not called for remuxing,
> >> >> > or is it?  
> >> >>
> >> >> Afair, this behaviour (copying the vendor on remuxing) is
> >> >> consistent with the Apple documentation.  
> >> >
> >> > What if you remux to mkv?  
> >>
> >> Afair, copying the vendor tag on remuxing is consistent with
> >> its documentation.  
> >
> > Well, the Apple docs don't have anything to do with mkv?  
> 
> So mkv has a specific definition of the vendor metadata?
> I did not immediately find it, could you point me to it?

You're completely misunderstanding.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH]lavf/mov: Export vendor metadata

[Carl Eugen Hoyos]

2017-01-27 8:56 GMT+01:00 wm4 :
> On Fri, 27 Jan 2017 08:26:26 +0100
> Carl Eugen Hoyos  wrote:
>
>> 2017-01-27 7:04 GMT+01:00 wm4 :
>> > On Thu, 26 Jan 2017 18:06:39 +0100
>> > Carl Eugen Hoyos  wrote:
>> >
>> >> 2017-01-26 9:26 GMT+01:00 wm4 :
>> >> > On Thu, 26 Jan 2017 09:16:00 +0100
>> >> > Carl Eugen Hoyos  wrote:
>> >> >
>> >> >> 2017-01-26 9:07 GMT+01:00 wm4 :
>> >> >>
>> >> >> >> >> > Any metadata you export can and will get copied to a new file 
>> >> >> >> >> > when
>> >> >> >> >> > remuxing, therefor exporting arbitrary info that isn't actual 
>> >> >> >> >> > stream
>> >> >> >> >> > metadata tags in metadata is problematic - it carries over to 
>> >> >> >> >> > the
>> >> >> >> >> > destination file, in which it would be entirely meaningless.
>> >> >> >> >>
>> >> >> >> >> Sorry, I don't understand:
>> >> >> >> >> Which application do you mean?
>> >> >> >> >
>> >> >> >> > ffmpeg
>> >> >> >>
>> >> >> >> Didn't I ping yesterday a patch that avoids this?
>> >> >> >> And wasn't this the mail you answered?
>> >> >> >
>> >> >> > Sorry, I couldn't find it just now. What was the subject line?
>> >> >>
>> >> >> My mailer (gmail) shows it as the first mail of this thread, so
>> >> >> it (hopefully) uses the same subject.
>> >> >
>> >> > Sorry, I missed that. But this code is not called for remuxing,
>> >> > or is it?
>> >>
>> >> Afair, this behaviour (copying the vendor on remuxing) is
>> >> consistent with the Apple documentation.
>> >
>> > What if you remux to mkv?
>>
>> Afair, copying the vendor tag on remuxing is consistent with
>> its documentation.
>
> Well, the Apple docs don't have anything to do with mkv?

So mkv has a specific definition of the vendor metadata?
I did not immediately find it, could you point me to it?

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