Re: [FFmpeg-devel] [PATCH v5] avdevice/xcbgrab: check return values of xcb query functions

[Alexander Strasser]

Hi Andriy!

On 2020-07-19 19:47 -0400, Andriy Gelman wrote:
> On Sun, 19. Jul 23:19, Alexander Strasser wrote:
> > On 2020-07-16 23:54 -0400, Andriy Gelman wrote:
> > > On Tue, 14. Jul 14:14, Moritz Barsnick wrote:
[...]
> > > > --- a/libavdevice/xcbgrab.c
> > > > +++ b/libavdevice/xcbgrab.c
> > > > @@ -346,8 +346,10 @@ static void xcbgrab_draw_mouse(AVFormatContext *s, 
> > > > AVPacket *pkt,
> > > >  return;
> > > >
> > >
> > > >  cursor = xcb_xfixes_get_cursor_image_cursor_image(ci);
> > > > -if (!cursor)
> > > > +if (!cursor) {
> > > > +free(ci);
> > > >  return;
> > > > +}
> > >
> > > This check seems dead code. Looking at xcb sources, cursor is just an 
> > > offset in
> > > memory from ci so I don't think it can be null here.
> >
> > It's great to look at the sources, but I don't think we should turn
> > an implementation snapshot into a guarantee.
> >
> > I guess it's safer to keep the check, if there is no documentation
> > about this being always non-NULL.
> >
> > I'm not entirely sure how well this is documented. Surely some of
> > functions definitely return NULL sometimes, which was the reason to
> > submit this patch, I would probably only assume non-NULL returns for
> > functions where that is explicitly documented.
>
> xcb_xfixes_get_cursor_image_cursor_image(ci) is just an accessor function to 
> the
> reply:
>
> uint32_t *
> xcb_xfixes_get_cursor_image_cursor_image (const 
> xcb_xfixes_get_cursor_image_reply_t *R)
> {
> return (uint32_t *) (R + 1);
> }
>
> All the error handling should be done after the call to:
> ci = xcb_xfixes_get_cursor_image_reply(gr->conn, cc, NULL);
>
> We check whether ci == NULL, but you can also pass xcb_generic_error_t** and
> check the result.

My argument is, that ci could well have been allocated, but maybe
it doesn't contain cursor data and then

xcb_xfixes_get_cursor_image_cursor_image

checks for that and does return NULL instead of an offset. Or
xcb_xfixes_get_cursor_image_cursor_image is changed to allocate
an internally managed copy of the cursor at access time, which
could fail.

It's purely hypothetical, but it could happen. Being able to do
things differently at some point in the future is often part of
the argumentation for having accessor functions in the first
place, but it only works if the callers don't assume details
about the implementation.


> But anyway, this part of the patch doesn't really have anything to do with
> ticket #7312, and should be in a separate patch.

Yes, it's definitely something that was changed in this patch
at all. So it's better not to touch it in this patch.

If you really like to change it in the future, that is acceptable
for me though I don't really see the point. Maybe there is even
stronger guarantees around, that I fail to see documented in the
API docs I read so far.


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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH] avformat/matroskadec: Avoid undefined pointer arithmetic

[Andreas Rheinhardt]

The Matroska demuxer currently always opens a GetByteContext to read the
content of the projection's private data buffer; it does this even if
there is no private data buffer in which case opening the GetByteContext
will lead to a NULL + 0 which is undefined behaviour.
Furthermore, in this case the code relied both on the implicit checks
of the bytestream2 API as well as on the fact that it returns zero
if there is not enough data available.

Both of these issues have been addressed by not using the bytestream API
any more; instead the data is simply read directly by using AV_RB. This
is possible because the offsets are constants.

