Re: [FFmpeg-devel] [PATCH] mov: Evaluate the movie display matrix

2016-10-07 Thread Vittorio Giovara 
On Fri, Oct 7, 2016 at 8:38 PM, Michael Niedermayer
 wrote:
> On Fri, Oct 07, 2016 at 03:31:46PM -0400, Vittorio Giovara wrote:
>> This matrix needs to be applied after all others have (currently only
>> display matrix from trak), but cannot be handled in movie box, since
>> streams are not allocated yet.
>>
>> So store it in main context and if not identity, apply it when appropriate,
>> handling the case when trak display matrix is identity and when it is not.
>>
>> Signed-off-by: Vittorio Giovara 
>> ---
>
>> +} else { // otherwise multiply the two and store the result
>> +int64_t val = 0;
>> +for (i = 0; i < 3; i++) {
>> +for (j = 0; j < 3; j++) {
>> +int sh = j == 2 ? 30 : 16;
>> +for (e = 0; e < 3; e++) {
>> +val += CONV_FP2INT(display_matrix[i][e], sh) *
>> +   CONV_FP2INT(c->movie_display_matrix[e][j], 
>> sh);
>
> This does not work (you are dividing the 32bit numbers down to 2 bit)

I don't understand this comment, are you referring to the fact that
the first two columns of the display matrix are 16.16, while the third
column is 2:30?

> also its not tested by the fate testcase
> i can just replace it by 0 and fate-mov-movie-display-matrix still
> passes

Yes, the sample I shared only has the movie display matrix, not
trak+movie. I can create another sample if you insist, but a fate test
would test little more than the matrix multiplication loops.

> the macros also lack some () protection giving probably unintended
> results

Can you tell me how many more () would they need?

From the subsequent email

> to explain a bit more how to do it with int64 instead of double floats
> int32 can be multiplied together to form int64 with a new fixed point
> without rounding or shifts
> then added up and then at the end shifted down with rounding to get
> back to the fixed point needed for the final destination

The problem is that the display matrix fields have different mantissa
length, the first two columns are 16.16, the last one is 2:30. I think
that method would work if all elements were the same length no?
-- 
Vittorio
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [OPW] OPW Project Proposal

2016-10-07 Thread Michael Niedermayer 
On Fri, Oct 07, 2016 at 10:49:42PM +0530, Pallavi Kumari wrote:
> >> iam interrested, i intend to mentor only one applicant though
> >> iam also the mentor for "Improve Selftest coverage"
> >> (hint  if someone wants to take mentoring that over for example ...)
> 
> Thank you for your interest :)
> Is there any applicant for ''Improve Selftest coverage'' ?

There were several applicants who contacted me and i told all to
add themselfs to
https://trac.ffmpeg.org/wiki/SponsoringPrograms/Outreachy/2016-12-Qualis
(same as previous years)
the page is still empty so iam not sure we have an applicant


> 
> >> Do you have a suggeted entry for
> >> https://trac.ffmpeg.org/wiki/SponsoringPrograms/Outreachy/2016-12
> >> for this project ?
> 
> This idea was proposed by me. I wanted to check first if it's relevant to
> community. The community has show immense interest so, I was looking for
> mentor(s). I would be happy to add this idea on the link mentioned above
> (if I have permission) or some one with the edit permission can do it.

its probably better if i add it but you can post a suggested wiki
marked up text here and ill add it to the page if it looks ok
... my lazyness is limitless

also with a project like this its important to carefuly test the
fingerprinting to ensure degraded songs can be quickly and reliably
be matched up in a large set of reference fingerprints/songs

and that changes done during the project do improve this.
It should not be implemented and after that tested that would be bad
the earlier there is something that can be tested an compared against
competing solutions the better and easier it would be to tune things
and easily try modifications ...


> 
> 
> >> also, important, we need a qualification task which should be a small
> >> part of the whole that shows that the applicant is qualified/able to
> >> do the project, any suggestion ?
> 
> A small task could be writing a module that reads a sound file extract
> sound information and frame rate also find point of interests (peak

frame rate ? you mean sample rate ?


> intensity points) and plot it on graph. Also add a unit test for this
> module.
> 
> what do you think?

that "module" can be a audio->video libavfilter
am i correct that with peaks you mean peaks in a frequency/time 2d
array ?

thx

[...]

-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus


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

Re: [FFmpeg-devel] [PATCH] mov: Evaluate the movie display matrix

2016-10-07 Thread Michael Niedermayer 
On Sat, Oct 08, 2016 at 02:38:27AM +0200, Michael Niedermayer wrote:
> On Fri, Oct 07, 2016 at 03:31:46PM -0400, Vittorio Giovara wrote:
> > This matrix needs to be applied after all others have (currently only
> > display matrix from trak), but cannot be handled in movie box, since
> > streams are not allocated yet.
> > 
> > So store it in main context and if not identity, apply it when appropriate,
> > handling the case when trak display matrix is identity and when it is not.
> > 
> > Signed-off-by: Vittorio Giovara 
> > ---
> > Updated according review.
> > Vittorio
> > 
> >  libavformat/isom.h  |  2 ++
> >  libavformat/mov.c   | 63 
> > +++--
> >  tests/fate/mov.mak  |  6 +++-
> >  tests/ref/fate/mov-movie-display-matrix | 10 ++
> >  4 files changed, 70 insertions(+), 11 deletions(-)
> >  create mode 100644 tests/ref/fate/mov-movie-display-matrix
> > 
> > diff --git a/libavformat/isom.h b/libavformat/isom.h
> > index 2246fed..2aeb8fa 100644
> > --- a/libavformat/isom.h
> > +++ b/libavformat/isom.h
> > @@ -238,6 +238,8 @@ typedef struct MOVContext {
> >  uint8_t *decryption_key;
> >  int decryption_key_len;
> >  int enable_drefs;
> > +
> > +int32_t movie_display_matrix[3][3]; ///< display matrix from mvhd
> >  } MOVContext;
> >  
> >  int ff_mp4_read_descr_len(AVIOContext *pb);
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index a15c8d1..307ce08 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -1211,6 +1211,7 @@ static int mov_read_mdhd(MOVContext *c, AVIOContext 
> > *pb, MOVAtom atom)
> >  
> >  static int mov_read_mvhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >  {
> > +int i;
> >  int64_t creation_time;
> >  int version = avio_r8(pb); /* version */
> >  avio_rb24(pb); /* flags */
> > @@ -1238,7 +1239,12 @@ static int mov_read_mvhd(MOVContext *c, AVIOContext 
> > *pb, MOVAtom atom)
> >  
> >  avio_skip(pb, 10); /* reserved */
> >  
> > -avio_skip(pb, 36); /* display matrix */
> > +/* movie display matrix, store it in main context and use it later on 
> > */
> > +for (i = 0; i < 3; i++) {
> > +c->movie_display_matrix[i][0] = avio_rb32(pb); // 16.16 fixed point
> > +c->movie_display_matrix[i][1] = avio_rb32(pb); // 16.16 fixed point
> > +c->movie_display_matrix[i][2] = avio_rb32(pb); //  2.30 fixed point
> > +}
> >  
> >  avio_rb32(pb); /* preview time */
> >  avio_rb32(pb); /* preview duration */
> > @@ -3798,9 +3804,24 @@ static int mov_read_meta(MOVContext *c, AVIOContext 
> > *pb, MOVAtom atom)
> >  return 0;
> >  }
> >  
> > +// return 0 when matrix is identity, 1 otherwise
> > +#define IS_MATRIX_FULL(matrix)   \
> > +(matrix[0][0] != (1 << 16) ||\
> > + matrix[1][1] != (1 << 16) ||\
> > + matrix[2][2] != (1 << 30) ||\
> > + matrix[0][1] || matrix[0][2] || \
> > + matrix[1][0] || matrix[1][2] || \
> > + matrix[2][0] || matrix[2][1])
> > +
> > +// fixed point to int64_t
> > +#define CONV_FP2INT(x, sh) ((int64_t) (x)) / (1 << sh)
> > +
> > +// int64_t to fixed point
> > +#define CONV_INT2FP(x, sh) (int32_t) ((x) * (1 << sh))
> > +
> >  static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> >  {
> > -int i;
> > +int i, j, e;
> >  int width;
> >  int height;
> >  int display_matrix[3][3];
> > @@ -3855,13 +3876,7 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext 
> > *pb, MOVAtom atom)
> >  
> >  // save the matrix and add rotate metadata when it is not the default
> >  // identity
> > -if (display_matrix[0][0] != (1 << 16) ||
> > -display_matrix[1][1] != (1 << 16) ||
> > -display_matrix[2][2] != (1 << 30) ||
> > -display_matrix[0][1] || display_matrix[0][2] ||
> > -display_matrix[1][0] || display_matrix[1][2] ||
> > -display_matrix[2][0] || display_matrix[2][1]) {
> > -int i, j;
> > +if (IS_MATRIX_FULL(display_matrix)) {
> >  double rotate;
> >  
> >  av_freep(>display_matrix);
> > @@ -3884,13 +3899,41 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext 
> > *pb, MOVAtom atom)
> >  }
> >  }
> >  
> > +// if movie display matrix is not identity, and if this is a video 
> > track
> > +if (IS_MATRIX_FULL(c->movie_display_matrix) && width && height) {
> > +// if trak display matrix was identity, just copy the movie one
> > +if (!sc->display_matrix) {
> > +sc->display_matrix = av_malloc(sizeof(int32_t) * 9);
> > +if (!sc->display_matrix)
> > +return AVERROR(ENOMEM);
> > +
> > +for (i = 0; i < 3; i++)
> > +for (j = 0; j < 3; j++)
> > +sc->display_matrix[i * 3 + j] = 
> > c->movie_display_matrix[i][j];
> 
> > +} else { // otherwise multiply the two and store the result
>

Re: [FFmpeg-devel] [PATCH] mov: Evaluate the movie display matrix

2016-10-07 Thread Michael Niedermayer 
On Fri, Oct 07, 2016 at 03:31:46PM -0400, Vittorio Giovara wrote:
> This matrix needs to be applied after all others have (currently only
> display matrix from trak), but cannot be handled in movie box, since
> streams are not allocated yet.
> 
> So store it in main context and if not identity, apply it when appropriate,
> handling the case when trak display matrix is identity and when it is not.
> 
> Signed-off-by: Vittorio Giovara 
> ---
> Updated according review.
> Vittorio
> 
>  libavformat/isom.h  |  2 ++
>  libavformat/mov.c   | 63 
> +++--
>  tests/fate/mov.mak  |  6 +++-
>  tests/ref/fate/mov-movie-display-matrix | 10 ++
>  4 files changed, 70 insertions(+), 11 deletions(-)
>  create mode 100644 tests/ref/fate/mov-movie-display-matrix
> 
> diff --git a/libavformat/isom.h b/libavformat/isom.h
> index 2246fed..2aeb8fa 100644
> --- a/libavformat/isom.h
> +++ b/libavformat/isom.h
> @@ -238,6 +238,8 @@ typedef struct MOVContext {
>  uint8_t *decryption_key;
>  int decryption_key_len;
>  int enable_drefs;
> +
> +int32_t movie_display_matrix[3][3]; ///< display matrix from mvhd
>  } MOVContext;
>  
>  int ff_mp4_read_descr_len(AVIOContext *pb);
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index a15c8d1..307ce08 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -1211,6 +1211,7 @@ static int mov_read_mdhd(MOVContext *c, AVIOContext 
> *pb, MOVAtom atom)
>  
>  static int mov_read_mvhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>  {
> +int i;
>  int64_t creation_time;
>  int version = avio_r8(pb); /* version */
>  avio_rb24(pb); /* flags */
> @@ -1238,7 +1239,12 @@ static int mov_read_mvhd(MOVContext *c, AVIOContext 
> *pb, MOVAtom atom)
>  
>  avio_skip(pb, 10); /* reserved */
>  
> -avio_skip(pb, 36); /* display matrix */
> +/* movie display matrix, store it in main context and use it later on */
> +for (i = 0; i < 3; i++) {
> +c->movie_display_matrix[i][0] = avio_rb32(pb); // 16.16 fixed point
> +c->movie_display_matrix[i][1] = avio_rb32(pb); // 16.16 fixed point
> +c->movie_display_matrix[i][2] = avio_rb32(pb); //  2.30 fixed point
> +}
>  
>  avio_rb32(pb); /* preview time */
>  avio_rb32(pb); /* preview duration */
> @@ -3798,9 +3804,24 @@ static int mov_read_meta(MOVContext *c, AVIOContext 
> *pb, MOVAtom atom)
>  return 0;
>  }
>  
> +// return 0 when matrix is identity, 1 otherwise
> +#define IS_MATRIX_FULL(matrix)   \
> +(matrix[0][0] != (1 << 16) ||\
> + matrix[1][1] != (1 << 16) ||\
> + matrix[2][2] != (1 << 30) ||\
> + matrix[0][1] || matrix[0][2] || \
> + matrix[1][0] || matrix[1][2] || \
> + matrix[2][0] || matrix[2][1])
> +
> +// fixed point to int64_t
> +#define CONV_FP2INT(x, sh) ((int64_t) (x)) / (1 << sh)
> +
> +// int64_t to fixed point
> +#define CONV_INT2FP(x, sh) (int32_t) ((x) * (1 << sh))
> +
>  static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
>  {
> -int i;
> +int i, j, e;
>  int width;
>  int height;
>  int display_matrix[3][3];
> @@ -3855,13 +3876,7 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext 
> *pb, MOVAtom atom)
>  
>  // save the matrix and add rotate metadata when it is not the default
>  // identity
> -if (display_matrix[0][0] != (1 << 16) ||
> -display_matrix[1][1] != (1 << 16) ||
> -display_matrix[2][2] != (1 << 30) ||
> -display_matrix[0][1] || display_matrix[0][2] ||
> -display_matrix[1][0] || display_matrix[1][2] ||
> -display_matrix[2][0] || display_matrix[2][1]) {
> -int i, j;
> +if (IS_MATRIX_FULL(display_matrix)) {
>  double rotate;
>  
>  av_freep(>display_matrix);
> @@ -3884,13 +3899,41 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext 
> *pb, MOVAtom atom)
>  }
>  }
>  
> +// if movie display matrix is not identity, and if this is a video track
> +if (IS_MATRIX_FULL(c->movie_display_matrix) && width && height) {
> +// if trak display matrix was identity, just copy the movie one
> +if (!sc->display_matrix) {
> +sc->display_matrix = av_malloc(sizeof(int32_t) * 9);
> +if (!sc->display_matrix)
> +return AVERROR(ENOMEM);
> +
> +for (i = 0; i < 3; i++)
> +for (j = 0; j < 3; j++)
> +sc->display_matrix[i * 3 + j] = 
> c->movie_display_matrix[i][j];

> +} else { // otherwise multiply the two and store the result
> +int64_t val = 0;
> +for (i = 0; i < 3; i++) {
> +for (j = 0; j < 3; j++) {
> +int sh = j == 2 ? 30 : 16;
> +for (e = 0; e < 3; e++) {
> +val += CONV_FP2INT(display_matrix[i][e], sh) *
> +

Re: [FFmpeg-devel] [PATCH] libavformat/tee: Add fifo support for tee

2016-10-07 Thread Michael Niedermayer 
On Fri, Oct 07, 2016 at 01:03:03AM +0200, sebechlebsky...@gmail.com wrote:
> From: Jan Sebechlebsky 
> 
> Signed-off-by: Jan Sebechlebsky 
> ---
> This commit makes use of fifo muxer together with tee muxer
> easier, fifo muxer does not have to be explicitly specified
> for each slave. For the most simple use case it is sufficient
> to turn fifo muxer on for all slaves by switching on use_fifo
> option. Same options can be passed to all fifo muxer instances of
> slaves by assigning them to fifo_options of tee. 
> Both use_fifo option and individual fifo_options can be 
> overriden per slave if needed.
> 
>  doc/muxers.texi   | 20 +
>  libavformat/tee.c | 89 
> ++-
>  2 files changed, 108 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/muxers.texi b/doc/muxers.texi
> index 9ec2e31..b88b83c 100644
> --- a/doc/muxers.texi
> +++ b/doc/muxers.texi
> @@ -1473,6 +1473,7 @@ Specify whether to remove all fragments when finished. 
> Default 0 (do not remove)
>  
>  @end table
>  
> +@anchor{fifo}
>  @section fifo
>  
>  The fifo pseudo-muxer allows the separation of encoding and muxing by using
> @@ -1580,6 +1581,18 @@ with the tee muxer; encoding can be a very expensive 
> process. It is not
>  useful when using the libavformat API directly because it is then possible
>  to feed the same packets to several muxers directly.
>  
> +@table @option
> +
> +@item use_fifo @var{bool}
> +If set to 1, slave outputs will be processed in separate thread using 
> @ref{fifo}
> +muxer. This allows to compensate for different speed/latency/reliability of
> +outputs and setup transparent recovery. By default this feature is turned 
> off.
> +
> +@item fifo_options
> +Options to pass to fifo pseudo-muxer instances. See @ref{fifo}.
> +
> +@end table
> +
>  The slave outputs are specified in the file name given to the muxer,
>  separated by '|'. If any of the slave name contains the '|' separator,
>  leading or trailing spaces or any special character, it must be
> @@ -1601,6 +1614,13 @@ output name suffix.
>  Specify a list of bitstream filters to apply to the specified
>  output.
>  
> +@item use_fifo @var{bool}
> +This allows to override tee muxer use_fifo option for individual slave muxer.
> +
> +@item fifo_options
> +This allows to override tee muxer fifo_options for individual slave muxer.
> +See @ref{fifo}.
> +
>  It is possible to specify to which streams a given bitstream filter
>  applies, by appending a stream specifier to the option separated by
>  @code{/}. @var{spec} must be a stream specifier (see @ref{Format
> diff --git a/libavformat/tee.c b/libavformat/tee.c
> index d59ad4d..764135d 100644
> --- a/libavformat/tee.c
> +++ b/libavformat/tee.c
> @@ -40,6 +40,8 @@ typedef struct {
>  AVBSFContext **bsfs; ///< bitstream filters per stream
>  
>  SlaveFailurePolicy on_fail;
> +int use_fifo;
> +AVDictionary *fifo_options;
>  
>  /** map from input to output streams indexes,
>   * disabled output streams are set to -1 */
> @@ -52,15 +54,28 @@ typedef struct TeeContext {
>  unsigned nb_slaves;
>  unsigned nb_alive;
>  TeeSlave *slaves;
> +int use_fifo;
> +AVDictionary *fifo_options;
> +char *fifo_options_str;
>  } TeeContext;
>  
>  static const char *const slave_delim = "|";
>  static const char *const slave_bsfs_spec_sep = "/";
>  static const char *const slave_select_sep = ",";
>  
> +#define OFFSET(x) offsetof(TeeContext, x)
> +static const AVOption options[] = {
> +{"use_fifo", "Use fifo pseudo-muxer to separate actual muxers from 
> encoder",
> + OFFSET(use_fifo), AV_OPT_TYPE_BOOL, {.i64 = 0}, 0, 1, 
> AV_OPT_FLAG_ENCODING_PARAM},
> +{"fifo_options", "fifo pseudo-muxer options", 
> OFFSET(fifo_options_str),
> + AV_OPT_TYPE_STRING, {.str = NULL}, 0, 0, 
> AV_OPT_FLAG_ENCODING_PARAM},
> +{NULL}
> +};
> +
>  static const AVClass tee_muxer_class = {
>  .class_name = "Tee muxer",
>  .item_name  = av_default_item_name,
> +.option = options,
>  .version= LIBAVUTIL_VERSION_INT,
>  };
>  
> @@ -81,6 +96,27 @@ static inline int parse_slave_failure_policy_option(const 
> char *opt, TeeSlave *t
>  return AVERROR(EINVAL);
>  }
>  
> +static int parse_slave_fifo_options(const char * use_fifo,
> +const char * fifo_options, TeeSlave * 
> tee_slave)
> +{
> +int ret = 0;
> +
> +if (use_fifo) {
> +if (av_match_name(use_fifo, "true,y,yes,enable,enabled,on,1")) {
> +tee_slave->use_fifo = 1;
> +} else if (av_match_name(use_fifo, 
> "false,n,no,disable,disabled,off,0")) {
> +tee_slave->use_fifo = 0;
> +} else {
> +return AVERROR(EINVAL);
> +}
> +}
> +
> +if (fifo_options)
> +ret = av_dict_parse_string(_slave->fifo_options, fifo_options, 
> "=", ":", 0);
> +
> +return ret;
> +}
>

Re: [FFmpeg-devel] [PATCH] lavfi/metadata: fix metadata deletion if comparison returns false

2016-10-07 Thread Michael Niedermayer 
On Thu, Oct 06, 2016 at 11:49:06PM +0200, Marton Balint wrote:
> Signed-off-by: Marton Balint 
> ---
>  libavfilter/f_metadata.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

should be ok if paul has no comment/time

thx

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

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch


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

Re: [FFmpeg-devel] [PATCH 1/2] avformat/matroskaenc: fix Tags master on seekable output if there are tags after the last stream duration

2016-10-07 Thread James Almer 
On 10/7/2016 3:29 PM, Dave Rice wrote:
> 
>> On Oct 7, 2016, at 10:30 AM, James Almer  wrote:
>>
>> On 10/7/2016 3:05 AM, James Almer wrote:
>>> The dynamic AVIOContext would get closed pointing to the wrong position
>>> in the buffer.
>>> This is a regression since 650e17d88b63b5aca6e0a43483e89e64b0f7d2dd.
>>>
>>> Signed-off-by: James Almer 
>>> ---
>>> Example:
>>> ./ffmpeg -i fate-samples/vorbis/vorbis_chapter_extension_demo.ogg -c:a copy 
>>> -metadata:c key=value out.mka
>>>
>>> The output is corrupt before this patch.
>>>
>>> The refactoring isn't necessary per se, but it comes in handy for the next 
>>> patch,
>>> where attachments will not be treated as tracks anymore when writting tags.
>>>
>>> libavformat/matroskaenc.c | 15 +--
>>> 1 file changed, 9 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>>> index 0878cb5..286b8b4 100644
>>> --- a/libavformat/matroskaenc.c
>>> +++ b/libavformat/matroskaenc.c
>>> @@ -2264,17 +2264,19 @@ static int mkv_write_trailer(AVFormatContext *s)
>>> end_ebml_master_crc32(pb, >info_bc, mkv, mkv->info);
>>>
>>> // update stream durations
>>> -if (mkv->stream_durations) {
>>> +if (!mkv->is_live && mkv->stream_durations) {
>>> int i;
>>> +int64_t curr = avio_tell(mkv->tags_bc);
>>> for (i = 0; i < s->nb_streams; ++i) {
>>> AVStream *st = s->streams[i];
>>> -double duration_sec = mkv->stream_durations[i] * 
>>> av_q2d(st->time_base);
>>> -char duration_string[20] = "";
>>>
>>> -av_log(s, AV_LOG_DEBUG, "stream %d end duration = %" 
>>> PRIu64 "\n", i,
>>> -   mkv->stream_durations[i]);
>>> +if (mkv->stream_duration_offsets[i] > 0) {
>>> +double duration_sec = mkv->stream_durations[i] * 
>>> av_q2d(st->time_base);
>>> +char duration_string[20] = "";
>>> +
>>> +av_log(s, AV_LOG_DEBUG, "stream %d end duration = %" 
>>> PRIu64 "\n", i,
>>> +   mkv->stream_durations[i]);
>>>
>>> -if (!mkv->is_live && mkv->stream_duration_offsets[i] > 0) {
>>> avio_seek(mkv->tags_bc, 
>>> mkv->stream_duration_offsets[i], SEEK_SET);
>>>
>>> snprintf(duration_string, 20, "%02d:%02d:%012.9f",
>>> @@ -2284,6 +2286,7 @@ static int mkv_write_trailer(AVFormatContext *s)
>>> put_ebml_binary(mkv->tags_bc, MATROSKA_ID_TAGSTRING, 
>>> duration_string, 20);
>>> }
>>> }
>>> +avio_seek(mkv->tags_bc, curr, SEEK_SET);
>>> }
>>> if (mkv->tags.pos && !mkv->is_live) {
>>> avio_seek(pb, mkv->tags.pos, SEEK_SET);
>>
>>
>> Ping. It's a recent regression on a muxer, so I'd like to solve it asap.
>> I'll push it later today if nobody comments.
> 
> I tested this and it does fix the issue so that the added "key" tag is 
> properly formed, whereas the tag is only null bytes without the patch. Thanks!
> Dave Rice

Pushed, thanks.

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

[FFmpeg-devel] [PATCH] mov: Evaluate the movie display matrix

2016-10-07 Thread Vittorio Giovara 
This matrix needs to be applied after all others have (currently only
display matrix from trak), but cannot be handled in movie box, since
streams are not allocated yet.

So store it in main context and if not identity, apply it when appropriate,
handling the case when trak display matrix is identity and when it is not.

Signed-off-by: Vittorio Giovara 
---
Updated according review.
Vittorio

 libavformat/isom.h  |  2 ++
 libavformat/mov.c   | 63 +++--
 tests/fate/mov.mak  |  6 +++-
 tests/ref/fate/mov-movie-display-matrix | 10 ++
 4 files changed, 70 insertions(+), 11 deletions(-)
 create mode 100644 tests/ref/fate/mov-movie-display-matrix

diff --git a/libavformat/isom.h b/libavformat/isom.h
index 2246fed..2aeb8fa 100644
--- a/libavformat/isom.h
+++ b/libavformat/isom.h
@@ -238,6 +238,8 @@ typedef struct MOVContext {
 uint8_t *decryption_key;
 int decryption_key_len;
 int enable_drefs;
+
+int32_t movie_display_matrix[3][3]; ///< display matrix from mvhd
 } MOVContext;
 
 int ff_mp4_read_descr_len(AVIOContext *pb);
diff --git a/libavformat/mov.c b/libavformat/mov.c
index a15c8d1..307ce08 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -1211,6 +1211,7 @@ static int mov_read_mdhd(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 static int mov_read_mvhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
+int i;
 int64_t creation_time;
 int version = avio_r8(pb); /* version */
 avio_rb24(pb); /* flags */
@@ -1238,7 +1239,12 @@ static int mov_read_mvhd(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 avio_skip(pb, 10); /* reserved */
 
-avio_skip(pb, 36); /* display matrix */
+/* movie display matrix, store it in main context and use it later on */
+for (i = 0; i < 3; i++) {
+c->movie_display_matrix[i][0] = avio_rb32(pb); // 16.16 fixed point
+c->movie_display_matrix[i][1] = avio_rb32(pb); // 16.16 fixed point
+c->movie_display_matrix[i][2] = avio_rb32(pb); //  2.30 fixed point
+}
 
 avio_rb32(pb); /* preview time */
 avio_rb32(pb); /* preview duration */
@@ -3798,9 +3804,24 @@ static int mov_read_meta(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 return 0;
 }
 
+// return 0 when matrix is identity, 1 otherwise
+#define IS_MATRIX_FULL(matrix)   \
+(matrix[0][0] != (1 << 16) ||\
+ matrix[1][1] != (1 << 16) ||\
+ matrix[2][2] != (1 << 30) ||\
+ matrix[0][1] || matrix[0][2] || \
+ matrix[1][0] || matrix[1][2] || \
+ matrix[2][0] || matrix[2][1])
+
+// fixed point to int64_t
+#define CONV_FP2INT(x, sh) ((int64_t) (x)) / (1 << sh)
+
+// int64_t to fixed point
+#define CONV_INT2FP(x, sh) (int32_t) ((x) * (1 << sh))
+
 static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
 {
-int i;
+int i, j, e;
 int width;
 int height;
 int display_matrix[3][3];
@@ -3855,13 +3876,7 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, 
MOVAtom atom)
 
 // save the matrix and add rotate metadata when it is not the default
 // identity
-if (display_matrix[0][0] != (1 << 16) ||
-display_matrix[1][1] != (1 << 16) ||
-display_matrix[2][2] != (1 << 30) ||
-display_matrix[0][1] || display_matrix[0][2] ||
-display_matrix[1][0] || display_matrix[1][2] ||
-display_matrix[2][0] || display_matrix[2][1]) {
-int i, j;
+if (IS_MATRIX_FULL(display_matrix)) {
 double rotate;
 
 av_freep(>display_matrix);
@@ -3884,13 +3899,41 @@ static int mov_read_tkhd(MOVContext *c, AVIOContext 
*pb, MOVAtom atom)
 }
 }
 
+// if movie display matrix is not identity, and if this is a video track
+if (IS_MATRIX_FULL(c->movie_display_matrix) && width && height) {
+// if trak display matrix was identity, just copy the movie one
+if (!sc->display_matrix) {
+sc->display_matrix = av_malloc(sizeof(int32_t) * 9);
+if (!sc->display_matrix)
+return AVERROR(ENOMEM);
+
+for (i = 0; i < 3; i++)
+for (j = 0; j < 3; j++)
+sc->display_matrix[i * 3 + j] = 
c->movie_display_matrix[i][j];
+} else { // otherwise multiply the two and store the result
+int64_t val = 0;
+for (i = 0; i < 3; i++) {
+for (j = 0; j < 3; j++) {
+int sh = j == 2 ? 30 : 16;
+for (e = 0; e < 3; e++) {
+val += CONV_FP2INT(display_matrix[i][e], sh) *
+   CONV_FP2INT(c->movie_display_matrix[e][j], sh);
+}
+sc->display_matrix[i * 3 + j] = CONV_INT2FP(val, sh);
+val = 0;
+}
+}
+}
+}
+
 // transform the display width/height according to the matrix
 // to keep the same scale,

Re: [FFmpeg-devel] [PATCH 1/2] avformat/matroskaenc: fix Tags master on seekable output if there are tags after the last stream duration

2016-10-07 Thread Dave Rice 

> On Oct 7, 2016, at 10:30 AM, James Almer  wrote:
> 
> On 10/7/2016 3:05 AM, James Almer wrote:
>> The dynamic AVIOContext would get closed pointing to the wrong position
>> in the buffer.
>> This is a regression since 650e17d88b63b5aca6e0a43483e89e64b0f7d2dd.
>> 
>> Signed-off-by: James Almer 
>> ---
>> Example:
>> ./ffmpeg -i fate-samples/vorbis/vorbis_chapter_extension_demo.ogg -c:a copy 
>> -metadata:c key=value out.mka
>> 
>> The output is corrupt before this patch.
>> 
>> The refactoring isn't necessary per se, but it comes in handy for the next 
>> patch,
>> where attachments will not be treated as tracks anymore when writting tags.
>> 
>> libavformat/matroskaenc.c | 15 +--
>> 1 file changed, 9 insertions(+), 6 deletions(-)
>> 
>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>> index 0878cb5..286b8b4 100644
>> --- a/libavformat/matroskaenc.c
>> +++ b/libavformat/matroskaenc.c
>> @@ -2264,17 +2264,19 @@ static int mkv_write_trailer(AVFormatContext *s)
>> end_ebml_master_crc32(pb, >info_bc, mkv, mkv->info);
>> 
>> // update stream durations
>> -if (mkv->stream_durations) {
>> +if (!mkv->is_live && mkv->stream_durations) {
>> int i;
>> +int64_t curr = avio_tell(mkv->tags_bc);
>> for (i = 0; i < s->nb_streams; ++i) {
>> AVStream *st = s->streams[i];
>> -double duration_sec = mkv->stream_durations[i] * 
>> av_q2d(st->time_base);
>> -char duration_string[20] = "";
>> 
>> -av_log(s, AV_LOG_DEBUG, "stream %d end duration = %" PRIu64 
>> "\n", i,
>> -   mkv->stream_durations[i]);
>> +if (mkv->stream_duration_offsets[i] > 0) {
>> +double duration_sec = mkv->stream_durations[i] * 
>> av_q2d(st->time_base);
>> +char duration_string[20] = "";
>> +
>> +av_log(s, AV_LOG_DEBUG, "stream %d end duration = %" 
>> PRIu64 "\n", i,
>> +   mkv->stream_durations[i]);
>> 
>> -if (!mkv->is_live && mkv->stream_duration_offsets[i] > 0) {
>> avio_seek(mkv->tags_bc, mkv->stream_duration_offsets[i], 
>> SEEK_SET);
>> 
>> snprintf(duration_string, 20, "%02d:%02d:%012.9f",
>> @@ -2284,6 +2286,7 @@ static int mkv_write_trailer(AVFormatContext *s)
>> put_ebml_binary(mkv->tags_bc, MATROSKA_ID_TAGSTRING, 
>> duration_string, 20);
>> }
>> }
>> +avio_seek(mkv->tags_bc, curr, SEEK_SET);
>> }
>> if (mkv->tags.pos && !mkv->is_live) {
>> avio_seek(pb, mkv->tags.pos, SEEK_SET);
> 
> 
> Ping. It's a recent regression on a muxer, so I'd like to solve it asap.
> I'll push it later today if nobody comments.

I tested this and it does fix the issue so that the added "key" tag is properly 
formed, whereas the tag is only null bytes without the patch. Thanks!
Dave Rice


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

Re: [FFmpeg-devel] [OPW] OPW Project Proposal

2016-10-07 Thread Pallavi Kumari 
>> iam interrested, i intend to mentor only one applicant though
>> iam also the mentor for "Improve Selftest coverage"
>> (hint  if someone wants to take mentoring that over for example ...)

Thank you for your interest :)
Is there any applicant for ''Improve Selftest coverage'' ?

>> Do you have a suggeted entry for
>> https://trac.ffmpeg.org/wiki/SponsoringPrograms/Outreachy/2016-12
>> for this project ?

This idea was proposed by me. I wanted to check first if it's relevant to
community. The community has show immense interest so, I was looking for
mentor(s). I would be happy to add this idea on the link mentioned above
(if I have permission) or some one with the edit permission can do it.


>> also, important, we need a qualification task which should be a small
>> part of the whole that shows that the applicant is qualified/able to
>> do the project, any suggestion ?

A small task could be writing a module that reads a sound file extract
sound information and frame rate also find point of interests (peak
intensity points) and plot it on graph. Also add a unit test for this
module.

what do you think?


Thanks and regards,
Pallavi


On Fri, Oct 7, 2016 at 7:14 AM, Michael Niedermayer 
wrote:

> On Wed, Oct 05, 2016 at 11:23:11PM +0530, Pallavi Kumari wrote:
> > I am looking for mentor(s). Kindly let me know if anyone interested.
>
> iam interrested, i intend to mentor only one applicant though
> iam also the mentor for "Improve Selftest coverage"
> (hint  if someone wants to take mentoring that over for example ...)
>
> Do you have a suggeted entry for
> https://trac.ffmpeg.org/wiki/SponsoringPrograms/Outreachy/2016-12
> for this project ?
>
> also, important, we need a qualification task which should be a small
> part of the whole that shows that the applicant is qualified/able to
> do the project, any suggestion ?
>
>
> thx
>
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Let us carefully observe those good qualities wherein our enemies excel us
> and endeavor to excel them, by avoiding what is faulty, and imitating what
> is excellent in them. -- Plutarch
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] avformat/matroskaenc: fix Tags master on seekable output if there are tags after the last stream duration

2016-10-07 Thread James Almer 
On 10/7/2016 3:05 AM, James Almer wrote:
> The dynamic AVIOContext would get closed pointing to the wrong position
> in the buffer.
> This is a regression since 650e17d88b63b5aca6e0a43483e89e64b0f7d2dd.
> 
> Signed-off-by: James Almer 
> ---
> Example:
> ./ffmpeg -i fate-samples/vorbis/vorbis_chapter_extension_demo.ogg -c:a copy 
> -metadata:c key=value out.mka
> 
> The output is corrupt before this patch.
> 
> The refactoring isn't necessary per se, but it comes in handy for the next 
> patch,
> where attachments will not be treated as tracks anymore when writting tags.
> 
>  libavformat/matroskaenc.c | 15 +--
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 0878cb5..286b8b4 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -2264,17 +2264,19 @@ static int mkv_write_trailer(AVFormatContext *s)
>  end_ebml_master_crc32(pb, >info_bc, mkv, mkv->info);
>  
>  // update stream durations
> -if (mkv->stream_durations) {
> +if (!mkv->is_live && mkv->stream_durations) {
>  int i;
> +int64_t curr = avio_tell(mkv->tags_bc);
>  for (i = 0; i < s->nb_streams; ++i) {
>  AVStream *st = s->streams[i];
> -double duration_sec = mkv->stream_durations[i] * 
> av_q2d(st->time_base);
> -char duration_string[20] = "";
>  
> -av_log(s, AV_LOG_DEBUG, "stream %d end duration = %" PRIu64 
> "\n", i,
> -   mkv->stream_durations[i]);
> +if (mkv->stream_duration_offsets[i] > 0) {
> +double duration_sec = mkv->stream_durations[i] * 
> av_q2d(st->time_base);
> +char duration_string[20] = "";
> +
> +av_log(s, AV_LOG_DEBUG, "stream %d end duration = %" 
> PRIu64 "\n", i,
> +   mkv->stream_durations[i]);
>  
> -if (!mkv->is_live && mkv->stream_duration_offsets[i] > 0) {
>  avio_seek(mkv->tags_bc, mkv->stream_duration_offsets[i], 
> SEEK_SET);
>  
>  snprintf(duration_string, 20, "%02d:%02d:%012.9f",
> @@ -2284,6 +2286,7 @@ static int mkv_write_trailer(AVFormatContext *s)
>  put_ebml_binary(mkv->tags_bc, MATROSKA_ID_TAGSTRING, 
> duration_string, 20);
>  }
>  }
> +avio_seek(mkv->tags_bc, curr, SEEK_SET);
>  }
>  if (mkv->tags.pos && !mkv->is_live) {
>  avio_seek(pb, mkv->tags.pos, SEEK_SET);


Ping. It's a recent regression on a muxer, so I'd like to solve it asap.
I'll push it later today if nobody comments.

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

Re: [FFmpeg-devel] [PATCH] ffmpeg: explicitly write headers for files with no streams

2016-10-07 Thread Michael Niedermayer 
On Fri, Oct 07, 2016 at 03:46:14PM +0200, Hendrik Leppkes wrote:
> Recent changes to ffmpeg.c tied output file init to stream init, which broke
> stream-less files, specifically ffmetadata output.
> ---
>  ffmpeg.c | 10 ++
>  1 file changed, 10 insertions(+)

LGTM

thx

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

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin


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

Re: [FFmpeg-devel] [PATCH] mov: Evaluate the movie display matrix

2016-10-07 Thread Carl Eugen Hoyos 
2016-10-06 22:40 GMT+02:00 Vittorio Giovara :
>>> https://www.dropbox.com/s/w2uu37o11rvoz1q/moviedispmat.mp4?dl=1

>> what is the intended / correct SAR / DAR for this sample ?
>
> without the patch Stream #0:0(und): Video: h264 (High) (avc1 /
> 0x31637661), yuv420p, 540x576 [SAR 1:1 DAR 15:16], 102 kb/s, 25 fps,
> 25 tbr, 12800 tbn, 50 tbc (default)
>
> with the patch Stream #0:0(und): Video: h264 (High) (avc1 /
> 0x31637661), yuv420p, 540x576 [SAR 1:1 DAR 15:16], 102 kb/s, SAR
> 93207:65536 DAR 1016804:762601, 25 fps, 25 tbr, 12800 tbn, 50 tbc
> (default)
>
> This last one is the intended and correct one.

Which application (except patched FFmpeg) allows to reproduce this?

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

Re: [FFmpeg-devel] [PATCH] ffmpeg: explicitly write headers for files with no streams

2016-10-07 Thread Hendrik Leppkes 
On Fri, Oct 7, 2016 at 3:46 PM, Hendrik Leppkes  wrote:
> Recent changes to ffmpeg.c tied output file init to stream init, which broke
> stream-less files, specifically ffmetadata output.
> ---
>  ffmpeg.c | 10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/ffmpeg.c b/ffmpeg.c
> index 454e193..b3e23ef 100644
> --- a/ffmpeg.c
> +++ b/ffmpeg.c
> @@ -3460,6 +3460,16 @@ static int transcode_init(void)
>  }
>  }
>
> +/* write headers for files with no streams */
> +for (i = 0; i < nb_output_files; i++) {
> +oc = output_files[i]->ctx;
> +if (oc->oformat->flags & AVFMT_NOSTREAMS) {

Locally added a check here that nb_streams is actually 0, because
AVFMT_NOSTREAMS doesn't forbid having streams, just allows zero.
Once it has a single stream, its initialized with the stream.

> +ret = check_init_output_file(output_files[i], i);
> +if (ret < 0)
> +goto dump_format;
> +}
> +}
> +
>   dump_format:
>  /* dump the stream mapping */
>  av_log(NULL, AV_LOG_INFO, "Stream mapping:\n");
> --
> 2.10.0.windows.1
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH] ffmpeg: explicitly write headers for files with no streams

2016-10-07 Thread Hendrik Leppkes 
Recent changes to ffmpeg.c tied output file init to stream init, which broke
stream-less files, specifically ffmetadata output.
---
 ffmpeg.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/ffmpeg.c b/ffmpeg.c
index 454e193..b3e23ef 100644
--- a/ffmpeg.c
+++ b/ffmpeg.c
@@ -3460,6 +3460,16 @@ static int transcode_init(void)
 }
 }
 
+/* write headers for files with no streams */
+for (i = 0; i < nb_output_files; i++) {
+oc = output_files[i]->ctx;
+if (oc->oformat->flags & AVFMT_NOSTREAMS) {
+ret = check_init_output_file(output_files[i], i);
+if (ret < 0)
+goto dump_format;
+}
+}
+
  dump_format:
 /* dump the stream mapping */
 av_log(NULL, AV_LOG_INFO, "Stream mapping:\n");
-- 
2.10.0.windows.1

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

Re: [FFmpeg-devel] [PATCH] ffmpeg: remove unused and errorneous AVFrame timestamp check

2016-10-07 Thread Hendrik Leppkes 
On Thu, Oct 6, 2016 at 4:08 PM, Hendrik Leppkes  wrote:
> On Tue, Oct 4, 2016 at 4:44 PM, James Almer  wrote:
>> On 10/4/2016 11:35 AM, Hendrik Leppkes wrote:
>>> On Tue, Oct 4, 2016 at 4:32 PM, wm4  wrote:
 On Tue, 4 Oct 2016 14:15:03 +0200
 Michael Niedermayer  wrote:

> On Tue, Oct 04, 2016 at 01:52:02PM +0200, Hendrik Leppkes wrote:
>> On Tue, Oct 4, 2016 at 1:44 PM, Hendrik Leppkes  
>> wrote:
>>> On Tue, Oct 4, 2016 at 1:23 PM, Michael Niedermayer
>>>  wrote:
 On Tue, Oct 04, 2016 at 08:41:42AM +0200, Hendrik Leppkes wrote:
> On Tue, Oct 4, 2016 at 4:05 AM, Michael Niedermayer
>  wrote:
>> On Sat, Oct 01, 2016 at 04:15:45PM +0200, Hendrik Leppkes wrote:
>>> Decoders have previously not used AVFrame.pts, and with the upcoming
>>> deprecation of pkt_pts (in favor of pts), this would lead to an 
>>> errorneous
>>> interpration of timestamps.
>>
>> I probably misunderstand the commit message but
>> If code is changed in a user application that cannot really lift
>> some blockage from changing a lib.
>> a lib can only change in an incompaible way with (deprecation and)
>> major version bump.
>>
>
> The lib never did what this code suggests it did, not that I remember
> (so at least not for a long long time).

 release/2.0 with

 diff --git a/libavcodec/utils.c b/libavcodec/utils.c
 index 29d5492..57c8d50 100644
 --- a/libavcodec/utils.c
 +++ b/libavcodec/utils.c
 @@ -2008,7 +2008,7 @@ int attribute_align_arg 
 avcodec_decode_video2(AVCodecContext *avctx, AVFrame *pi
  avci->to_free.extended_data = avci->to_free.data;
  memset(picture->buf, 0, sizeof(picture->buf));
  }
 -
 +av_assert0(picture->pts == 0 || picture->pts == AV_NOPTS_VALUE);
  avctx->frame_number++;
  av_frame_set_best_effort_timestamp(picture,
 
 guess_correct_pts(avctx,

 causes many tests to fail, indicating that AVFrame.pts was set for
 several video decoders, the ffmpeg code is audio decoder specific
 and i failed to find a case where it was triggered, i tried IIRC 3
 or so checkouts from the past

 so AVFrame.pts was maybe never set for decoding audio but it was set
 for video
>>>
>>> Can you extend the test to add "|| picture->pts == picture->pkt_pts"?
>>> Because thats what it would be set to after the merge. A quick check
>>> in the 2.0 code base looks like some decoders did set that, but to the
>>> exact same value as pkt_pts (which is what the merge is proposing
>>> right now as well)
>>>
>>
>> And I found this (after 2.0):
>> http://git.videolan.org/?p=ffmpeg.git;a=commitdiff;h=a1c5cc429d99216406170eac7e8352860076d3e8
>>
>> Which apparently set pts for mpeg4 to a number parsed from the
>> bitstream, entirely uncorrelated to container or audio timestamps, so
>> using them would have been rather impractical for any real use-cases.
>
> They can be usfull, some random examples:
>
> playing MPEG4-ES with timing stored from the bitstream (assuming there
>   is no demuxer lib used that is capable to extract them)
>
> forensics, raw video stream could have its timing
>   recovered, a video with manipulated container timestamps could be
>   detected.
>
> error correction, if the container level timestamps are lost or
>   corrupted the stream level ones can be used to recreate them
>
> There may be more, these are just some of the top of my head,
> not your mainstream multimedia player stuff maybe but they arent
> useless
>
> [...]
>

 They don't belong into the AVFrame.pts field, though.
>>>
>>> And they don't go in there anymore right now, so thats that.
>>>
>>> The real question is, what do we do about this merge now?
>>> Can we set AVFrame.pts to the same value as AVFrame.pkt_pts safely,
>>> considering it was unused in the current ABI/API, or would that be
>>> considered an API break and we better delay this change until the next
>>> major?
>>>
>>> - Hendrik
>>
>> Delaying it could result in further merges becoming technically wrong,
>> or at least require extra manual changes for them to not misbehave in
>> our tree.
>>
>> IMO merge it now, and if needed/preferred, we could make sure it
>> doesn't make it to 3.2
>>
>
> Last call for any actual and clear objections to going forward with
> this route. I would like to get merging a bunch over the weekend

[FFmpeg-devel] [PATCH 2/2 v2] avformat/matroskaenc: fix targets for attachment tags

2016-10-07 Thread James Almer 
Attachment tags were being written targetting non-existent streams in the
output file.
Also filter filename and mimetype entries, as they are standard elements
in the Attachment master.

Signed-off-by: James Almer 
---
Now rebased after the CRC32 patchset.

The fileuid is changed to be four bytes long instead of eight, because the
mkv_write_tag*() functions pass that value as unsigned int.
If preferred i can change them to pass uint64_t and keep the current huge
fileuids.

 libavformat/matroskaenc.c | 66 ++-
 tests/ref/lavf/mkv|  4 +--
 2 files changed, 61 insertions(+), 9 deletions(-)

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index 286b8b4..61baeb2 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -96,6 +96,16 @@ typedef struct mkv_track {
 int64_t ts_offset;
 } mkv_track;
 
+typedef struct mkv_attachment {
+int stream_idx;
+uint32_tfileuid;
+} mkv_attachment;
+
+typedef struct mkv_attachments {
+mkv_attachment  *entries;
+int num_entries;
+} mkv_attachments;
+
 #define MODE_MATROSKAv2 0x01
 #define MODE_WEBM   0x02
 
@@ -121,6 +131,7 @@ typedef struct MatroskaMuxContext {
 mkv_seekhead*main_seekhead;
 mkv_cues*cues;
 mkv_track   *tracks;
+mkv_attachments *attachments;
 
 AVPacketcur_audio_pkt;
 
@@ -368,6 +379,10 @@ static void mkv_free(MatroskaMuxContext *mkv) {
 av_freep(>cues->entries);
 av_freep(>cues);
 }
+if (mkv->attachments) {
+av_freep(>attachments->entries);
+av_freep(>attachments);
+}
 av_freep(>tracks);
 av_freep(>stream_durations);
 av_freep(>stream_duration_offsets);
@@ -1393,7 +1408,10 @@ static int mkv_check_tag_name(const char *name, unsigned 
int elementid)
av_strcasecmp(name, "encoding_tool") &&
av_strcasecmp(name, "duration") &&
(elementid != MATROSKA_ID_TAGTARGETS_TRACKUID ||
-av_strcasecmp(name, "language"));
+av_strcasecmp(name, "language")) &&
+   (elementid != MATROSKA_ID_TAGTARGETS_ATTACHUID ||
+(av_strcasecmp(name, "filename") &&
+ av_strcasecmp(name, "mimetype")));
 }
 
 static int mkv_write_tag(AVFormatContext *s, AVDictionary *m, unsigned int 
elementid,
@@ -1446,6 +1464,9 @@ static int mkv_write_tags(AVFormatContext *s)
 for (i = 0; i < s->nb_streams; i++) {
 AVStream *st = s->streams[i];
 
+if (st->codecpar->codec_type == AVMEDIA_TYPE_ATTACHMENT)
+continue;
+
 if (!mkv_check_tag(st->metadata, MATROSKA_ID_TAGTARGETS_TRACKUID))
 continue;
 
@@ -1456,9 +1477,13 @@ static int mkv_write_tags(AVFormatContext *s)
 if (s->pb->seekable && !mkv->is_live) {
 for (i = 0; i < s->nb_streams; i++) {
 AVIOContext *pb;
+AVStream *st = s->streams[i];
 ebml_master tag_target;
 ebml_master tag;
 
+if (st->codecpar->codec_type == AVMEDIA_TYPE_ATTACHMENT)
+continue;
+
 mkv_write_tag_targets(s, MATROSKA_ID_TAGTARGETS_TRACKUID, i + 1, 
>tags, _target);
 pb = mkv->tags_bc;
 
@@ -1484,6 +1509,20 @@ static int mkv_write_tags(AVFormatContext *s)
 if (ret < 0) return ret;
 }
 
+if (mkv->have_attachments) {
+for (i = 0; i < mkv->attachments->num_entries; i++) {
+mkv_attachment *attachment = >attachments->entries[i];
+AVStream *st = s->streams[attachment->stream_idx];
+
+if (!mkv_check_tag(st->metadata, MATROSKA_ID_TAGTARGETS_ATTACHUID))
+continue;
+
+ret = mkv_write_tag(s, st->metadata, 
MATROSKA_ID_TAGTARGETS_ATTACHUID, attachment->fileuid, >tags);
+if (ret < 0)
+return ret;
+}
+}
+
 if (mkv->tags.pos) {
 if (s->pb->seekable && !mkv->is_live)
 put_ebml_void(s->pb, avio_tell(mkv->tags_bc) + ((mkv->write_crc && 
mkv->mode != MODE_WEBM) ? 2 /* ebml id + data size */ + 4 /* CRC32 */ : 0));
@@ -1504,6 +1543,10 @@ static int mkv_write_attachments(AVFormatContext *s)
 if (!mkv->have_attachments)
 return 0;
 
+mkv->attachments = av_mallocz(sizeof(*mkv->attachments));
+if (!mkv->attachments)
+return ret;
+
 av_lfg_init(, av_get_random_seed());
 
 ret = mkv_add_seekhead_entry(mkv->main_seekhead, MATROSKA_ID_ATTACHMENTS, 
avio_tell(pb));
@@ -1515,13 +1558,19 @@ static int mkv_write_attachments(AVFormatContext *s)
 for (i = 0; i < s->nb_streams; i++) {
 AVStream *st = s->streams[i];
 ebml_master attached_file;
+mkv_attachment *attachment = mkv->attachments->entries;
 AVDictionaryEntry *t;
 const char *mimetype = NULL;
-uint64_t fileuid;
+uint32_t fileuid;
 
 if (st->codecpar->codec_type != AVMEDIA_TYPE_ATTACHMENT)