Signed-off-by: Andreas Rheinhardt 
---
 libavformat/matroskadec.c | 22 +-
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index cff7f0cb54..6abb5412de 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -2162,30 +2162,26 @@ static int mkv_parse_video_projection(AVStream *st, 
const MatroskaTrack *track,
   void *logctx)
 {
 AVSphericalMapping *spherical;
+const MatroskaTrackVideoProjection *mkv_projection = 
&track->video.projection;
+const uint8_t *priv_data = mkv_projection->private.data;
 enum AVSphericalProjection projection;
 size_t spherical_size;
 uint32_t l = 0, t = 0, r = 0, b = 0;
 uint32_t padding = 0;
 int ret;
-GetByteContext gb;
 
-bytestream2_init(&gb, track->video.projection.private.data,
- track->video.projection.private.size);
-
-if (bytestream2_get_byte(&gb) != 0) {
+if (mkv_projection->private.size && priv_data[0] != 0) {
 av_log(logctx, AV_LOG_WARNING, "Unknown spherical metadata\n");
 return 0;
 }
 
-bytestream2_skip(&gb, 3); // flags
-
 switch (track->video.projection.type) {
 case MATROSKA_VIDEO_PROJECTION_TYPE_EQUIRECTANGULAR:
 if (track->video.projection.private.size == 20) {
-t = bytestream2_get_be32(&gb);
-b = bytestream2_get_be32(&gb);
-l = bytestream2_get_be32(&gb);
-r = bytestream2_get_be32(&gb);
+t = AV_RB32(priv_data +  4);
+b = AV_RB32(priv_data +  8);
+l = AV_RB32(priv_data + 12);
+r = AV_RB32(priv_data + 16);
 
 if (b >= UINT_MAX - t || r >= UINT_MAX - l) {
 av_log(logctx, AV_LOG_ERROR,
@@ -2209,14 +2205,14 @@ static int mkv_parse_video_projection(AVStream *st, 
const MatroskaTrack *track,
 av_log(logctx, AV_LOG_ERROR, "Missing projection private 
properties\n");
 return AVERROR_INVALIDDATA;
 } else if (track->video.projection.private.size == 12) {
-uint32_t layout = bytestream2_get_be32(&gb);
+uint32_t layout = AV_RB32(priv_data + 4);
 if (layout) {
 av_log(logctx, AV_LOG_WARNING,
"Unknown spherical cubemap layout %"PRIu32"\n", layout);
 return 0;
 }
 projection = AV_SPHERICAL_CUBEMAP;
-padding = bytestream2_get_be32(&gb);
+padding = AV_RB32(priv_data + 8);
 } else {
 av_log(logctx, AV_LOG_ERROR, "Unknown spherical metadata\n");
 return AVERROR_INVALIDDATA;
-- 
2.20.1

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] Request for Technical committee action

[Paul B Mahol]

Look at thread:

avfilter: add ff_inlink_peek_samples and ff_inlink_skip samples

Another developer is constantly blocking my patches.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/4] avformat/au: Store strings instead of pointers to strings in array

[Alexander Strasser]

On 2020-07-19 23:38 +0200, Andreas Rheinhardt wrote:
> Alexander Strasser:
[...]
> >
> > On 2020-07-17 13:14 +0200, Andreas Rheinhardt wrote:
> >> Andreas Rheinhardt:
> >>> Signed-off-by: Andreas Rheinhardt 
> >>> ---
> >>>  libavformat/au.c | 13 ++---
> >>>  1 file changed, 6 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/libavformat/au.c b/libavformat/au.c
> >>> index f92863e400..b419c9ed95 100644
> >>> --- a/libavformat/au.c
> >>> +++ b/libavformat/au.c
> >>> @@ -68,13 +68,13 @@ static int au_probe(const AVProbeData *p)
> >>>
> >>>  static int au_read_annotation(AVFormatContext *s, int size)
> >>>  {
> >>> -static const char * keys[] = {
> >>> +static const char keys[][7] = {
> >>>  "title",
> >>>  "artist",
> >>>  "album",
> >>>  "track",
> >>>  "genre",
> >>> -NULL };
> >>> +};
> >
> > Sorry, I couldn't test myself but wouldn't just
> >
> > static const char * keys[] = {
> > "title",
> > "artist",
> > "album",
> > "track",
> > "genre",
> > };
> >
> > have been the better choice with the same benefits?
> >
> > I'm not sure without looking up the C standard and writing some
> > little test programs. From my guts I would say it's equivalent,
> > but the syntax is more convenient this way.
> >
> > That's also another issue with the commit message, since I don't
> > think it is true that in your version strings are stored in the
> > array. No offense intended and I sure might be mistaken.
> >
> But it is true.

Yes, you're right. I read the array dimension the wrong way around :(

Sorry for the noise.


> The strings are stored directly in the array, so that
> each array takes up 5 * 7 bytes. Before the change, the array itself
> took up 5 * sizeof(char *). And on top of that comes the space for the
> strings itself.
>
> Storing the strings itself in the array instead of pointers to the
> strings saves the space of the pointers, but it forces the shortest
> string to the size of the longest string. Therefore it is worthwhile if
> the average size differs from the longest size by less than sizeof(char
> *); there is furthermore one exception to this rule: If one stores
> pointers, different pointers may point to the same string (or to a part
> of a larger string). In the case here, the strings itself are smaller
> than sizeof(char *) on 64bit systems, so this alone shows that one saves
> space.

So it's about 40 bytes on systems with 64 addresses, right?


> Also notice that there are two more differences:
> a) The earlier version consisted of an array of nonconst pointers to
> const strings. Making the array entries const has been forgotten (it
> would go like this: "static const char *const keys[]"). Given that
> arrays are automatically nonmodifiable, this problem doesn't exist.
> (Given that these arrays are static and given that the compiler can see
> that they are not modified means that the compiler could infer that they
> are const and put them in the appropriate section for const data.)
> b) The actual access to the string is different: If the array contains
> pointer to strings, then one has to read the array entry (containing the
> pointer to the string) and pass it to av_strcasecmp() etc. If the array
> contains the strings, then one just has to get the address of the array
> entry (which coincides with the address of the string itself) and pass
> it to av_strcasecmp() etc..

I agree the new version is technically superior.

It's also more rigid, but that should probably not matter much in
this specific case.

Still I think the savings are marginal, so I guess you did them
while looking into the code for fixing the things later in this
patch set.

Fair enough.

A small additional sentence in the commit message body, would have
been better IMHO:

   Save a bit of space and one layer of indirection.



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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] dnn/native: add native support for avg_pool

[Fu, Ting]

> -Original Message-
> From: ffmpeg-devel  On Behalf Of Guo,
> Yejun
> Sent: Monday, July 20, 2020 01:46 PM
> To: FFmpeg development discussions and patches 
> Subject: Re: [FFmpeg-devel] [PATCH 1/2] dnn/native: add native support for
> avg_pool
> 
> 
> 
> > -Original Message-
> > From: ffmpeg-devel  On Behalf Of Ting
> > Fu
> > Sent: 2020年7月17日 23:23
> > To: ffmpeg-devel@ffmpeg.org
> > Subject: [FFmpeg-devel] [PATCH 1/2] dnn/native: add native support for
> > avg_pool
> >
> > It can be tested with the model generated with below python script:
> >
> > import tensorflow as tf
> > import numpy as np
> > import imageio
> >
> > in_img = imageio.imread('input_odd.jpg') in_img =
> > in_img.astype(np.float32)/255.0 in_data = in_img[np.newaxis, :]
> >
> > x = tf.placeholder(tf.float32, shape=[1, None, None, 3],
> > name='dnn_in') x_pool = tf.nn.avg_pool(x, ksize=[1,2,2,1],
> > strides=[1,2,2,1], padding='SAME') #please alter the params as needed
> > y = tf.identity(x_pool, name='dnn_out')
> >
> > sess=tf.Session()
> > sess.run(tf.global_variables_initializer())
> >
> > graph_def = tf.graph_util.convert_variables_to_constants(sess,
> > sess.graph_def,
> > ['dnn_out'])
> > tf.train.write_graph(graph_def, '.', 'image_process.pb',
> > as_text=False)
> >
> > print("image_process.pb generated, please use \
> > path_to_ffmpeg/tools/python/convert.py to generate
> > image_process.model\n")
> >
> > output = sess.run(y, feed_dict={x: in_data}) imageio.imsave("out.jpg",
> > np.squeeze(output))
> >
> > Signed-off-by: Ting Fu 
> > ---
> >  libavfilter/dnn/Makefile  |   1 +
> >  libavfilter/dnn/dnn_backend_native.h  |   2 +
> >  .../dnn/dnn_backend_native_layer_avgpool.c| 136 ++
> >  .../dnn/dnn_backend_native_layer_avgpool.h|  35 +
> >  .../dnn/dnn_backend_native_layer_conv2d.h |   3 +-
> >  libavfilter/dnn/dnn_backend_native_layers.c   |   2 +
> >  tools/python/convert_from_tensorflow.py   |  31 +++-
> >  7 files changed, 207 insertions(+), 3 deletions(-)  create mode
> > 100644 libavfilter/dnn/dnn_backend_native_layer_avgpool.c
> >  create mode 100644 libavfilter/dnn/dnn_backend_native_layer_avgpool.h
> >
[...]
> > +int32_t input_operand_index = input_operand_indexes[0];
> > +int number = operands[input_operand_index].dims[0];
> > +int height = operands[input_operand_index].dims[1];
> > +int width = operands[input_operand_index].dims[2];
> > +int channel = operands[input_operand_index].dims[3];
> 
> the input channel should come from here, not in AvgPoolParams.
> And so as output channel.

HI Yejun,

I got it that the in_channel should come from here. Does the 'so as output 
channel' mean out_channel = in_channel here (since the pooling of channel is 
not supported)?

> 
> > +const float *input = operands[input_operand_index].data;
> > +const AvgPoolParams *avgpool_params = (const AvgPoolParams
> > *)parameters;
> > +
> > +float kernel_strides = avgpool_params->strides;
> 
> why float?

In order to calculate height/kernel_strides with float output in following 
ceil(). Or should I multiply kernel_strides with 1.0  when using ceil function?

> 
> > +int src_linesize = width * avgpool_params->in_channels;
> > +DnnOperand *output_operand = &operands[output_operand_index];
> > +
> > +if (avgpool_params->padding_method == SAME) {
> > +height_end = height;
> > +width_end = width;
> > +height_radius = (avgpool_params->kernel_size - ((height - 1)
> > + % (int)
> > kernel_strides + 1));
> 
> don't need the first '(' and last ')'.

OK

> 
> why we need to consider kernel_strides here?

Because when padding_method=SAME, the tensorflow will only padding the half 
number of 0 pixels except the remainders.
Eg: if the width is 1080, strides=11, so the 1080%11=2
And if ksize=5, it will fill (5-2)>>1=1 column before image and 
2 columns after the image.
And if ksize=2, so 2-2=0, so the remainder pixels just meet the 
need of calculating one time pooling, so no 0 pixels will be filled.
Which means the numbers of filling 0-pixels rely on the remainder-pixels.
Does the example make any sense?

> 
> > +width_radius = (avgpool_params->kernel_size - ((width - 1) %
> > + (int)
> > kernel_strides + 1));
> 
> same as above.
> 
> > +height_radius = height_radius < 0 ? 0 : height_radius >> 1;
> > +width_radius = width_radius < 0 ? 0 : width_radius >> 1;
[...]
> > +for (int y = 0; y < height_end; y += kernel_strides) {
> > +for (int x = 0; x < width_end; x += kernel_strides) {
> > +for (int n_filter = 0; n_filter <
> > + avgpool_params->out_channels;
> > ++n_filter) {
> []
> better to use n_channel, instead of n_filter.

Sure

> 
> > +output[n_filter] = 0.0;
> > +kernel_area = 0;
[...]
> > +def dump_avg_pool_to_file(self, node, f):
> > +assert(node.op == 'AvgPool')
> > +self.l

[FFmpeg-devel] [PATCH] avcodec/apm: fix sample_rate check

[Zane van Iperen]

Signed-off-by: Zane van Iperen 
---
 libavformat/apm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavformat/apm.c b/libavformat/apm.c
index 0d88e1099a..fe95a9ad7a 100644
--- a/libavformat/apm.c
+++ b/libavformat/apm.c
@@ -126,8 +126,8 @@ static int apm_read_header(AVFormatContext *s)
 if (avio_rl32(s->pb) != APM_FILE_EXTRADATA_SIZE)
 return AVERROR_INVALIDDATA;
 
-/* I've never seen files greater than this. */
-if (par->sample_rate > 44100)
+/* 8 = bits per sample * max channels */
+if (par->sample_rate > (INT_MAX / 8))
 return AVERROR_INVALIDDATA;
 
 if (par->bits_per_coded_sample != 4)
-- 
2.25.1


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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 3/3] lavf/dashdec: enable custom interrup callback in sub-demuxer

[myp...@gmail.com]

On Mon, Jul 20, 2020 at 10:18 AM Steven Liu  wrote:
>
> Jun Zhao  于2020年7月18日周六 下午8:19写道：
> >
> > From: Jun Zhao 
> >
> > Enable the custom callback in sub-demuxer
> >
> > Signed-off-by: Jun Zhao 
> > ---
> >  libavformat/dashdec.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/libavformat/dashdec.c b/libavformat/dashdec.c
> > index 694782c..c5a5ff6 100644
> > --- a/libavformat/dashdec.c
> > +++ b/libavformat/dashdec.c
> > @@ -1943,6 +1943,7 @@ static int reopen_demux_for_component(AVFormatContext 
> > *s, struct representation
> >  pls->ctx->flags = AVFMT_FLAG_CUSTOM_IO;
> >  pls->ctx->probesize = s->probesize > 0 ? s->probesize : 1024 * 4;
> >  pls->ctx->max_analyze_duration = s->max_analyze_duration > 0 ? 
> > s->max_analyze_duration : 4 * AV_TIME_BASE;
> > +pls->ctx->interrupt_callback = s->interrupt_callback;
> >  ret = av_probe_input_buffer(&pls->pb, &in_fmt, "", NULL, 0, 0);
> >  if (ret < 0) {
> >  av_log(s, AV_LOG_ERROR, "Error when loading first fragment, 
> > playlist %d\n", (int)pls->rep_idx);
> > --
> > 2.7.4
> >
> > ___
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
>
> LGTM
>
Pushed, thx
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/3] lavf/hls: enable custom interrup callback in sub-demuxer

[myp...@gmail.com]

On Mon, Jul 20, 2020 at 10:18 AM Steven Liu  wrote:
>
> Jun Zhao  于2020年7月18日周六 下午7:56写道：
> >
> > From: Jun Zhao 
> >
> > Enable the custom callback in sub-demuxer
> >
> > Signed-off-by: Jun Zhao 
> > ---
> >  libavformat/hls.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/libavformat/hls.c b/libavformat/hls.c
> > index ba17c4e..cf0b71d 100644
> > --- a/libavformat/hls.c
> > +++ b/libavformat/hls.c
> > @@ -1984,6 +1984,7 @@ static int hls_read_header(AVFormatContext *s)
> >read_data, NULL, NULL);
> >  pls->ctx->probesize = s->probesize > 0 ? s->probesize : 1024 * 4;
> >  pls->ctx->max_analyze_duration = s->max_analyze_duration > 0 ? 
> > s->max_analyze_duration : 4 * AV_TIME_BASE;
> > +pls->ctx->interrupt_callback = s->interrupt_callback;
> >  url = av_strdup(pls->segments[0]->url);
> >  ret = av_probe_input_buffer(&pls->pb, &in_fmt, url, NULL, 0, 0);
> >  av_free(url);
> > --
> > 2.7.4
> >
> > ___
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> > To unsubscribe, visit link above, or email
> > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
>
>
> LGTM
>
Pushed, thx
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avdevice/decklink_dec: mark the field flag if framerate > 30FPS

[lance . lmwang]

On Sun, Jul 19, 2020 at 09:05:33AM +0200, Marton Balint wrote:
> 
> 
> On Sun, 19 Jul 2020, lance.lmw...@gmail.com wrote:
> 
> > On Sat, Jul 18, 2020 at 08:38:16PM +0200, Marton Balint wrote:
> > > 
> > > 
> > > On Sat, 18 Jul 2020, lance.lmw...@gmail.com wrote:
> > > 
> > > > On Sat, Jul 18, 2020 at 09:44:03AM +0200, Marton Balint wrote:
> > > > > > > > > On Sat, 18 Jul 2020, lance.lmw...@gmail.com wrote:
> > > > > > > > On Fri, Jul 17, 2020 at 07:13:28PM +0200, Marton Balint
> > > wrote:
> > > > > > > > > > > On Fri, 17 Jul 2020, lance.lmw...@gmail.com wrote:
> > > > > > > > > > From: Limin Wang 
> > > > > > > > > In SMPTE ST 12-1: 2014 Sec 12.2, we need to mark the frame 
> > > > > > > > > flag
> > > > > > > if the frame rate > 30FPS to
> > > > > > > > avoid interoperability issues. It will be used by the encoder 
> > > > > > > > to identify even or odd frames
> > > > > > > > and correctly calculate the frame number of the SEI TC.
> > > > > > > > > This feature looks like it belongs to
> > > > > av_timecode_get_smpte_from_framenum
> > > > > > > and not into decklink. Also checking previous timecode and 
> > > > > > > guessing
> > > > > > > something is a hack, especially since you have the exact source 
> > > > > > > timecode.
> > > > > > If I understand correctly, it's not hacky, for the framerate > 30, 
> > > > > > the source timecode will be
> > > > > > frame pair(it's limited by 2bits limit of frame number ten digital. 
> > > > > > Below is one exmaple for 50fps
> > > > > > if you tested with SDI with TC.
> > > > > > 00:00:00:24
> > > > > > 00:00:00:24
> > > > > > 00:00:00:25
> > > > > > 00:00:00:25
> > > > > > 00:00:00:00
> > > > > > 00:00:00:00
> > > > > > > Have you actually tested this with a real SDI signal
> > > containing timecode?
> > > > > Yes, have tested with SDI playback by decklink UltraStudio 4K
> > > mini with TC enabled.
> > > 
> > > I still don't understand how this works. Isn't timecode in the SDI signal
> > > also contain the so called "field bit" flag to support 50fps timecode? And
> > > if it does, then why can't you query it from decklink? Timecode in SDI
> > > signal is also supposed to be an SMPTE 12M timecode, so all the bits you
> > > need should be in it, you should not need to guess anything. Therefore it
> > > makes little sense to me that the DeckLink API does not provide a proper
> > > timecode as string, unless the original capture you made with your
> > > UltraStudio and which you played back also has a faulty timecode.
> > > What generated the signal which you captured with UltraStudio? Not ffmpeg 
> > > I
> > > hope...
> > 
> > It's auto-generated by configure of UltraStudio.
> 
> You mean using XLR timecode input selection? I would not trust that with
> 50fps...

I'm not sure it's XLR or not, it's configured by the menu.

> 
> > I have tested with bmdTimecodeFieldMark
> > flag, it's not set as expected for testing. I have no clue how to get
> > the flag as the API provide the TC string is same for the frame pair.
> > Maybe I miss something.
> 
> I'd try to generate SDI video with timecode by some other means. E.g. using
> MediaExpress / DaVinci Resolve and a native 1080p50 file.

What's your tested result for the 1080p50 file?  I haven't MediaExpress / 
DaVinci Resolve
to generate the native 1080p50 file to try.


> 
> Regards,
> Marton
> 
> 
> > 
> > > 
> > > Regards,
> > > Marton
> > > 
> > > 
> > > > > > > > Thanks,
> > > > > Marton
> > > > > > > > > That's why I check the last TC to get the frame is even
> > > or odd.
> > > > > > > Why not use av_timecode_get_smpte_from_framenum(), for it's
> > > > > calculated by the string TC by one time,
> > > > > > so it lacks the information whether it's odd or even frame.
> > > > > > > > > > Regards,
> > > > > > > Marton
> > > > > > > > > > > ---
> > > > > > > > libavdevice/decklink_common.h |  1 +
> > > > > > > > libavdevice/decklink_dec.cpp  | 14 ++
> > > > > > > > 2 files changed, 15 insertions(+)
> > > > > > > > > diff --git a/libavdevice/decklink_common.h
> > > > > > > b/libavdevice/decklink_common.h
> > > > > > > > index bd68c7b..8ddc411 100644
> > > > > > > > --- a/libavdevice/decklink_common.h
> > > > > > > > +++ b/libavdevice/decklink_common.h
> > > > > > > > @@ -151,6 +151,7 @@ struct decklink_ctx {
> > > > > > > > int channels;
> > > > > > > > int audio_depth;
> > > > > > > > unsigned long tc_seen;// used with option wait_for_tc
> > > > > > > > +uint32_t last_tc;
> > > > > > > > };
> > > > > > > > > typedef enum { DIRECTION_IN, DIRECTION_OUT}
> > > > > > > decklink_direction_t;
> > > > > > > > diff --git a/libavdevice/decklink_dec.cpp 
> > > > > > > > b/libavdevice/decklink_dec.cpp
> > > > > > > > index dde68ff..a60c01b 100644
> > > > > > > > --- a/libavdevice/decklink_dec.cpp
> > > > > > > > +++ b/libavdevice/decklink_dec.cpp
> > > > > > > > @@ -884,12 +884,26 @@ HRESULT 
> > > > > > > > decklink_input_callback::VideoInputFrameArrived(
> > > > > > > > in

[FFmpeg-devel] [PATCH] lavc/vaapi_encode: Add render target support for 422 10-bit

[Linjie Fu]

This enables VAAPI encoding support for 422 10-bit(Y210).

Signed-off-by: Linjie Fu 
---
 libavcodec/vaapi_encode.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
index 1de43cc..6766641 100644
--- a/libavcodec/vaapi_encode.c
+++ b/libavcodec/vaapi_encode.c
@@ -1248,6 +1248,9 @@ static const VAAPIEncodeRTFormat 
vaapi_encode_rt_formats[] = {
 { "YUV400",VA_RT_FORMAT_YUV400,8, 1,  },
 { "YUV420",VA_RT_FORMAT_YUV420,8, 3, 1, 1 },
 { "YUV422",VA_RT_FORMAT_YUV422,8, 3, 1, 0 },
+#if VA_CHECK_VERSION(1, 2, 0)
+{ "YUV422_10", VA_RT_FORMAT_YUV422_10,10, 3, 1, 0 },
+#endif
 { "YUV444",VA_RT_FORMAT_YUV444,8, 3, 0, 0 },
 { "YUV411",VA_RT_FORMAT_YUV411,8, 3, 2, 0 },
 #if VA_CHECK_VERSION(0, 38, 1)
-- 
2.7.4

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/4] vaapi_encode_h265: Enable 4:2:2 support

[Linjie Fu]

On Fri, May 15, 2020 at 3:21 PM Fu, Linjie  wrote:
>
> > From: ffmpeg-devel  On Behalf Of
> > Mark Thompson
> > Sent: Sunday, March 8, 2020 00:15
> > To: ffmpeg-devel@ffmpeg.org
> > Subject: Re: [FFmpeg-devel] [PATCH 4/4] vaapi_encode_h265: Enable 4:2:2
> > support
> >
> > On 05/03/2020 02:49, Fu, Linjie wrote:
> > > 2. recon surface of Y210 or 444 (AYUV and Y410 in media-driver) depends
> > on the surface hint [3] in
> > > libva and corresponding code in media-driver to resize the recon surface
> > which is not upstreamed
> > > yet.
> >
> > What is the reasoning for forcing the user to pass new extra attributes with
> > this rather than handling it transparently so that it works like all other
> > encoders?
> >
> > In some places in Mesa surfaces are reallocated with different properties
> > when they are used for a purpose they don't currently support, which avoids
> > weird constraints being exported to the user (e.g. see
> >  > a/surface.c#n1000>).  Since reconstructed picture surfaces are pretty 
> > unlikely
> > to be used for anything else (just being copied out for debugging maybe?), 
> > it
> > seems like an answer like that would be simpler in this case too.  (Though
> > perhaps I'm missing something weird about the Intel hardware which makes
> > this case uglier.)
> >
>
> Implemented the surface reallocation inside media driver in [1], merged the 
> query
> support in [2],  verified that it works for both AYUV(or XYUV)/Y410, yuyv422.
>
> And for Y210, it seems to be better to implement render target support in
> vaapi_encoder in this patch as well:
> { "YUV422_10", VA_RT_FORMAT_YUV422_10,10, 3, 1, 0 },
>
> Hence patch LGTM with or without above modifications, thx.
>
> [1] < https://github.com/intel/media-driver/pull/915>
> [2] < https://github.com/intel/media-driver/pull/855>

Since it's well supported for now, prefer to apply this soon
together with the patch for 422 10-bit encoding :

https://patchwork.ffmpeg.org/project/ffmpeg/patch/1595254554-12809-1-git-send-email-linjie...@intel.com/
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avcodec/apm: fix sample_rate check

[Zane van Iperen]

On Mon, 20 Jul 2020 10:46:29 +
"Zane van Iperen"  wrote:

> 
> Signed-off-by: Zane van Iperen 
> ---
>  libavformat/apm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
This is a trivial fix, will apply tomorrow if no objections.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avdevice/decklink_dec: mark the field flag if framerate > 30FPS

[Marton Balint]

On Mon, 20 Jul 2020, lance.lmw...@gmail.com wrote:


On Sun, Jul 19, 2020 at 09:05:33AM +0200, Marton Balint wrote:



On Sun, 19 Jul 2020, lance.lmw...@gmail.com wrote:

> On Sat, Jul 18, 2020 at 08:38:16PM +0200, Marton Balint wrote:
> > 
> > 
> > On Sat, 18 Jul 2020, lance.lmw...@gmail.com wrote:
> > 
> > > On Sat, Jul 18, 2020 at 09:44:03AM +0200, Marton Balint wrote:

> > > > > > > > On Sat, 18 Jul 2020, lance.lmw...@gmail.com wrote:
> > > > > > > On Fri, Jul 17, 2020 at 07:13:28PM +0200, Marton Balint
> > wrote:
> > > > > > > > > > On Fri, 17 Jul 2020, lance.lmw...@gmail.com wrote:
> > > > > > > > > From: Limin Wang 
> > > > > > > > In SMPTE ST 12-1: 2014 Sec 12.2, we need to mark the frame flag
> > > > > > if the frame rate > 30FPS to
> > > > > > > avoid interoperability issues. It will be used by the encoder to 
identify even or odd frames
> > > > > > > and correctly calculate the frame number of the SEI TC.
> > > > > > > > This feature looks like it belongs to
> > > > av_timecode_get_smpte_from_framenum
> > > > > > and not into decklink. Also checking previous timecode and guessing
> > > > > > something is a hack, especially since you have the exact source 
timecode.
> > > > > If I understand correctly, it's not hacky, for the framerate > 30, 
the source timecode will be
> > > > > frame pair(it's limited by 2bits limit of frame number ten digital. 
Below is one exmaple for 50fps
> > > > > if you tested with SDI with TC.
> > > > > 00:00:00:24
> > > > > 00:00:00:24
> > > > > 00:00:00:25
> > > > > 00:00:00:25
> > > > > 00:00:00:00
> > > > > 00:00:00:00
> > > > > > Have you actually tested this with a real SDI signal
> > containing timecode?
> > > > Yes, have tested with SDI playback by decklink UltraStudio 4K
> > mini with TC enabled.
> > 
> > I still don't understand how this works. Isn't timecode in the SDI signal

> > also contain the so called "field bit" flag to support 50fps timecode? And
> > if it does, then why can't you query it from decklink? Timecode in SDI
> > signal is also supposed to be an SMPTE 12M timecode, so all the bits you
> > need should be in it, you should not need to guess anything. Therefore it
> > makes little sense to me that the DeckLink API does not provide a proper
> > timecode as string, unless the original capture you made with your
> > UltraStudio and which you played back also has a faulty timecode.
> > What generated the signal which you captured with UltraStudio? Not ffmpeg I
> > hope...
> 
> It's auto-generated by configure of UltraStudio.


You mean using XLR timecode input selection? I would not trust that with
50fps...


I'm not sure it's XLR or not, it's configured by the menu.



> I have tested with bmdTimecodeFieldMark
> flag, it's not set as expected for testing. I have no clue how to get
> the flag as the API provide the TC string is same for the frame pair.
> Maybe I miss something.

I'd try to generate SDI video with timecode by some other means. E.g. using
MediaExpress / DaVinci Resolve and a native 1080p50 file.


What's your tested result for the 1080p50 file?  I haven't MediaExpress / 
DaVinci Resolve
to generate the native 1080p50 file to try.


I have not tested anything, all I am saying is that I find that highly 
unlikely that decklink API cannot detect a 50fps timecode properly in the 
SDI signal, so I suspect that your tests are somehow flawed, and you 
should try testing by some other means.


I can do tests next week if you don't figure it out until then.

Regards,
Marton






Regards,
Marton


> 
> > 
> > Regards,

> > Marton
> > 
> > 
> > > > > > > Thanks,

> > > > Marton
> > > > > > > > That's why I check the last TC to get the frame is even
> > or odd.
> > > > > > Why not use av_timecode_get_smpte_from_framenum(), for it's
> > > > calculated by the string TC by one time,
> > > > > so it lacks the information whether it's odd or even frame.
> > > > > > > > > Regards,
> > > > > > Marton
> > > > > > > > > > ---
> > > > > > > libavdevice/decklink_common.h |  1 +
> > > > > > > libavdevice/decklink_dec.cpp  | 14 ++
> > > > > > > 2 files changed, 15 insertions(+)
> > > > > > > > diff --git a/libavdevice/decklink_common.h
> > > > > > b/libavdevice/decklink_common.h
> > > > > > > index bd68c7b..8ddc411 100644
> > > > > > > --- a/libavdevice/decklink_common.h
> > > > > > > +++ b/libavdevice/decklink_common.h
> > > > > > > @@ -151,6 +151,7 @@ struct decklink_ctx {
> > > > > > > int channels;
> > > > > > > int audio_depth;
> > > > > > > unsigned long tc_seen;// used with option wait_for_tc
> > > > > > > +uint32_t last_tc;
> > > > > > > };
> > > > > > > > typedef enum { DIRECTION_IN, DIRECTION_OUT}
> > > > > > decklink_direction_t;
> > > > > > > diff --git a/libavdevice/decklink_dec.cpp 
b/libavdevice/decklink_dec.cpp
> > > > > > > index dde68ff..a60c01b 100644
> > > > > > > --- a/libavdevice/decklink_dec.cpp
> > > > > > > +++ b/libavdevice/decklink_dec.cpp
> > > > > > > @@ -884,12 +884,26 @@ HRESULT 
decklink_inp

Re: [FFmpeg-devel] [PATCH 1/4] avformat/au: Store strings instead of pointers to strings in array

[Andreas Rheinhardt]

Alexander Strasser:
> On 2020-07-19 23:38 +0200, Andreas Rheinhardt wrote:
>> Alexander Strasser:
> [...]
>>>
>>> On 2020-07-17 13:14 +0200, Andreas Rheinhardt wrote:
 Andreas Rheinhardt:
> Signed-off-by: Andreas Rheinhardt 
> ---
>  libavformat/au.c | 13 ++---
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/libavformat/au.c b/libavformat/au.c
> index f92863e400..b419c9ed95 100644
> --- a/libavformat/au.c
> +++ b/libavformat/au.c
> @@ -68,13 +68,13 @@ static int au_probe(const AVProbeData *p)
>
>  static int au_read_annotation(AVFormatContext *s, int size)
>  {
> -static const char * keys[] = {
> +static const char keys[][7] = {
>  "title",
>  "artist",
>  "album",
>  "track",
>  "genre",
> -NULL };
> +};
>>>
>>> Sorry, I couldn't test myself but wouldn't just
>>>
>>> static const char * keys[] = {
>>> "title",
>>> "artist",
>>> "album",
>>> "track",
>>> "genre",
>>> };
>>>
>>> have been the better choice with the same benefits?
>>>
>>> I'm not sure without looking up the C standard and writing some
>>> little test programs. From my guts I would say it's equivalent,
>>> but the syntax is more convenient this way.
>>>
>>> That's also another issue with the commit message, since I don't
>>> think it is true that in your version strings are stored in the
>>> array. No offense intended and I sure might be mistaken.
>>>
>> But it is true.
> 
> Yes, you're right. I read the array dimension the wrong way around :(
> 
> Sorry for the noise.
> 
> 
>> The strings are stored directly in the array, so that
>> each array takes up 5 * 7 bytes. Before the change, the array itself
>> took up 5 * sizeof(char *). And on top of that comes the space for the
>> strings itself.
>>
>> Storing the strings itself in the array instead of pointers to the
>> strings saves the space of the pointers, but it forces the shortest
>> string to the size of the longest string. Therefore it is worthwhile if
>> the average size differs from the longest size by less than sizeof(char
>> *); there is furthermore one exception to this rule: If one stores
>> pointers, different pointers may point to the same string (or to a part
>> of a larger string). In the case here, the strings itself are smaller
>> than sizeof(char *) on 64bit systems, so this alone shows that one saves
>> space.
> 
> So it's about 40 bytes on systems with 64 addresses, right?
> 
6 pointers amount to 48 bytes, but one also has to waste 4 bytes to make
the shorter strings as long as the longest.
> 
>> Also notice that there are two more differences:
>> a) The earlier version consisted of an array of nonconst pointers to
>> const strings. Making the array entries const has been forgotten (it
>> would go like this: "static const char *const keys[]"). Given that
>> arrays are automatically nonmodifiable, this problem doesn't exist.
>> (Given that these arrays are static and given that the compiler can see
>> that they are not modified means that the compiler could infer that they
>> are const and put them in the appropriate section for const data.)
>> b) The actual access to the string is different: If the array contains
>> pointer to strings, then one has to read the array entry (containing the
>> pointer to the string) and pass it to av_strcasecmp() etc. If the array
>> contains the strings, then one just has to get the address of the array
>> entry (which coincides with the address of the string itself) and pass
>> it to av_strcasecmp() etc..
> 
> I agree the new version is technically superior.
> 
> It's also more rigid, but that should probably not matter much in
> this specific case.
> 
Indeed, it is usually not suitable when one has too many strings.

> Still I think the savings are marginal, so I guess you did them
> while looking into the code for fixing the things later in this
> patch set.
> 
Yes.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] Ticket 5012

[Gautam Ramakrishnan]

On Mon, Jul 20, 2020 at 12:28 AM Carl Eugen Hoyos  wrote:
>
> Am So., 19. Juli 2020 um 20:18 Uhr schrieb Gautam Ramakrishnan
> :
> >
> > On Sun, Jul 19, 2020 at 11:33 PM Thomas Volkert  wrote:
> > >
> > > Hi,
> > >
> > > On 19.07.2020 18:10, Gautam Ramakrishnan wrote:
> > > > On Thu, Jul 16, 2020 at 11:45 PM Carl Eugen Hoyos  
> > > > wrote:
> > > >> Am Do., 16. Juli 2020 um 19:42 Uhr schrieb Gautam Ramakrishnan
> > > >> :
> > > >>
> > > >>> How could I get access to some stream/file for testing this feature if
> > > >>> I implement this?
> > > >> GStreamer should allow you to test your implementation.
> > > >>
> > > >> Carl Eugen
> > > > So to clarify things, basically the enc version would take a compressed
> > > > codestream, and split it into RTP packets given in the RFC? Is there 
> > > > anything
> > > > more than that? Is there any documentation page I can refer to? I was 
> > > > not able
> > > > to find anything specific.
> > >
> > >
> > > Do you mean the RFC itself?
> > > -> https://tools.ietf.org/html/rfc5371
> > >
> > Yes
> > >
> > > Alternatively to GStreamer you can use Live555 to test your
> > > implementation. I would start with the receiver and then continue with
> > > the sender part.
> > > Live555 source code for handling this payload:
> > >
> > > ->
> > > https://github.com/ThomasVolkert/live555/blob/master/liveMedia/JPEG2000VideoRTPSink.cpp
> > >
> > > ->
> > > https://github.com/ThomasVolkert/live555/blob/master/liveMedia/JPEG2000VideoRTPSource.cpp
> > >
> > >
> > Thanks a lot for these, will be very helpful
> >
> > Additionally, what I understand is that the packetisation is of 3 types,
> > main header, tile part header and codestream packets.
> > However, if I understand it correctly, the rtp encoder provides an encoded
> > j2k codestream. To get the codestream packets from this, the codestream will
> > have to be decoded. Any suggestions on how to solve this? It does not make 
> > sense
> > to decode a codestream for this purpose. Can the tile part packets be
> > dumped from the encoder itself?
>
> Did you already look into the rtp muxer / demuxer for jpeg or other formats?
> I wonder if that would answer your questions.
I have got a better understanding now



-- 
-
Gautam |
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH] avformat/matroskadec: Slightly simplify version check

[Andreas Rheinhardt]

Signed-off-by: Andreas Rheinhardt 
---
Will apply this triviality in two days if there are no objections.

 libavformat/matroskadec.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 6abb5412de..b1ef344aa7 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -2027,12 +2027,12 @@ static int matroska_parse_flac(AVFormatContext *s,
 
 static int mkv_field_order(MatroskaDemuxContext *matroska, int64_t field_order)
 {
-int major, minor, micro, bttb = 0;
+int minor, micro, bttb = 0;
 
 /* workaround a bug in our Matroska muxer, introduced in version 57.36 
alongside
  * this function, and fixed in 57.52 */
-if (matroska->muxingapp && sscanf(matroska->muxingapp, "Lavf%d.%d.%d", 
&major, &minor, µ) == 3)
-bttb = (major == 57 && minor >= 36 && minor <= 51 && micro >= 100);
+if (matroska->muxingapp && sscanf(matroska->muxingapp, "Lavf57.%d.%d", 
&minor, µ) == 2)
+bttb = (minor >= 36 && minor <= 51 && micro >= 100);
 
 switch (field_order) {
 case MATROSKA_VIDEO_FIELDORDER_PROGRESSIVE:
-- 
2.20.1

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 3/6] avformat/sbgdec: Check for overflow in parse_timestamp()

[Michael Niedermayer]

On Sun, Jul 19, 2020 at 07:51:47PM +0200, Nicolas George wrote:
> Michael Niedermayer (12020-07-19):
> > Fixes: signed integer overflow: 339867072 + 9195561788997000192 
> > cannot be represented in type 'long'
> > Fixes: 
> > 23790/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-6554232198266880
> > 
> > Found-by: continuous fuzzing process 
> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer 
> > ---
> >  libavformat/sbgdec.c | 2 ++
> >  1 file changed, 2 insertions(+)
> 
> No objection.

will apply

thx

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Does the universe only have a finite lifespan? No, its going to go on
forever, its just that you wont like living in it. -- Hiranya Peiri


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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avcodec/put_bits: Fix LZW warning

[Michael Niedermayer]

On Sun, Jul 19, 2020 at 08:29:29PM +0200, Steinar H. Gunderson wrote:
> lzwenc stores a function pointer to either put_bits or put_bits_le;
> however, after the recent change, the function pointer's prototype
> would depend on BitBuf. BitBuf is defined in put_bits.h, whose
> definition depends on whether BITSTREAM_WRITER_LE is #defined or not.
> For safety, we set a boolean flag for little/big endian instead,
> which also allows the definition to be inlined.
> ---
>  libavcodec/gif.c |  4 ++--
>  libavcodec/lzw.h |  6 ++
>  libavcodec/lzwenc.c  | 28 ++--
>  libavcodec/tiffenc.c |  4 ++--
>  4 files changed, 24 insertions(+), 18 deletions(-)

will apply

thx

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

Never trust a computer, one day, it may think you are the virus. -- Compn


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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH] avcodec/libvpxenc: use pix_fmt descriptors where useful

[James Zern]

similar to:
36e51c190b avcodec/libaomenc: use pix_fmt descriptors where useful

Signed-off-by: James Zern 
---
 libavcodec/libvpxenc.c | 13 +++--
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c
index 8e0ea42375..3d5e6c12e3 100644
--- a/libavcodec/libvpxenc.c
+++ b/libavcodec/libvpxenc.c
@@ -41,6 +41,7 @@
 #include "libavutil/intreadwrite.h"
 #include "libavutil/mathematics.h"
 #include "libavutil/opt.h"
+#include "libavutil/pixdesc.h"
 
 /**
  * Portion of struct vpx_codec_cx_pkt from vpx_encoder.h.
@@ -546,7 +547,8 @@ static int set_pix_fmt(AVCodecContext *avctx, 
vpx_codec_caps_t codec_caps,
vpx_img_fmt_t *img_fmt)
 {
 VPxContext av_unused *ctx = avctx->priv_data;
-enccfg->g_bit_depth = enccfg->g_input_bit_depth = 8;
+const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(avctx->pix_fmt);
+enccfg->g_bit_depth = enccfg->g_input_bit_depth = desc->comp[0].depth;
 switch (avctx->pix_fmt) {
 case AV_PIX_FMT_YUV420P:
 case AV_PIX_FMT_YUVA420P:
@@ -570,8 +572,6 @@ static int set_pix_fmt(AVCodecContext *avctx, 
vpx_codec_caps_t codec_caps,
 case AV_PIX_FMT_YUV420P10:
 case AV_PIX_FMT_YUV420P12:
 if (codec_caps & VPX_CODEC_CAP_HIGHBITDEPTH) {
-enccfg->g_bit_depth = enccfg->g_input_bit_depth =
-avctx->pix_fmt == AV_PIX_FMT_YUV420P10 ? 10 : 12;
 enccfg->g_profile = 2;
 *img_fmt = VPX_IMG_FMT_I42016;
 *flags |= VPX_CODEC_USE_HIGHBITDEPTH;
@@ -581,8 +581,6 @@ static int set_pix_fmt(AVCodecContext *avctx, 
vpx_codec_caps_t codec_caps,
 case AV_PIX_FMT_YUV422P10:
 case AV_PIX_FMT_YUV422P12:
 if (codec_caps & VPX_CODEC_CAP_HIGHBITDEPTH) {
-enccfg->g_bit_depth = enccfg->g_input_bit_depth =
-avctx->pix_fmt == AV_PIX_FMT_YUV422P10 ? 10 : 12;
 enccfg->g_profile = 3;
 *img_fmt = VPX_IMG_FMT_I42216;
 *flags |= VPX_CODEC_USE_HIGHBITDEPTH;
@@ -592,8 +590,6 @@ static int set_pix_fmt(AVCodecContext *avctx, 
vpx_codec_caps_t codec_caps,
 case AV_PIX_FMT_YUV440P10:
 case AV_PIX_FMT_YUV440P12:
 if (codec_caps & VPX_CODEC_CAP_HIGHBITDEPTH) {
-enccfg->g_bit_depth = enccfg->g_input_bit_depth =
-avctx->pix_fmt == AV_PIX_FMT_YUV440P10 ? 10 : 12;
 enccfg->g_profile = 3;
 *img_fmt = VPX_IMG_FMT_I44016;
 *flags |= VPX_CODEC_USE_HIGHBITDEPTH;
@@ -606,9 +602,6 @@ static int set_pix_fmt(AVCodecContext *avctx, 
vpx_codec_caps_t codec_caps,
 case AV_PIX_FMT_YUV444P10:
 case AV_PIX_FMT_YUV444P12:
 if (codec_caps & VPX_CODEC_CAP_HIGHBITDEPTH) {
-enccfg->g_bit_depth = enccfg->g_input_bit_depth =
-avctx->pix_fmt == AV_PIX_FMT_YUV444P10 ||
-avctx->pix_fmt == AV_PIX_FMT_GBRP10 ? 10 : 12;
 enccfg->g_profile = 3;
 *img_fmt = VPX_IMG_FMT_I44416;
 *flags |= VPX_CODEC_USE_HIGHBITDEPTH;
-- 
2.28.0.rc0.105.gf9edc3c819-goog

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2 1/2] avformat: Redo cleanup of demuxer upon read_header() failure

[Michael Niedermayer]

On Sun, Jul 19, 2020 at 10:47:54PM +0200, Andreas Rheinhardt wrote:
> If reading the header fails, the demuxer's read_close() function (if
> existing) is not called automatically; instead several demuxers call it
> via "goto fail" in read_header().
> 
> This commit intends to change this by adding a flag to AVInputFormat
> that can be used to set on a per-AVInputFormat basis whether read_close()
> should be called generically after an error during read_header().
> 
> The flag controlling this behaviour needs to be added because it might
> be unsafe to call read_close() generally (e.g. this might lead to
> read_close() being called twice and this might e.g. lead to double-frees
> if av_free() is used instead of av_freep(); or a size field has not
> been reset after freeing the elements (see the mov demuxer for an
> example of this)). Yet the intention is to check and fix all demuxers
> and make the flag redundant in the medium run.
> 
> The flag itself is non-public (it resides in libavformat/internal.h),
> but it has been added to the ordinary (i.e. public) flags field of
> AVInputFormat, because there is no field for internal flags and adding
> one is not possible, because libavdevice also defines AVInputFormats
> and so there is the possibility that a newer libavformat is used
> together with an older libavdevice that would then lack the new field
> for internal flags. When it has become redundant, it can be removed again
> at the next major version bump.
> 
> Signed-off-by: Andreas Rheinhardt 
> ---
> This is an updated version of my initial patch [1]. I have also rebased
> the whole set of patches following it (with the exception of the w3c
> patch in the next patch they no longer fix a memleak; instead they now
> only set the flag and remove the manual calls to read_close). Should I
> resend the other patches, too?
> 
> [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-March/258830.html
> 
>  libavformat/internal.h |  6 ++
>  libavformat/utils.c| 11 +--
>  2 files changed, 15 insertions(+), 2 deletions(-)

LGTM

thx

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

Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus


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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2 2/2] avformat/wc3movie: Fix memleak upon read_header failure

[Michael Niedermayer]

On Sun, Jul 19, 2020 at 10:47:55PM +0200, Andreas Rheinhardt wrote:
> wc3_read_header() might fail after having read some data into a packet
> in which case this data would leak. Fix this by setting the
> AVFMT_HEADER_CLEANUP flag that ensures that the demuxer's read_close
> function is called (it unrefs the packet) if reading the header failed.
> 
> Fixes: memleak
> Fixes: 
> 23660/clusterfuzz-testcase-minimized-ffmpeg_DEMUXER_fuzzer-6007508031504384
> 
> Found-by: continuous fuzzing process 
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Andreas Rheinhardt 
> ---
> Michael, can you confirm that this fixes the memleak?

confirmed, memleak fixed too
LGTM

thx

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

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 


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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] swscale/yuv2rgb: consider x2rgb10le on big endian hardware

[Michael Niedermayer]

On Mon, Jul 20, 2020 at 09:34:07AM +0800, Fei Wang wrote:
> This fixed FATE fail report by filter-pixfmts* for x2rgb10le on big
> endian hardware.
> ---
>  libswscale/input.c   | 11 ++-
>  libswscale/yuv2rgb.c |  6 +-
>  2 files changed, 11 insertions(+), 6 deletions(-)

will apply

thanks

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

Democracy is the form of government in which you can choose your dictator


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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] Request for Technical committee action

[Paul B Mahol]

On 7/20/20, Paul B Mahol  wrote:
> Look at thread:
>
> avfilter: add ff_inlink_peek_samples and ff_inlink_skip samples
>
> Another developer is constantly blocking my patches.
>

If TC does not act soon I will be forced to do it myself.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/7] avfilter: add ff_inlink_peek_samples and ff_inlink_skip samples

[Michael Niedermayer]

On Thu, Jun 11, 2020 at 08:31:57PM +0200, Nicolas George wrote:
> Paul B Mahol (12020-06-11):
> > Signed-off-by: Paul B Mahol 
> > ---
> >  libavfilter/avfilter.c | 61 +-
> >  libavfilter/filters.h  | 17 
> >  2 files changed, 72 insertions(+), 6 deletions(-)
> 
> Still no. It makes a very important function much more complex, it would
> be simpler implemented in the filters directly since they are not using
> the full features of the queue.

shouldnt code which is used by multiple filters be shared and available
to all filters ?
but maybe iam missing something

And it would be nice if you and paul could come up with some solution
which you both are happy with
because i dont think arbitration by someone is going to make both of
you happy. No matter what the result, likely one at least would be unhappy

Thanks

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato


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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/7] avfilter: add ff_inlink_peek_samples and ff_inlink_skip samples

[Nicolas George]

Michael Niedermayer (12020-07-20):
> shouldnt code which is used by multiple filters be shared and available
> to all filters ?

Yes indeed. Since a fixed-size window on samples is a recurring pattern
in filters, a helper API for it would be very welcome. But it should not
make the evolution of the code for all filters more complex, and
therefore it should not be mixed with the common code.

> And it would be nice if you and paul could come up with some solution
> which you both are happy with
> because i dont think arbitration by someone is going to make both of
> you happy. No matter what the result, likely one at least would be unhappy

There is no way I will work with Paul after all the abuse I receive from
him, and I resent you a little for suggesting it.

Regards,

-- 
  Nicolas George


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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avcodec/tdsc: Fix tile checks

[Michael Niedermayer]

On Thu, Jul 16, 2020 at 09:27:27AM +0200, Michael Niedermayer wrote:
> Fixes: out of array access
> Fixes: crash.asf
> 
> Found-by: anton listov 
> Reviewed-by: anton listov 
> Signed-off-by: Michael Niedermayer 
> ---
>  libavcodec/tdsc.c | 21 ++---
>  1 file changed, 10 insertions(+), 11 deletions(-)

will apply

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

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 


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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avformat/hls: add supporting EXT-X-DISCONTINUITY tag

[Philip Langdale]

On Sat, 18 Jul 2020 20:29:01 +0300
Aleksey Skripka  wrote:

> Greetings!
> 
> unconditional AVFMT_TS_DISCONT also helps to survive when pts
> rollover in mpegts occurs. seems like this patch will break again
> reading long-lasting hls.
> 

Right.

The flag on the InputFormat says the format _might_ have
discontinuities. The EXT-X-DISCONTINUITY tag shows up in the hls m3u8
each time there actually is a discontinuity.

And this is the part where I complain about how hard the hls demuxer
makes it to propagate discontinuities correctly. It concatenates the
fragments and then passes them on to the mpegts demuxer. This means you
lose the discontinuity information and you can't easily translate it
into an mpegts marker because we don't work at the packet level. *sigh*


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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/7] avfilter: add ff_inlink_peek_samples and ff_inlink_skip samples

[Paul B Mahol]

On 7/20/20, Nicolas George  wrote:
> Michael Niedermayer (12020-07-20):
>> shouldnt code which is used by multiple filters be shared and available
>> to all filters ?
>
> Yes indeed. Since a fixed-size window on samples is a recurring pattern
> in filters, a helper API for it would be very welcome. But it should not
> make the evolution of the code for all filters more complex, and
> therefore it should not be mixed with the common code.

_Should_ is very hard word here.

>
>> And it would be nice if you and paul could come up with some solution
>> which you both are happy with
>> because i dont think arbitration by someone is going to make both of
>> you happy. No matter what the result, likely one at least would be
>> unhappy
>
> There is no way I will work with Paul after all the abuse I receive from
> him, and I resent you a little for suggesting it.

Does this means you will not push your patches that I approve on mailing list?

All abuse came from my frustration with dealing with Nicolas behavior,
specifically
his tactics and strategy when he is reviewing my patches.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 1/3] avformat/dvenc: do not set binary group and biphase polarity flags

[Marton Balint]

We are not providing a binary group pack with the flags and the biphase
polarity flag is destroying 50/60 fps timecode bits.

Signed-off-by: Marton Balint 
---
 libavformat/dvenc.c | 1 -
 tests/ref/lavf/dv   | 2 +-
 tests/ref/lavf/dv_ntsc  | 2 +-
 tests/ref/lavf/dv_pal   | 2 +-
 tests/ref/vsynth/vsynth1-dv | 2 +-
 tests/ref/vsynth/vsynth1-dv-411 | 2 +-
 tests/ref/vsynth/vsynth1-dv-50  | 2 +-
 tests/ref/vsynth/vsynth1-dv-fhd | 2 +-
 tests/ref/vsynth/vsynth1-dv-hd  | 2 +-
 tests/ref/vsynth/vsynth2-dv | 2 +-
 tests/ref/vsynth/vsynth2-dv-411 | 2 +-
 tests/ref/vsynth/vsynth2-dv-50  | 2 +-
 tests/ref/vsynth/vsynth2-dv-fhd | 2 +-
 tests/ref/vsynth/vsynth2-dv-hd  | 2 +-
 tests/ref/vsynth/vsynth3-dv-fhd | 2 +-
 tests/ref/vsynth/vsynth3-dv-hd  | 2 +-
 tests/ref/vsynth/vsynth_lena-dv | 2 +-
 tests/ref/vsynth/vsynth_lena-dv-411 | 2 +-
 tests/ref/vsynth/vsynth_lena-dv-50  | 2 +-
 tests/ref/vsynth/vsynth_lena-dv-fhd | 2 +-
 tests/ref/vsynth/vsynth_lena-dv-hd  | 2 +-
 21 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/libavformat/dvenc.c b/libavformat/dvenc.c
index b04d6044d7..915ac9527d 100644
--- a/libavformat/dvenc.c
+++ b/libavformat/dvenc.c
@@ -97,7 +97,6 @@ static int dv_write_pack(enum dv_pack_type pack_id, 
DVMuxContext *c, uint8_t* bu
 switch (pack_id) {
 case dv_timecode:
 timecode  = av_timecode_get_smpte_from_framenum(&c->tc, c->frames);
-timecode |= 1<<23 | 1<<15 | 1<<7 | 1<<6; // biphase and binary group 
flags
 AV_WB32(buf + 1, timecode);
 break;
 case dv_audio_source:  /* AAUX source pack */
diff --git a/tests/ref/lavf/dv b/tests/ref/lavf/dv
index 7ae4223a21..6f7ea319e7 100644
--- a/tests/ref/lavf/dv
+++ b/tests/ref/lavf/dv
@@ -1,3 +1,3 @@
-2fb332aab8f2ba9c33b1b2368194392a *tests/data/lavf/lavf.dv
+c79671431b71e00b6d70d50a1cb560a9 *tests/data/lavf/lavf.dv
 360 tests/data/lavf/lavf.dv
 tests/data/lavf/lavf.dv CRC=0xbdaf7f52
diff --git a/tests/ref/lavf/dv_ntsc b/tests/ref/lavf/dv_ntsc
index 410b6ec254..f4663d3e32 100644
--- a/tests/ref/lavf/dv_ntsc
+++ b/tests/ref/lavf/dv_ntsc
@@ -1,3 +1,3 @@
-5569626370c7c72d40de2c4559e32856 *tests/data/lavf/lavf.dv_ntsc
+af21369e617965e4625d34fc60af7787 *tests/data/lavf/lavf.dv_ntsc
 348 tests/data/lavf/lavf.dv_ntsc
 tests/data/lavf/lavf.dv_ntsc CRC=0xa0088163
diff --git a/tests/ref/lavf/dv_pal b/tests/ref/lavf/dv_pal
index 93bb728c46..5d7818f76f 100644
--- a/tests/ref/lavf/dv_pal
+++ b/tests/ref/lavf/dv_pal
@@ -1,3 +1,3 @@
-7830f9c6716ceb6011f865f1e521b951 *tests/data/lavf/lavf.dv_pal
+bbcfa20cb185e13352178457d9755848 *tests/data/lavf/lavf.dv_pal
 360 tests/data/lavf/lavf.dv_pal
 tests/data/lavf/lavf.dv_pal CRC=0xd428d3ee
diff --git a/tests/ref/vsynth/vsynth1-dv b/tests/ref/vsynth/vsynth1-dv
index ea357b7723..2964b1bc29 100644
--- a/tests/ref/vsynth/vsynth1-dv
+++ b/tests/ref/vsynth/vsynth1-dv
@@ -1,4 +1,4 @@
-4246668d61439617101d051d7a995108 *tests/data/fate/vsynth1-dv.dv
+c78d747be2c1151d7dc5680a86ca23c3 *tests/data/fate/vsynth1-dv.dv
 720 tests/data/fate/vsynth1-dv.dv
 d52e7a9eac459ade9561d0b89bba58e7 *tests/data/fate/vsynth1-dv.out.rawvideo
 stddev:6.90 PSNR: 31.34 MAXDIFF:   76 bytes:  7603200/  7603200
diff --git a/tests/ref/vsynth/vsynth1-dv-411 b/tests/ref/vsynth/vsynth1-dv-411
index ec88d67ece..0b08a6a9ac 100644
--- a/tests/ref/vsynth/vsynth1-dv-411
+++ b/tests/ref/vsynth/vsynth1-dv-411
@@ -1,4 +1,4 @@
-df067afe65f1712d9e8efa7117aab6ea *tests/data/fate/vsynth1-dv-411.dv
+4c1017a99b78ff015415283906fb3a80 *tests/data/fate/vsynth1-dv-411.dv
 720 tests/data/fate/vsynth1-dv-411.dv
 ed493bad827dc903188fce8d3b597fcb *tests/data/fate/vsynth1-dv-411.out.rawvideo
 stddev:9.45 PSNR: 28.62 MAXDIFF:   84 bytes:  7603200/  7603200
diff --git a/tests/ref/vsynth/vsynth1-dv-50 b/tests/ref/vsynth/vsynth1-dv-50
index 4a2ff00110..54d0a531c1 100644
--- a/tests/ref/vsynth/vsynth1-dv-50
+++ b/tests/ref/vsynth/vsynth1-dv-50
@@ -1,4 +1,4 @@
-adb1df1a65cecab225677003a5de9f28 *tests/data/fate/vsynth1-dv-50.dv
+eddfa29c6fcee5574256eb9878112214 *tests/data/fate/vsynth1-dv-50.dv
 1440 tests/data/fate/vsynth1-dv-50.dv
 4ab1f5b7aad15fab9e3c1ea5b96da39b *tests/data/fate/vsynth1-dv-50.out.rawvideo
 stddev:1.72 PSNR: 43.37 MAXDIFF:   29 bytes:  7603200/  7603200
diff --git a/tests/ref/vsynth/vsynth1-dv-fhd b/tests/ref/vsynth/vsynth1-dv-fhd
index b81141f340..257898e0b2 100644
--- a/tests/ref/vsynth/vsynth1-dv-fhd
+++ b/tests/ref/vsynth/vsynth1-dv-fhd
@@ -1,4 +1,4 @@
-74315a8678d12c7f592c02990dc8952d *tests/data/fate/vsynth1-dv-fhd.dv
+d16b498c00ffaf3ed0b750e0a597bbe7 *tests/data/fate/vsynth1-dv-fhd.dv
 2880 tests/data/fate/vsynth1-dv-fhd.dv
 c95b309bc128b162e5c8241374eb66a9 *tests/data/fate/vsynth1-dv-fhd.out.rawvideo
 stddev:2.53 PSNR: 40.03 MAXDIFF:   35 bytes:  7603200/  7603200
diff --git a/tests/ref/vsynth/vsynth1-dv-hd b/tests/ref/vsynth/vsynth1-dv-hd
index 6b6d6e8159..e22f28c

[FFmpeg-devel] [PATCH 2/3] avutil/timecode: fix av_timecode_get_smpte_from_framenum with 50/60 fps

[Marton Balint]

50/60 fps timecode is using the field bit (which is the same as the phase
correction bit) to signal the least significant bit of a 50/60 fps timecode.
See SMPTE ST 12-1:2014 section 12.1.

Let's add support for this by using the recently added av_timecode_get_smpte
function which handles this properly.

This change affects DV and MXF encoder, MXF has no fate for 50/60fps content,
DV does, therefore the changes. It also affects decklink indev.

MediaInfo (a recent version) confirms that half-frame timecode must be
inserted. (although MediaInfo does not seem to check the field flag).
MXFInspect confirms valid timecode insertion to the System Item of MXF files.

Signed-off-by: Marton Balint 
---
 libavutil/timecode.c   | 15 +--
 libavutil/timecode.h   |  7 +++
 tests/ref/vsynth/vsynth1-dv-hd |  2 +-
 tests/ref/vsynth/vsynth2-dv-hd |  2 +-
 tests/ref/vsynth/vsynth3-dv-hd |  2 +-
 tests/ref/vsynth/vsynth_lena-dv-hd |  2 +-
 6 files changed, 8 insertions(+), 22 deletions(-)

diff --git a/libavutil/timecode.c b/libavutil/timecode.c
index cca53d73c4..cb916970ef 100644
--- a/libavutil/timecode.c
+++ b/libavutil/timecode.c
@@ -65,20 +65,7 @@ uint32_t av_timecode_get_smpte_from_framenum(const 
AVTimecode *tc, int framenum)
 ss = framenum / fps  % 60;
 mm = framenum / (fps*60) % 60;
 hh = framenum / (fps*3600) % 24;
-return 0 << 31 | // color frame flag (0: unsync mode, 1: sync mode)
-   drop  << 30 | // drop  frame flag (0: non drop,1: drop)
-   (ff / 10) << 28 | // tens  of frames
-   (ff % 10) << 24 | // units of frames
-   0 << 23 | // PC (NTSC) or BGF0 (PAL)
-   (ss / 10) << 20 | // tens  of seconds
-   (ss % 10) << 16 | // units of seconds
-   0 << 15 | // BGF0 (NTSC) or BGF2 (PAL)
-   (mm / 10) << 12 | // tens  of minutes
-   (mm % 10) <<  8 | // units of minutes
-   0 <<  7 | // BGF2 (NTSC) or PC (PAL)
-   0 <<  6 | // BGF1
-   (hh / 10) <<  4 | // tens  of hours
-   (hh % 10);// units of hours
+return av_timecode_get_smpte(tc->rate, drop, hh, mm, ss, ff);
 }
 
 uint32_t av_timecode_get_smpte(AVRational rate, int drop, int hh, int mm, int 
ss, int ff)
diff --git a/libavutil/timecode.h b/libavutil/timecode.h
index 5801330921..e54b116e93 100644
--- a/libavutil/timecode.h
+++ b/libavutil/timecode.h
@@ -66,11 +66,11 @@ int av_timecode_adjust_ntsc_framenum2(int framenum, int 
fps);
  * the format description as follows:
  * bits 0-5:   hours, in BCD(6bits)
  * bits 6: BGF1
- * bits 7: BGF2 (NTSC) or PC (PAL)
+ * bits 7: BGF2 (NTSC) or FIELD (PAL)
  * bits 8-14:  minutes, in BCD(7bits)
  * bits 15:BGF0 (NTSC) or BGF2 (PAL)
  * bits 16-22: seconds, in BCD(7bits)
- * bits 23:PC (NTSC) or BGF0 (PAL)
+ * bits 23:FIELD (NTSC) or BGF0 (PAL)
  * bits 24-29: frames, in BCD(6bits)
  * bits 30:drop  frame flag (0: non drop,1: drop)
  * bits 31:color frame flag (0: unsync mode, 1: sync mode)
@@ -78,8 +78,7 @@ int av_timecode_adjust_ntsc_framenum2(int framenum, int fps);
  * @note Frame number adjustment is automatically done in case of drop 
timecode,
  *   you do NOT have to call av_timecode_adjust_ntsc_framenum2().
  * @note The frame number is relative to tc->start.
- * @note Color frame (CF), binary group flags (BGF) and biphase mark polarity
- *   correction (PC) bits are set to zero.
+ * @note Color frame (CF) and binary group flags (BGF) bits are set to zero.
  */
 uint32_t av_timecode_get_smpte_from_framenum(const AVTimecode *tc, int 
framenum);
 
diff --git a/tests/ref/vsynth/vsynth1-dv-hd b/tests/ref/vsynth/vsynth1-dv-hd
index e22f28c704..a93ce50ec0 100644
--- a/tests/ref/vsynth/vsynth1-dv-hd
+++ b/tests/ref/vsynth/vsynth1-dv-hd
@@ -1,4 +1,4 @@
-77f146c73a24495495c649f1e0d26fee *tests/data/fate/vsynth1-dv-hd.dv
+ce9f887af2391edbfcb21675962db54c *tests/data/fate/vsynth1-dv-hd.dv
 1440 tests/data/fate/vsynth1-dv-hd.dv
 34b78cf725346c7f819c9d6209b8299a *tests/data/fate/vsynth1-dv-hd.out.rawvideo
 stddev:4.30 PSNR: 35.45 MAXDIFF:   74 bytes:  7603200/  7603200
diff --git a/tests/ref/vsynth/vsynth2-dv-hd b/tests/ref/vsynth/vsynth2-dv-hd
index 5c4ed86fe7..9552c31948 100644
--- a/tests/ref/vsynth/vsynth2-dv-hd
+++ b/tests/ref/vsynth/vsynth2-dv-hd
@@ -1,4 +1,4 @@
-168f80951daf5eeefacac33fd06db87d *tests/data/fate/vsynth2-dv-hd.dv
+c152c7c3f9ef8f404b4bb7a388b947be *tests/data/fate/vsynth2-dv-hd.dv
 1440 tests/data/fate/vsynth2-dv-hd.dv
 15dbe911532aca81c67bdd2846419027 *tests/data/fate/vsynth2-dv-hd.out.rawvideo
 stddev:1.75 PSNR: 43.26 MAXDIFF:   34 bytes:  7603200/  7603200
diff --git a/tests/ref/vsynth/vsynth3-dv-hd b/tests/ref/vsynth/vsynth3-dv-hd
index ed94d08155..d0420d6f07 100644
--- a/tests/ref/vsynth/vsynth3-dv-hd
+++ b/tests/ref/vsynth/vsynth3-dv-hd
@@ -1,4 +1,4 @@
-1211b843d8dde81c583023e15beff35a *tests/data/fate/vsynth3-dv

[FFmpeg-devel] [PATCH 3/3] avutil/timecode: cosmetics on av_timecode_get_smpte_from_framenum

[Marton Balint]

Signed-off-by: Marton Balint 
---
 libavutil/timecode.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/libavutil/timecode.c b/libavutil/timecode.c
index cb916970ef..b528e4a510 100644
--- a/libavutil/timecode.c
+++ b/libavutil/timecode.c
@@ -71,7 +71,7 @@ uint32_t av_timecode_get_smpte_from_framenum(const AVTimecode 
*tc, int framenum)
 uint32_t av_timecode_get_smpte(AVRational rate, int drop, int hh, int mm, int 
ss, int ff)
 {
 uint32_t tc = 0;
-uint32_t frames;
+int frames = ff;
 
 /* For SMPTE 12-M timecodes, frame count is a special case if > 30 FPS.
See SMPTE ST 12-1:2014 Sec 12.1 for more info. */
@@ -83,8 +83,6 @@ uint32_t av_timecode_get_smpte(AVRational rate, int drop, int 
hh, int mm, int ss
 else
 tc |= (1 << 23);
 }
-} else {
-frames = ff;
 }
 
 tc |= drop << 30;
@@ -95,7 +93,7 @@ uint32_t av_timecode_get_smpte(AVRational rate, int drop, int 
hh, int mm, int ss
 tc |= (mm / 10) << 12;
 tc |= (mm % 10) << 8;
 tc |= (hh / 10) << 4;
-tc |= (hh  % 10);
+tc |= (hh % 10);
 
 return tc;
 }
-- 
2.26.2

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avdevice/decklink_dec: mark the field flag if framerate > 30FPS

[Marton Balint]

On Mon, 20 Jul 2020, Marton Balint wrote:



This feature looks like it belongs to 
av_timecode_get_smpte_from_framenum and not into decklink.


I just sent a patch which implements this, because I confirmed that other 
code using av_timecode_get_smpte_from_framenum also seem to require the 
special handling for 50/60fps which you already implemented in 
av_timecode_get_smpte.


Regards,
Marton
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] Request for Technical committee action

[Zachariah Brown]

I'm just an independent observer who has submitted just a single trivial 
patch that decided to stay subscribed to this mailing list to keep up to 
date with what is happening. I have no bone in this game but I feel like 
something needs to be said.


Let me just say that this entire thread does not look good on the 
community as a whole. And yes, I read the entire threads before deciding 
to reply to this because I felt compelled to say something.


Paul, no body attacked you in anyway personally. I don't know what 
happened in the past because I've only been here for a couple of months, 
but it was very clear that Steinar was being purely objective about your 
patch. He provided valid criticism (which I agree with after looking at 
his evidence) about the result of the filter. You then proceeded to turn 
the multiple threads into personal attacks and claims that you were 
being discriminated against. That simply isn't the case.


You then proceeded to say that everyone is just wrong with no 
explanation as to why. When asked to leave comments your response was 
effectively "git gud". Comments are just as vital to a well maintained 
code base as good code is. I can say a lot more about this, but I'll 
leave it there.


And now you are threatening to do what? Your initial email in this 
thread was 12 hours ago, and from my limited understanding to how the 
FFmpeg development process works these committees just finished the 
voting process not too long ago and are a new concept entirely. You are 
hardly giving anyone time to look at the situation.


You need to take a chill pill, step back for a few days and let the dust 
settle. I'm not attacking you or being hostile, but your attitude in 
general is not that of someone that I would want to have on my team or 
have access to my code repos. There is a reason for peer review, to 
produce better quality code and a better resulting product. But that 
takes time, you can't rush it or threaten to go around people. That just 
degrades the situation further.


-Zachariah

On 7/20/2020 4:05 PM, Paul B Mahol wrote:

On 7/20/20, Paul B Mahol  wrote:

Look at thread:

avfilter: add ff_inlink_peek_samples and ff_inlink_skip samples

Another developer is constantly blocking my patches.


If TC does not act soon I will be forced to do it myself.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] Request for Technical committee action

[Steinar H. Gunderson]

On Mon, Jul 20, 2020 at 04:28:02PM -0400, Zachariah Brown wrote:
> Paul, no body attacked you in anyway personally. I don't know what happened
> in the past because I've only been here for a couple of months, but it was
> very clear that Steinar was being purely objective about your patch.

Like I did, you are confusing two threads; Paul's request for TC action was
not on the radial/circular blur patch, but on another one.

/* Steinar */
-- 
Homepage: https://www.sesse.net/
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2 1/3] libavutil/imgutils: add utility to get plane sizes

[Brian Kim]

Just wanted to check if there was any consensus on what we wanted to do
with these changes. Are we holding off until a future module wide change?

On Wed, Jul 15, 2020 at 8:09 AM James Almer  wrote:

> On 7/15/2020 4:06 AM, Nicolas George wrote:
> > Michael Niedermayer (12020-07-14):
> >> Let me phrase my suggestion in a more high level sense
> >>
> >> Currently the functions use int for lots of cases where they should not
> >> ultimately we want the functions to use more correct types for linesize
> >> sizes and so on.
> >>
> >> We need to add these function(s) because we fix a bug.
> >> Can we take a step toward more correct types while doing this in a
> >> way that makes sense ?
> >>
> >> if so that should be done.
> >>
> >> If not (as in its more mess than if its done later in some other way)
> >> then we should not try to do this now
> >>
> >> The idea is just to take a step towards how these functions/API should
> look
> >> ideally. Its not to make some inconvenient mess
> >
> > I strongly agree.
> >
> > If we use ints now, then the next time the same question comes this
> > function will be one more argument for using ints again. And the next
> > time, and the next time, all this making that much work for when we
> > cannot wait any longer to make the change, instead of that much less.
>
> I don't share the sentiment, since as i said an entire new set of
> functions will have to be added for a module-wide type switch. The way
> this function is designed here will not increase or reduce any future
> work in any way whatsoever. It's meant to be a solution today for a
> problem in the current state of the imgutils module.
>
> For all we know, by the time ints start being a real issue or the need
> to replace the current functions arise, the new set of functions might
> have to be designed in a way this one wont be reusable. For example,
> will AVPixelFormat have been replaced by then with an alternative that
> removes the need to have 20 entries to cover all bitdepths, chroma
> variants, endianness, and plane presence/order, for what's ultimately a
> single format?
>
> Sure, at first glance using the proper types here will seem like making
> a step forward, but we may instead be getting ahead of ourselves for no
> real gain. av_image_check_size() is truly future proof, as is
> av_image_check_sar(), but most others aren't, and that includes this one
> regardless of the data type we choose.
>
> >
> > Consistency is not a end in itself, it is a means to an end. And it is
> > one of the weakest arguments. If there are no other reason for doing
> things
> > one way or another, then sure, by all means let us choose the way that
> > looks the same at the rest of the code. But if there is a reason, if one
> > way is more efficient, or more convenient, or more future proof, or more
> > compatible, etc., then we choose this way, and too bad for consistency.
> >
> > Regards,
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] Request for Technical committee action

[Marvin Scholz]

On 20 Jul 2020, at 23:53, Steinar H. Gunderson wrote:


On Mon, Jul 20, 2020 at 04:28:02PM -0400, Zachariah Brown wrote:
Paul, no body attacked you in anyway personally. I don't know what 
happened
in the past because I've only been here for a couple of months, but 
it was

very clear that Steinar was being purely objective about your patch.


Like I did, you are confusing two threads; Paul's request for TC 
action was

not on the radial/circular blur patch, but on another one.



Nevertheless, a lot of the points raised in the mail are anyway
valid and I feel mostly the same.

As someone not very involved with FFmpeg and mostly a silent
observer I just never felt like I should comment on some of
the escalating discussions going on here…


/* Steinar */
--
Homepage: https://www.sesse.net/
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/3] avutil/timecode: fix av_timecode_get_smpte_from_framenum with 50/60 fps

[lance . lmwang]

On Mon, Jul 20, 2020 at 11:04:38PM +0200, Marton Balint wrote:
> 50/60 fps timecode is using the field bit (which is the same as the phase
> correction bit) to signal the least significant bit of a 50/60 fps timecode.
> See SMPTE ST 12-1:2014 section 12.1.
> 
> Let's add support for this by using the recently added av_timecode_get_smpte
> function which handles this properly.
> 
> This change affects DV and MXF encoder, MXF has no fate for 50/60fps content,
> DV does, therefore the changes. It also affects decklink indev.
> 
> MediaInfo (a recent version) confirms that half-frame timecode must be
> inserted. (although MediaInfo does not seem to check the field flag).
> MXFInspect confirms valid timecode insertion to the System Item of MXF files.
> 
> Signed-off-by: Marton Balint 
> ---
>  libavutil/timecode.c   | 15 +--
>  libavutil/timecode.h   |  7 +++
>  tests/ref/vsynth/vsynth1-dv-hd |  2 +-
>  tests/ref/vsynth/vsynth2-dv-hd |  2 +-
>  tests/ref/vsynth/vsynth3-dv-hd |  2 +-
>  tests/ref/vsynth/vsynth_lena-dv-hd |  2 +-
>  6 files changed, 8 insertions(+), 22 deletions(-)
> 
> diff --git a/libavutil/timecode.c b/libavutil/timecode.c
> index cca53d73c4..cb916970ef 100644
> --- a/libavutil/timecode.c
> +++ b/libavutil/timecode.c
> @@ -65,20 +65,7 @@ uint32_t av_timecode_get_smpte_from_framenum(const 
> AVTimecode *tc, int framenum)
>  ss = framenum / fps  % 60;
>  mm = framenum / (fps*60) % 60;
>  hh = framenum / (fps*3600) % 24;
> -return 0 << 31 | // color frame flag (0: unsync mode, 1: sync 
> mode)
> -   drop  << 30 | // drop  frame flag (0: non drop,1: drop)
> -   (ff / 10) << 28 | // tens  of frames
> -   (ff % 10) << 24 | // units of frames
> -   0 << 23 | // PC (NTSC) or BGF0 (PAL)
> -   (ss / 10) << 20 | // tens  of seconds
> -   (ss % 10) << 16 | // units of seconds
> -   0 << 15 | // BGF0 (NTSC) or BGF2 (PAL)
> -   (mm / 10) << 12 | // tens  of minutes
> -   (mm % 10) <<  8 | // units of minutes
> -   0 <<  7 | // BGF2 (NTSC) or PC (PAL)
> -   0 <<  6 | // BGF1
> -   (hh / 10) <<  4 | // tens  of hours
> -   (hh % 10);// units of hours
> +return av_timecode_get_smpte(tc->rate, drop, hh, mm, ss, ff);

av_timecode_get_smpte() is used by the h264/hevc, and the specs reserved more 
bits for ff, so in case it's
> 30fps, so av_timecode_get_smpte() will divide ff by 2 to get frame pair. But 
> for SMPTE ST 12 specs, the
ff reserved 2 bits for tens, so for > 30fps, it's supported by frame pair and ff
is divide by 2 already by the spec, so you'll get unexpected results if divide
by 2 again(four frame pair if test) 


>  }
>  
>  uint32_t av_timecode_get_smpte(AVRational rate, int drop, int hh, int mm, 
> int ss, int ff)
> diff --git a/libavutil/timecode.h b/libavutil/timecode.h
> index 5801330921..e54b116e93 100644
> --- a/libavutil/timecode.h
> +++ b/libavutil/timecode.h
> @@ -66,11 +66,11 @@ int av_timecode_adjust_ntsc_framenum2(int framenum, int 
> fps);
>   * the format description as follows:
>   * bits 0-5:   hours, in BCD(6bits)
>   * bits 6: BGF1
> - * bits 7: BGF2 (NTSC) or PC (PAL)
> + * bits 7: BGF2 (NTSC) or FIELD (PAL)
>   * bits 8-14:  minutes, in BCD(7bits)
>   * bits 15:BGF0 (NTSC) or BGF2 (PAL)
>   * bits 16-22: seconds, in BCD(7bits)
> - * bits 23:PC (NTSC) or BGF0 (PAL)
> + * bits 23:FIELD (NTSC) or BGF0 (PAL)
>   * bits 24-29: frames, in BCD(6bits)
>   * bits 30:drop  frame flag (0: non drop,1: drop)
>   * bits 31:color frame flag (0: unsync mode, 1: sync mode)
> @@ -78,8 +78,7 @@ int av_timecode_adjust_ntsc_framenum2(int framenum, int 
> fps);
>   * @note Frame number adjustment is automatically done in case of drop 
> timecode,
>   *   you do NOT have to call av_timecode_adjust_ntsc_framenum2().
>   * @note The frame number is relative to tc->start.
> - * @note Color frame (CF), binary group flags (BGF) and biphase mark polarity
> - *   correction (PC) bits are set to zero.
> + * @note Color frame (CF) and binary group flags (BGF) bits are set to zero.
>   */
>  uint32_t av_timecode_get_smpte_from_framenum(const AVTimecode *tc, int 
> framenum);
>  
> diff --git a/tests/ref/vsynth/vsynth1-dv-hd b/tests/ref/vsynth/vsynth1-dv-hd
> index e22f28c704..a93ce50ec0 100644
> --- a/tests/ref/vsynth/vsynth1-dv-hd
> +++ b/tests/ref/vsynth/vsynth1-dv-hd
> @@ -1,4 +1,4 @@
> -77f146c73a24495495c649f1e0d26fee *tests/data/fate/vsynth1-dv-hd.dv
> +ce9f887af2391edbfcb21675962db54c *tests/data/fate/vsynth1-dv-hd.dv
>  1440 tests/data/fate/vsynth1-dv-hd.dv
>  34b78cf725346c7f819c9d6209b8299a *tests/data/fate/vsynth1-dv-hd.out.rawvideo
>  stddev:4.30 PSNR: 35.45 MAXDIFF:   74 bytes:  7603200/  7603200
> diff --git a/tests/ref/vsynth/vsynth2-dv-hd b/tests/ref/vsynth/vsynth2-dv-hd
> index 5c4ed86fe7..9552c31948 100644
> --

Re: [FFmpeg-devel] [PATCH v2 1/2] avformat: Redo cleanup of demuxer upon read_header() failure

[James Almer]

On 7/19/2020 5:47 PM, Andreas Rheinhardt wrote:
> If reading the header fails, the demuxer's read_close() function (if
> existing) is not called automatically; instead several demuxers call it
> via "goto fail" in read_header().
> 
> This commit intends to change this by adding a flag to AVInputFormat
> that can be used to set on a per-AVInputFormat basis whether read_close()
> should be called generically after an error during read_header().
> 
> The flag controlling this behaviour needs to be added because it might
> be unsafe to call read_close() generally (e.g. this might lead to
> read_close() being called twice and this might e.g. lead to double-frees
> if av_free() is used instead of av_freep(); or a size field has not
> been reset after freeing the elements (see the mov demuxer for an
> example of this)). Yet the intention is to check and fix all demuxers
> and make the flag redundant in the medium run.
> 
> The flag itself is non-public (it resides in libavformat/internal.h),
> but it has been added to the ordinary (i.e. public) flags field of
> AVInputFormat, because there is no field for internal flags and adding
> one is not possible, because libavdevice also defines AVInputFormats
> and so there is the possibility that a newer libavformat is used
> together with an older libavdevice that would then lack the new field
> for internal flags. When it has become redundant, it can be removed again
> at the next major version bump.
> 
> Signed-off-by: Andreas Rheinhardt 
> ---
> This is an updated version of my initial patch [1]. I have also rebased
> the whole set of patches following it (with the exception of the w3c
> patch in the next patch they no longer fix a memleak; instead they now
> only set the flag and remove the manual calls to read_close). Should I
> resend the other patches, too?
> 
> [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-March/258830.html
> 
>  libavformat/internal.h |  6 ++
>  libavformat/utils.c| 11 +--
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/internal.h b/libavformat/internal.h
> index 17a6ab07d3..b7441a5959 100644
> --- a/libavformat/internal.h
> +++ b/libavformat/internal.h
> @@ -39,6 +39,12 @@
>  #define hex_dump_debug(class, buf, size) do { if (0) 
> av_hex_dump_log(class, AV_LOG_DEBUG, buf, size); } while(0)
>  #endif
>  
> +/** Internal flag that is part of AVInputFormat.flags due to
> + *  ABI restrictions that forbid adding a new flags_internal
> + *  to AVInputFormat. */

You can add fields below the "Not public" notice with a minor bump.
Nothing is meant to access those directly, except unfortunately lavd.
And if you're effectively talking about lavd, then adding it at the end
should not affect the offsets of fields currently accessed by indevs. Or
am i missing something?

> +#define AVFMT_HEADER_CLEANUP 0x4000 /**< read_close() should be called
> + on read_header() failure */
> +
>  typedef struct AVCodecTag {
>  enum AVCodecID id;
>  unsigned int tag;
> diff --git a/libavformat/utils.c b/libavformat/utils.c
> index 807d9f10cb..2148a03497 100644
> --- a/libavformat/utils.c
> +++ b/libavformat/utils.c
> @@ -396,8 +396,12 @@ int av_demuxer_open(AVFormatContext *ic) {
>  
>  if (ic->iformat->read_header) {
>  err = ic->iformat->read_header(ic);
> -if (err < 0)
> +if (err < 0) {
> +if (ic->iformat->read_close &&
> +ic->iformat->flags & AVFMT_HEADER_CLEANUP)
> +ic->iformat->read_close(ic);
>  return err;
> +}
>  }
>  
>  if (ic->pb && !ic->internal->data_offset)
> @@ -624,8 +628,11 @@ FF_ENABLE_DEPRECATION_WARNINGS
>  
>  
>  if (!(s->flags&AVFMT_FLAG_PRIV_OPT) && s->iformat->read_header)
> -if ((ret = s->iformat->read_header(s)) < 0)
> +if ((ret = s->iformat->read_header(s)) < 0) {
> +if (s->iformat->flags & AVFMT_HEADER_CLEANUP)
> +goto close;
>  goto fail;
> +}
>  
>  if (!s->metadata) {
>  s->metadata = s->internal->id3v2_meta;
> 

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2 1/2] avformat: Redo cleanup of demuxer upon read_header() failure

[Andreas Rheinhardt]

James Almer:
> On 7/19/2020 5:47 PM, Andreas Rheinhardt wrote:
>> If reading the header fails, the demuxer's read_close() function (if
>> existing) is not called automatically; instead several demuxers call it
>> via "goto fail" in read_header().
>>
>> This commit intends to change this by adding a flag to AVInputFormat
>> that can be used to set on a per-AVInputFormat basis whether read_close()
>> should be called generically after an error during read_header().
>>
>> The flag controlling this behaviour needs to be added because it might
>> be unsafe to call read_close() generally (e.g. this might lead to
>> read_close() being called twice and this might e.g. lead to double-frees
>> if av_free() is used instead of av_freep(); or a size field has not
>> been reset after freeing the elements (see the mov demuxer for an
>> example of this)). Yet the intention is to check and fix all demuxers
>> and make the flag redundant in the medium run.
>>
>> The flag itself is non-public (it resides in libavformat/internal.h),
>> but it has been added to the ordinary (i.e. public) flags field of
>> AVInputFormat, because there is no field for internal flags and adding
>> one is not possible, because libavdevice also defines AVInputFormats
>> and so there is the possibility that a newer libavformat is used
>> together with an older libavdevice that would then lack the new field
>> for internal flags. When it has become redundant, it can be removed again
>> at the next major version bump.
>>
>> Signed-off-by: Andreas Rheinhardt 
>> ---
>> This is an updated version of my initial patch [1]. I have also rebased
>> the whole set of patches following it (with the exception of the w3c
>> patch in the next patch they no longer fix a memleak; instead they now
>> only set the flag and remove the manual calls to read_close). Should I
>> resend the other patches, too?
>>
>> [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-March/258830.html
>>
>>  libavformat/internal.h |  6 ++
>>  libavformat/utils.c| 11 +--
>>  2 files changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavformat/internal.h b/libavformat/internal.h
>> index 17a6ab07d3..b7441a5959 100644
>> --- a/libavformat/internal.h
>> +++ b/libavformat/internal.h
>> @@ -39,6 +39,12 @@
>>  #define hex_dump_debug(class, buf, size) do { if (0) 
>> av_hex_dump_log(class, AV_LOG_DEBUG, buf, size); } while(0)
>>  #endif
>>  
>> +/** Internal flag that is part of AVInputFormat.flags due to
>> + *  ABI restrictions that forbid adding a new flags_internal
>> + *  to AVInputFormat. */
> 
> You can add fields below the "Not public" notice with a minor bump.
> Nothing is meant to access those directly, except unfortunately lavd.
> And if you're effectively talking about lavd, then adding it at the end
> should not affect the offsets of fields currently accessed by indevs. Or
> am i missing something?
> 
The point is that it is (unfortunately) allowed to use an older
libavdevice together with a newer libavformat. This means it is possible
that the AVInputFormat used in avformat_open_input() may be from a
libavdevice that is so old that it doesn't have the new internal flags
field yet. So one either uses an unused bit of the ordinary flags or one
adds functions that allow to check whether a given AVInputFormat is an
input device from a too old libavdevice. Or one waits with this change
until the next major bump and does everything at once.

Notice that in any case, every demuxer with a read_close function needs
to be checked for compatibility with calling read_close generically on
read_header error. I have just found two more (besides mov) demuxers
(rmdec and concat) that are not. Adding this flag allows to easily see
which demuxers have already been checked.

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2 1/3] libavutil/imgutils: add utility to get plane sizes

[James Almer]

On 7/20/2020 6:32 PM, Brian Kim wrote:
> Just wanted to check if there was any consensus on what we wanted to do
> with these changes. Are we holding off until a future module wide change?

No, i'll push v3 soon if my argumentation below was not enough to
convince Nicolas or Michael. My intention is to use ints for the new
function, not to postpone committing it in any form indefinitely.

> 
> On Wed, Jul 15, 2020 at 8:09 AM James Almer  wrote:
> 
>> On 7/15/2020 4:06 AM, Nicolas George wrote:
>>> Michael Niedermayer (12020-07-14):
 Let me phrase my suggestion in a more high level sense

 Currently the functions use int for lots of cases where they should not
 ultimately we want the functions to use more correct types for linesize
 sizes and so on.

 We need to add these function(s) because we fix a bug.
 Can we take a step toward more correct types while doing this in a
 way that makes sense ?

 if so that should be done.

 If not (as in its more mess than if its done later in some other way)
 then we should not try to do this now

 The idea is just to take a step towards how these functions/API should
>> look
 ideally. Its not to make some inconvenient mess
>>>
>>> I strongly agree.
>>>
>>> If we use ints now, then the next time the same question comes this
>>> function will be one more argument for using ints again. And the next
>>> time, and the next time, all this making that much work for when we
>>> cannot wait any longer to make the change, instead of that much less.
>>
>> I don't share the sentiment, since as i said an entire new set of
>> functions will have to be added for a module-wide type switch. The way
>> this function is designed here will not increase or reduce any future
>> work in any way whatsoever. It's meant to be a solution today for a
>> problem in the current state of the imgutils module.
>>
>> For all we know, by the time ints start being a real issue or the need
>> to replace the current functions arise, the new set of functions might
>> have to be designed in a way this one wont be reusable. For example,
>> will AVPixelFormat have been replaced by then with an alternative that
>> removes the need to have 20 entries to cover all bitdepths, chroma
>> variants, endianness, and plane presence/order, for what's ultimately a
>> single format?
>>
>> Sure, at first glance using the proper types here will seem like making
>> a step forward, but we may instead be getting ahead of ourselves for no
>> real gain. av_image_check_size() is truly future proof, as is
>> av_image_check_sar(), but most others aren't, and that includes this one
>> regardless of the data type we choose.
>>
>>>
>>> Consistency is not a end in itself, it is a means to an end. And it is
>>> one of the weakest arguments. If there are no other reason for doing
>> things
>>> one way or another, then sure, by all means let us choose the way that
>>> looks the same at the rest of the code. But if there is a reason, if one
>>> way is more efficient, or more convenient, or more future proof, or more
>>> compatible, etc., then we choose this way, and too bad for consistency.
>>>
>>> Regards,
>> ___
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
> 

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH v2 1/2] avformat: Redo cleanup of demuxer upon read_header() failure

[James Almer]

On 7/20/2020 9:35 PM, Andreas Rheinhardt wrote:
> James Almer:
>> On 7/19/2020 5:47 PM, Andreas Rheinhardt wrote:
>>> If reading the header fails, the demuxer's read_close() function (if
>>> existing) is not called automatically; instead several demuxers call it
>>> via "goto fail" in read_header().
>>>
>>> This commit intends to change this by adding a flag to AVInputFormat
>>> that can be used to set on a per-AVInputFormat basis whether read_close()
>>> should be called generically after an error during read_header().
>>>
>>> The flag controlling this behaviour needs to be added because it might
>>> be unsafe to call read_close() generally (e.g. this might lead to
>>> read_close() being called twice and this might e.g. lead to double-frees
>>> if av_free() is used instead of av_freep(); or a size field has not
>>> been reset after freeing the elements (see the mov demuxer for an
>>> example of this)). Yet the intention is to check and fix all demuxers
>>> and make the flag redundant in the medium run.
>>>
>>> The flag itself is non-public (it resides in libavformat/internal.h),
>>> but it has been added to the ordinary (i.e. public) flags field of
>>> AVInputFormat, because there is no field for internal flags and adding
>>> one is not possible, because libavdevice also defines AVInputFormats
>>> and so there is the possibility that a newer libavformat is used
>>> together with an older libavdevice that would then lack the new field
>>> for internal flags. When it has become redundant, it can be removed again
>>> at the next major version bump.
>>>
>>> Signed-off-by: Andreas Rheinhardt 
>>> ---
>>> This is an updated version of my initial patch [1]. I have also rebased
>>> the whole set of patches following it (with the exception of the w3c
>>> patch in the next patch they no longer fix a memleak; instead they now
>>> only set the flag and remove the manual calls to read_close). Should I
>>> resend the other patches, too?
>>>
>>> [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2020-March/258830.html
>>>
>>>  libavformat/internal.h |  6 ++
>>>  libavformat/utils.c| 11 +--
>>>  2 files changed, 15 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libavformat/internal.h b/libavformat/internal.h
>>> index 17a6ab07d3..b7441a5959 100644
>>> --- a/libavformat/internal.h
>>> +++ b/libavformat/internal.h
>>> @@ -39,6 +39,12 @@
>>>  #define hex_dump_debug(class, buf, size) do { if (0) 
>>> av_hex_dump_log(class, AV_LOG_DEBUG, buf, size); } while(0)
>>>  #endif
>>>  
>>> +/** Internal flag that is part of AVInputFormat.flags due to
>>> + *  ABI restrictions that forbid adding a new flags_internal
>>> + *  to AVInputFormat. */
>>
>> You can add fields below the "Not public" notice with a minor bump.
>> Nothing is meant to access those directly, except unfortunately lavd.
>> And if you're effectively talking about lavd, then adding it at the end
>> should not affect the offsets of fields currently accessed by indevs. Or
>> am i missing something?
>>
> The point is that it is (unfortunately) allowed to use an older
> libavdevice together with a newer libavformat. This means it is possible
> that the AVInputFormat used in avformat_open_input() may be from a
> libavdevice that is so old that it doesn't have the new internal flags
> field yet. So one either uses an unused bit of the ordinary flags or one
> adds functions that allow to check whether a given AVInputFormat is an
> input device from a too old libavdevice. Or one waits with this change
> until the next major bump and does everything at once.

Wouldn't a check for AV_IS_INPUT_DEVICE(ifmt->priv_class->category)
before trying to look at the new flags_internal field in
avformat_open_input() be enough? Perhaps with an avdevice_version()
check as well in case a device cares about using it. This can be safely
removed after a major bump.

> 
> Notice that in any case, every demuxer with a read_close function needs
> to be checked for compatibility with calling read_close generically on
> read_header error. I have just found two more (besides mov) demuxers
> (rmdec and concat) that are not. Adding this flag allows to easily see
> which demuxers have already been checked.
> 
> - Andreas
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
> 

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 3/9] avformat/mxfdec: Simplify cleanup after read_header failure

[Andreas Rheinhardt]

by setting the AVFMT_HEADER_CLEANUP flag.

Signed-off-by: Andreas Rheinhardt 
---
 libavformat/mxfdec.c | 24 +++-
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index 90546d42b3..06c6e0890b 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -302,8 +302,6 @@ typedef struct MXFMetadataReadTableEntry {
 enum MXFMetadataSetType type;
 } MXFMetadataReadTableEntry;
 
-static int mxf_read_close(AVFormatContext *s);
-
 /* partial keys to match */
 static const uint8_t mxf_header_partition_pack_key[]   = { 
0x06,0x0e,0x2b,0x34,0x02,0x05,0x01,0x01,0x0d,0x01,0x02,0x01,0x01,0x02 };
 static const uint8_t mxf_essence_element_key[] = { 
0x06,0x0e,0x2b,0x34,0x01,0x02,0x01,0x01,0x0d,0x01,0x03,0x01 };
@@ -3169,7 +3167,6 @@ static int mxf_read_header(AVFormatContext *s)
 
 if (!mxf_read_sync(s->pb, mxf_header_partition_pack_key, 14)) {
 av_log(s, AV_LOG_ERROR, "could not find header partition pack key\n");
-//goto fail should not be needed as no metadata sets will have been 
parsed yet
 return AVERROR_INVALIDDATA;
 }
 avio_seek(s->pb, -14, SEEK_CUR);
@@ -3200,8 +3197,7 @@ static int mxf_read_header(AVFormatContext *s)
 
 if (!mxf->current_partition) {
 av_log(mxf->fc, AV_LOG_ERROR, "found essence prior to first 
PartitionPack\n");
-ret = AVERROR_INVALIDDATA;
-goto fail;
+return AVERROR_INVALIDDATA;
 }
 
 if (!mxf->current_partition->first_essence_klv.offset)
@@ -3226,7 +3222,7 @@ static int mxf_read_header(AVFormatContext *s)
 for (metadata = mxf_metadata_read_table; metadata->read; metadata++) {
 if (IS_KLV_KEY(klv.key, metadata->key)) {
 if ((ret = mxf_parse_klv(mxf, klv, metadata->read, 
metadata->ctx_size, metadata->type)) < 0)
-goto fail;
+return ret;
 break;
 }
 }
@@ -3239,21 +3235,20 @@ static int mxf_read_header(AVFormatContext *s)
 /* FIXME avoid seek */
 if (!essence_offset)  {
 av_log(s, AV_LOG_ERROR, "no essence\n");
-ret = AVERROR_INVALIDDATA;
-goto fail;
+return AVERROR_INVALIDDATA;
 }
 avio_seek(s->pb, essence_offset, SEEK_SET);
 
 /* we need to do this before computing the index tables
  * to be able to fill in zero IndexDurations with st->duration */
 if ((ret = mxf_parse_structural_metadata(mxf)) < 0)
-goto fail;
+return ret;
 
 for (int i = 0; i < s->nb_streams; i++)
 mxf_handle_missing_index_segment(mxf, s->streams[i]);
 
 if ((ret = mxf_compute_index_tables(mxf)) < 0)
-goto fail;
+return ret;
 
 if (mxf->nb_index_tables > 1) {
 /* TODO: look up which IndexSID to use via EssenceContainerData */
@@ -3261,8 +3256,7 @@ static int mxf_read_header(AVFormatContext *s)
mxf->nb_index_tables, mxf->index_tables[0].index_sid);
 } else if (mxf->nb_index_tables == 0 && mxf->op == OPAtom && 
(s->error_recognition & AV_EF_EXPLODE)) {
 av_log(mxf->fc, AV_LOG_ERROR, "cannot demux OPAtom without an 
index\n");
-ret = AVERROR_INVALIDDATA;
-goto fail;
+return AVERROR_INVALIDDATA;
 }
 
 mxf_compute_essence_containers(s);
@@ -3271,10 +3265,6 @@ static int mxf_read_header(AVFormatContext *s)
 mxf_compute_edit_units_per_packet(mxf, s->streams[i]);
 
 return 0;
-fail:
-mxf_read_close(s);
-
-return ret;
 }
 
 /* Get the edit unit of the next packet from current_offset in a track. The 
returned edit unit can be original_duration as well! */
@@ -3740,7 +3730,7 @@ static const AVClass demuxer_class = {
 AVInputFormat ff_mxf_demuxer = {
 .name   = "mxf",
 .long_name  = NULL_IF_CONFIG_SMALL("MXF (Material eXchange Format)"),
-.flags  = AVFMT_SEEK_TO_PTS,
+.flags  = AVFMT_SEEK_TO_PTS | AVFMT_HEADER_CLEANUP,
 .priv_data_size = sizeof(MXFContext),
 .read_probe = mxf_probe,
 .read_header= mxf_read_header,
-- 
2.20.1

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 4/9] avformat/rmdec: Actually return value < 0 on read_header failure

[Andreas Rheinhardt]

The RealMedia demuxer's read_header function initially initializes ret,
the variable designated for the return variable to -1. Afterwards, chunks
of the file are parsed in a loop until an error happens or until the actual
frame data is encountered. If the first function whose return
value is put into ret doesn't fail, then ret contains a value >= 0
(actually == 0) and this is what will be returned if an error is
encountered afterwards.

This is a regression since 35bbc1955a58ba74552c50d9161084644f00bbd3.
Before that, ret had never been overwritten with a nonnegative value.

Signed-off-by: Andreas Rheinhardt 
---
 libavformat/rmdec.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
index a36e693ab2..6851b7e1f4 100644
--- a/libavformat/rmdec.c
+++ b/libavformat/rmdec.c
@@ -538,7 +538,7 @@ static int rm_read_header(AVFormatContext *s)
 unsigned int data_off = 0, indx_off = 0;
 char buf[128], mime[128];
 int flags = 0;
-int ret = -1;
+int ret;
 unsigned size, v;
 int64_t codec_pos;
 
@@ -554,6 +554,7 @@ static int rm_read_header(AVFormatContext *s)
 avio_skip(pb, tag_size - 8);
 
 for(;;) {
+ret = AVERROR_INVALIDDATA;
 if (avio_feof(pb))
 goto fail;
 tag = avio_rl32(pb);
@@ -619,8 +620,9 @@ static int rm_read_header(AVFormatContext *s)
 avio_seek(pb, codec_pos + size, SEEK_SET);
 } else {
 avio_skip(pb, -4);
-if (ff_rm_read_mdpr_codecdata(s, s->pb, st, st->priv_data,
-  size, mime) < 0)
+ret = ff_rm_read_mdpr_codecdata(s, s->pb, st, st->priv_data,
+size, mime);
+if (ret < 0)
 goto fail;
 }
 
-- 
2.20.1

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 5/9] avformat/rmdec: Fix potential crash on allocation failure

[Andreas Rheinhardt]

The RealMedia demuxer uses the priv_data of its streams to store a
structure containing an AVPacket. These packets are unreferenced in the
read_close function, yet said function simply presumed that the
priv_data has been successfully allocated. This implies that it mustn't
be called when an allocation of priv_data fails; but this can happen
since commit 35bbc1955a58ba74552c50d9161084644f00bbd3 if one has a
stream with multiple substreams (also exported as AVStream) and if
allocating the priv_data for one of these substreams fails.

This has been fixed by making sure that read_close can handle the case
in which priv_data has not been successfully allocated.

Signed-off-by: Andreas Rheinhardt 
---
This here is another reason why every demuxer needs to be carefully
checked for whether it is compatible with calling read_close
automatically.

 libavformat/rmdec.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
index 6851b7e1f4..72b8dba741 100644
--- a/libavformat/rmdec.c
+++ b/libavformat/rmdec.c
@@ -115,6 +115,9 @@ RMStream *ff_rm_alloc_rmstream (void)
 
 void ff_rm_free_rmstream (RMStream *rms)
 {
+if (!rms)
+return;
+
 av_packet_unref(&rms->pkt);
 }
 
-- 
2.20.1

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 8/9] avformat/paf: Simplify cleanup after read_header failure

[Andreas Rheinhardt]

by setting the AVFMT_HEADER_CLEANUP flag.

Signed-off-by: Andreas Rheinhardt 
---
 libavformat/paf.c | 10 ++
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/libavformat/paf.c b/libavformat/paf.c
index a31d01502b..9072c79edd 100644
--- a/libavformat/paf.c
+++ b/libavformat/paf.c
@@ -90,7 +90,6 @@ static int read_header(AVFormatContext *s)
 PAFDemuxContext *p  = s->priv_data;
 AVIOContext *pb = s->pb;
 AVStream*ast, *vst;
-int ret = 0;
 
 avio_skip(pb, 132);
 
@@ -165,8 +164,7 @@ static int read_header(AVFormatContext *s)
 !p->video_frame ||
 !p->audio_frame ||
 !p->temp_audio_frame) {
-ret = AVERROR(ENOMEM);
-goto fail;
+return AVERROR(ENOMEM);
 }
 
 avio_seek(pb, p->buffer_size, SEEK_SET);
@@ -182,11 +180,6 @@ static int read_header(AVFormatContext *s)
 avio_seek(pb, p->start_offset, SEEK_SET);
 
 return 0;
-
-fail:
-read_close(s);
-
-return ret;
 }
 
 static int read_packet(AVFormatContext *s, AVPacket *pkt)
@@ -260,6 +253,7 @@ static int read_packet(AVFormatContext *s, AVPacket *pkt)
 AVInputFormat ff_paf_demuxer = {
 .name   = "paf",
 .long_name  = NULL_IF_CONFIG_SMALL("Amazing Studio Packed Animation 
File"),
+.flags  = AVFMT_HEADER_CLEANUP,
 .priv_data_size = sizeof(PAFDemuxContext),
 .read_probe = read_probe,
 .read_header= read_header,
-- 
2.20.1

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 7/9] avformat/rmdec: Fix potential shift outside of range of int

[Andreas Rheinhardt]

The loop variable here that can be as high as UINT16_MAX - 1 gets
left-shifted by 16 bits which is outside the range of int. So use
unsigned.

Signed-off-by: Andreas Rheinhardt 
---
 libavformat/rmdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
index c88f41c121..e97b861dee 100644
--- a/libavformat/rmdec.c
+++ b/libavformat/rmdec.c
@@ -500,7 +500,7 @@ static int rm_read_multi(AVFormatContext *s, AVIOContext 
*pb,
 if (number_of_mdpr != 1) {
 avpriv_request_sample(s, "MLTI with multiple (%d) MDPR", 
number_of_mdpr);
 }
-for (i = 0; i < number_of_mdpr; i++) {
+for (unsigned i = 0; i < number_of_mdpr; i++) {
 AVStream *st2;
 if (i > 0) {
 st2 = avformat_new_stream(s, NULL);
-- 
2.20.1

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 9/9] avformat/concatdec: Simplify cleanup after read_header failure

[Andreas Rheinhardt]

by setting the AVFMT_HEADER_CLEANUP flag.

(Btw: concat_read_close() is not idempotent (it frees cat->files, but
doesn't reset cat->nb_files), so this demuxer was incompatible with
simply calling read_close generically upon read_header failure.)

Signed-off-by: Andreas Rheinhardt 
---
 libavformat/concatdec.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/libavformat/concatdec.c b/libavformat/concatdec.c
index 4b56b61404..5f3c63a621 100644
--- a/libavformat/concatdec.c
+++ b/libavformat/concatdec.c
@@ -510,12 +510,9 @@ static int concat_read_header(AVFormatContext *avf)
MATCH_ONE_TO_ONE;
 if ((ret = open_file(avf, 0)) < 0)
 goto fail;
-av_bprint_finalize(&bp, NULL);
-return 0;
 
 fail:
 av_bprint_finalize(&bp, NULL);
-concat_read_close(avf);
 return ret;
 }
 
@@ -778,6 +775,7 @@ static const AVClass concat_class = {
 AVInputFormat ff_concat_demuxer = {
 .name   = "concat",
 .long_name  = NULL_IF_CONFIG_SMALL("Virtual concatenation script"),
+.flags  = AVFMT_HEADER_CLEANUP,
 .priv_data_size = sizeof(ConcatContext),
 .read_probe = concat_probe,
 .read_header= concat_read_header,
-- 
2.20.1

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 6/9] avformat/rmdec: Fix memleaks upon read_header failure

[Andreas Rheinhardt]

For both the RealMedia as well as the IVR demuxer (which share the same
context) each AVStream's priv_data contains an AVPacket that might
contain data (even when reading the header) and therefore needs to be
unreferenced. Up until now, this has not always been done:

The RealMedia demuxer didn't do it when allocating a new stream's
priv_data failed although there might other streams with packets to
unreference. (The reason for this was that until recently rm_read_close()
couldn't handle an AVStream without priv_data, so one had to choose
between a potential crash and a memleak.)

The IVR demuxer meanwhile never ever called read_close so that the data
already contained in packets leaks upon error.

This patch fixes both demuxers by setting the AVFMT_HEADER_CLEANUP flag,
thereby ensuring that rm_read_close() is always called when reading the
header fails. This also allows to remove several "goto fail" in
rm_read_header().

Signed-off-by: Andreas Rheinhardt 
---
 libavformat/rmdec.c | 20 +++-
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
index 72b8dba741..c88f41c121 100644
--- a/libavformat/rmdec.c
+++ b/libavformat/rmdec.c
@@ -66,8 +66,6 @@ typedef struct RMDemuxContext {
 int data_end;
 } RMDemuxContext;
 
-static int rm_read_close(AVFormatContext *s);
-
 static inline void get_strl(AVIOContext *pb, char *buf, int buf_size, int len)
 {
 int read = avio_get_str(pb, len, buf, buf_size);
@@ -557,16 +555,15 @@ static int rm_read_header(AVFormatContext *s)
 avio_skip(pb, tag_size - 8);
 
 for(;;) {
-ret = AVERROR_INVALIDDATA;
 if (avio_feof(pb))
-goto fail;
+return AVERROR_INVALIDDATA;
 tag = avio_rl32(pb);
 tag_size = avio_rb32(pb);
 avio_rb16(pb);
 av_log(s, AV_LOG_TRACE, "tag=%s size=%d\n",
av_fourcc2str(tag), tag_size);
 if (tag_size < 10 && tag != MKTAG('D', 'A', 'T', 'A'))
-goto fail;
+return AVERROR_INVALIDDATA;
 switch(tag) {
 case MKTAG('P', 'R', 'O', 'P'):
 /* file header */
@@ -589,8 +586,7 @@ static int rm_read_header(AVFormatContext *s)
 case MKTAG('M', 'D', 'P', 'R'):
 st = avformat_new_stream(s, NULL);
 if (!st) {
-ret = AVERROR(ENOMEM);
-goto fail;
+return AVERROR(ENOMEM);
 }
 st->id = avio_rb16(pb);
 avio_rb32(pb); /* max bit rate */
@@ -619,14 +615,14 @@ static int rm_read_header(AVFormatContext *s)
 if (v == MKBETAG('M', 'L', 'T', 'I')) {
 ret = rm_read_multi(s, s->pb, st, mime);
 if (ret < 0)
-goto fail;
+return ret;
 avio_seek(pb, codec_pos + size, SEEK_SET);
 } else {
 avio_skip(pb, -4);
 ret = ff_rm_read_mdpr_codecdata(s, s->pb, st, st->priv_data,
 size, mime);
 if (ret < 0)
-goto fail;
+return ret;
 }
 
 break;
@@ -654,10 +650,6 @@ static int rm_read_header(AVFormatContext *s)
 }
 
 return 0;
-
-fail:
-rm_read_close(s);
-return ret;
 }
 
 static int get_num(AVIOContext *pb, int *len)
@@ -1141,6 +1133,7 @@ static int rm_read_seek(AVFormatContext *s, int 
stream_index,
 AVInputFormat ff_rm_demuxer = {
 .name   = "rm",
 .long_name  = NULL_IF_CONFIG_SMALL("RealMedia"),
+.flags  = AVFMT_HEADER_CLEANUP,
 .priv_data_size = sizeof(RMDemuxContext),
 .read_probe = rm_probe,
 .read_header= rm_read_header,
@@ -1393,6 +1386,7 @@ static int ivr_read_packet(AVFormatContext *s, AVPacket 
*pkt)
 AVInputFormat ff_ivr_demuxer = {
 .name   = "ivr",
 .long_name  = NULL_IF_CONFIG_SMALL("IVR (Internet Video Recording)"),
+.flags  = AVFMT_HEADER_CLEANUP,
 .priv_data_size = sizeof(RMDemuxContext),
 .read_probe = ivr_probe,
 .read_header= ivr_read_header,
-- 
2.20.1

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 7/9] avformat/rmdec: Fix potential shift outside of range of int

[James Almer]

On 7/20/2020 11:12 PM, Andreas Rheinhardt wrote:
> The loop variable here that can be as high as UINT16_MAX - 1 gets
> left-shifted by 16 bits which is outside the range of int. So use
> unsigned.
> 
> Signed-off-by: Andreas Rheinhardt 
> ---
>  libavformat/rmdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavformat/rmdec.c b/libavformat/rmdec.c
> index c88f41c121..e97b861dee 100644
> --- a/libavformat/rmdec.c
> +++ b/libavformat/rmdec.c
> @@ -500,7 +500,7 @@ static int rm_read_multi(AVFormatContext *s, AVIOContext 
> *pb,
>  if (number_of_mdpr != 1) {
>  avpriv_request_sample(s, "MLTI with multiple (%d) MDPR", 
> number_of_mdpr);

So most of the code below is untested?

Also, AVStream->id is an int, so maybe just ensure number_of_mdpr is
equal or less than INT16_MAX, and perhaps also that st->id is equal or
less than UINT16_MAX before doing the addition, and abort otherwise
instead of changing the type for i.

>  }
> -for (i = 0; i < number_of_mdpr; i++) {
> +for (unsigned i = 0; i < number_of_mdpr; i++) {
>  AVStream *st2;
>  if (i > 0) {
>  st2 = avformat_new_stream(s, NULL);
> 

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avcodec/libvpxenc: use pix_fmt descriptors where useful

[James Almer]

On 7/20/2020 3:01 PM, James Zern wrote:
> similar to:
> 36e51c190b avcodec/libaomenc: use pix_fmt descriptors where useful
> 
> Signed-off-by: James Zern 
> ---
>  libavcodec/libvpxenc.c | 13 +++--
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/libavcodec/libvpxenc.c b/libavcodec/libvpxenc.c
> index 8e0ea42375..3d5e6c12e3 100644
> --- a/libavcodec/libvpxenc.c
> +++ b/libavcodec/libvpxenc.c
> @@ -41,6 +41,7 @@
>  #include "libavutil/intreadwrite.h"
>  #include "libavutil/mathematics.h"
>  #include "libavutil/opt.h"
> +#include "libavutil/pixdesc.h"
>  
>  /**
>   * Portion of struct vpx_codec_cx_pkt from vpx_encoder.h.
> @@ -546,7 +547,8 @@ static int set_pix_fmt(AVCodecContext *avctx, 
> vpx_codec_caps_t codec_caps,
> vpx_img_fmt_t *img_fmt)
>  {
>  VPxContext av_unused *ctx = avctx->priv_data;
> -enccfg->g_bit_depth = enccfg->g_input_bit_depth = 8;
> +const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(avctx->pix_fmt);
> +enccfg->g_bit_depth = enccfg->g_input_bit_depth = desc->comp[0].depth;
>  switch (avctx->pix_fmt) {
>  case AV_PIX_FMT_YUV420P:
>  case AV_PIX_FMT_YUVA420P:
> @@ -570,8 +572,6 @@ static int set_pix_fmt(AVCodecContext *avctx, 
> vpx_codec_caps_t codec_caps,
>  case AV_PIX_FMT_YUV420P10:
>  case AV_PIX_FMT_YUV420P12:
>  if (codec_caps & VPX_CODEC_CAP_HIGHBITDEPTH) {
> -enccfg->g_bit_depth = enccfg->g_input_bit_depth =
> -avctx->pix_fmt == AV_PIX_FMT_YUV420P10 ? 10 : 12;
>  enccfg->g_profile = 2;
>  *img_fmt = VPX_IMG_FMT_I42016;
>  *flags |= VPX_CODEC_USE_HIGHBITDEPTH;
> @@ -581,8 +581,6 @@ static int set_pix_fmt(AVCodecContext *avctx, 
> vpx_codec_caps_t codec_caps,
>  case AV_PIX_FMT_YUV422P10:
>  case AV_PIX_FMT_YUV422P12:
>  if (codec_caps & VPX_CODEC_CAP_HIGHBITDEPTH) {
> -enccfg->g_bit_depth = enccfg->g_input_bit_depth =
> -avctx->pix_fmt == AV_PIX_FMT_YUV422P10 ? 10 : 12;
>  enccfg->g_profile = 3;
>  *img_fmt = VPX_IMG_FMT_I42216;
>  *flags |= VPX_CODEC_USE_HIGHBITDEPTH;
> @@ -592,8 +590,6 @@ static int set_pix_fmt(AVCodecContext *avctx, 
> vpx_codec_caps_t codec_caps,
>  case AV_PIX_FMT_YUV440P10:
>  case AV_PIX_FMT_YUV440P12:
>  if (codec_caps & VPX_CODEC_CAP_HIGHBITDEPTH) {
> -enccfg->g_bit_depth = enccfg->g_input_bit_depth =
> -avctx->pix_fmt == AV_PIX_FMT_YUV440P10 ? 10 : 12;
>  enccfg->g_profile = 3;
>  *img_fmt = VPX_IMG_FMT_I44016;
>  *flags |= VPX_CODEC_USE_HIGHBITDEPTH;
> @@ -606,9 +602,6 @@ static int set_pix_fmt(AVCodecContext *avctx, 
> vpx_codec_caps_t codec_caps,
>  case AV_PIX_FMT_YUV444P10:
>  case AV_PIX_FMT_YUV444P12:
>  if (codec_caps & VPX_CODEC_CAP_HIGHBITDEPTH) {
> -enccfg->g_bit_depth = enccfg->g_input_bit_depth =
> -avctx->pix_fmt == AV_PIX_FMT_YUV444P10 ||
> -avctx->pix_fmt == AV_PIX_FMT_GBRP10 ? 10 : 12;
>  enccfg->g_profile = 3;
>  *img_fmt = VPX_IMG_FMT_I44416;
>  *flags |= VPX_CODEC_USE_HIGHBITDEPTH;

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

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".