Re: [libav-devel] [PATCH] matroskadec: don't warn about unknown spherical medata when none is present

2017-11-02 Thread Sean McGovern 
On Thu, Nov 2, 2017 at 8:36 PM, James Almer  wrote:
> On 11/2/2017 5:39 PM, Sean McGovern wrote:
>> Hi,
>>
>> On Nov 2, 2017 16:32, "James Almer"  wrote:
>>
>> On 11/2/2017 5:12 PM, Sean McGovern wrote:
>>> Hi James,
>>>
>>> On Nov 2, 2017 10:03, "James Almer"  wrote:
>>>
>>> track->video.projection.type is 0 by default, and is the value set by the
>>> demuxer for files without the element.
>>>
>>> Signed-off-by: James Almer 
>>> ---
>>>  libavformat/matroskadec.c | 3 ---
>>>  1 file changed, 3 deletions(-)
>>>
>>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>>> index c6e1a190a8..5ed03bb642 100644
>>> --- a/libavformat/matroskadec.c
>>> +++ b/libavformat/matroskadec.c
>>> @@ -1659,9 +1659,6 @@ static int mkv_parse_video_projection(AVStream *st,
>>> const MatroskaTrack *track)
>>>  }
>>>  break;
>>>  default:
>>> -av_log(NULL, AV_LOG_WARNING,
>>> -   "Unknown spherical metadata type %"PRIu64"\n",
>>> -   track->video.projection.type);
>>>  return 0;
>>>  }
>>>
>>> --
>>> 2.14.2
>>>
>>> ___
>>> libav-devel mailing list
>>> libav-devel@libav.org
>>> https://lists.libav.org/mailman/listinfo/libav-devel
>>>
>>>
>>> Er... I'm not sure this is a better than what I had (with which I
>> agree
>>> on your review point). Isn't this log message potentially useful for
>>> corrupted streams?
>>
>> That's a good reason to add a "do nothing" case for
>> MATROSKA_VIDEO_PROJECTION_TYPE_RECTANGULAR (aka, none), which is the
>> default value that the demuxer will fill for every single mkv file it
>> parses, and keep keep the warning for default:.
>>
>>>
>>> Also please note that the sample from BZ #1055 currently registers as
>>> projection type 15 which maps to the _NB. Pretty strange for a sample
>>> hailing from 2008. I have a feeling the real bug is elsewhere...
>>
>> Not projection type, stereomode type 15. And in that case then the file
>> is corrupt, and it should perhaps be handled in ff_mkv_stereo3d_conv()
>> or similar.
>>
>>>
>>> -- Sean McGovern
>>> ___
>>> libav-devel mailing list
>>> libav-devel@libav.org
>>> https://lists.libav.org/mailman/listinfo/libav-devel
>>>
>>
>> ___
>> libav-devel mailing list
>> libav-devel@libav.org
>> https://lists.libav.org/mailman/listinfo/libav-devel
>>
>>
>>
>> Sorry yes I meant stereomode 15, and I suspect that is a bug from
>> ff_mkv_stereo3d_conv(). I can't agree to call that sample corrupt however
>> as mkvtoolnix iterates it just fine.
>
> Right, so the file doesn't have a StereoMode element at all, and the _NB
> value is simply set as default by the demuxer.
> In any case, all the StereoMode checks effectively make sure that
> track->video.stereo_mode is < _NB, so there is no bug there.
>
> I already sent a new version to silence the Spherical log message on
> files with no Spherical metadata, so that should be enough.
> ___
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel


Fixes the bogus log message, and LGTM.

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

Re: [libav-devel] [PATCH] matroskadec: don't warn about unknown spherical medata when none is present

2017-11-02 Thread James Almer 
On 11/2/2017 5:39 PM, Sean McGovern wrote:
> Hi,
> 
> On Nov 2, 2017 16:32, "James Almer"  wrote:
> 
> On 11/2/2017 5:12 PM, Sean McGovern wrote:
>> Hi James,
>>
>> On Nov 2, 2017 10:03, "James Almer"  wrote:
>>
>> track->video.projection.type is 0 by default, and is the value set by the
>> demuxer for files without the element.
>>
>> Signed-off-by: James Almer 
>> ---
>>  libavformat/matroskadec.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>> index c6e1a190a8..5ed03bb642 100644
>> --- a/libavformat/matroskadec.c
>> +++ b/libavformat/matroskadec.c
>> @@ -1659,9 +1659,6 @@ static int mkv_parse_video_projection(AVStream *st,
>> const MatroskaTrack *track)
>>  }
>>  break;
>>  default:
>> -av_log(NULL, AV_LOG_WARNING,
>> -   "Unknown spherical metadata type %"PRIu64"\n",
>> -   track->video.projection.type);
>>  return 0;
>>  }
>>
>> --
>> 2.14.2
>>
>> ___
>> libav-devel mailing list
>> libav-devel@libav.org
>> https://lists.libav.org/mailman/listinfo/libav-devel
>>
>>
>> Er... I'm not sure this is a better than what I had (with which I
> agree
>> on your review point). Isn't this log message potentially useful for
>> corrupted streams?
> 
> That's a good reason to add a "do nothing" case for
> MATROSKA_VIDEO_PROJECTION_TYPE_RECTANGULAR (aka, none), which is the
> default value that the demuxer will fill for every single mkv file it
> parses, and keep keep the warning for default:.
> 
>>
>> Also please note that the sample from BZ #1055 currently registers as
>> projection type 15 which maps to the _NB. Pretty strange for a sample
>> hailing from 2008. I have a feeling the real bug is elsewhere...
> 
> Not projection type, stereomode type 15. And in that case then the file
> is corrupt, and it should perhaps be handled in ff_mkv_stereo3d_conv()
> or similar.
> 
>>
>> -- Sean McGovern
>> ___
>> libav-devel mailing list
>> libav-devel@libav.org
>> https://lists.libav.org/mailman/listinfo/libav-devel
>>
> 
> ___
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel
> 
> 
> 
> Sorry yes I meant stereomode 15, and I suspect that is a bug from
> ff_mkv_stereo3d_conv(). I can't agree to call that sample corrupt however
> as mkvtoolnix iterates it just fine.

Right, so the file doesn't have a StereoMode element at all, and the _NB
value is simply set as default by the demuxer.
In any case, all the StereoMode checks effectively make sure that
track->video.stereo_mode is < _NB, so there is no bug there.

I already sent a new version to silence the Spherical log message on
files with no Spherical metadata, so that should be enough.
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

[libav-devel] [PATCH v2] matroskadec: don't warn about unknown spherical medata when none is present

2017-11-02 Thread James Almer 
track->video.projection.type is set to 0 (a Matroska specific "No spherical
metadata present" value, with no related AVSphericalMapping) by default on
files without the element.

This removes bogus warnings on every single matroska file without Spherical
metadata.

Signed-off-by: James Almer 
---
 libavformat/matroskadec.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index c6e1a190a8..3953cd304e 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -1658,6 +1658,9 @@ static int mkv_parse_video_projection(AVStream *st, const 
MatroskaTrack *track)
 return AVERROR_INVALIDDATA;
 }
 break;
+case MATROSKA_VIDEO_PROJECTION_TYPE_RECTANGULAR:
+/* No Spherical metadata */
+return 0;
 default:
 av_log(NULL, AV_LOG_WARNING,
"Unknown spherical metadata type %"PRIu64"\n",
-- 
2.14.2

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

Re: [libav-devel] [PATCH] matroskadec: don't warn about unknown spherical medata when none is present

2017-11-02 Thread Sean McGovern 
Hi,

On Nov 2, 2017 16:32, "James Almer"  wrote:

On 11/2/2017 5:12 PM, Sean McGovern wrote:
> Hi James,
>
> On Nov 2, 2017 10:03, "James Almer"  wrote:
>
> track->video.projection.type is 0 by default, and is the value set by the
> demuxer for files without the element.
>
> Signed-off-by: James Almer 
> ---
>  libavformat/matroskadec.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index c6e1a190a8..5ed03bb642 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -1659,9 +1659,6 @@ static int mkv_parse_video_projection(AVStream *st,
> const MatroskaTrack *track)
>  }
>  break;
>  default:
> -av_log(NULL, AV_LOG_WARNING,
> -   "Unknown spherical metadata type %"PRIu64"\n",
> -   track->video.projection.type);
>  return 0;
>  }
>
> --
> 2.14.2
>
> ___
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel
>
>
> Er... I'm not sure this is a better than what I had (with which I
agree
> on your review point). Isn't this log message potentially useful for
> corrupted streams?

That's a good reason to add a "do nothing" case for
MATROSKA_VIDEO_PROJECTION_TYPE_RECTANGULAR (aka, none), which is the
default value that the demuxer will fill for every single mkv file it
parses, and keep keep the warning for default:.

>
> Also please note that the sample from BZ #1055 currently registers as
> projection type 15 which maps to the _NB. Pretty strange for a sample
> hailing from 2008. I have a feeling the real bug is elsewhere...

Not projection type, stereomode type 15. And in that case then the file
is corrupt, and it should perhaps be handled in ff_mkv_stereo3d_conv()
or similar.

>
> -- Sean McGovern
> ___
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel
>

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



Sorry yes I meant stereomode 15, and I suspect that is a bug from
ff_mkv_stereo3d_conv(). I can't agree to call that sample corrupt however
as mkvtoolnix iterates it just fine.

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

Re: [libav-devel] [PATCH] matroskadec: don't warn about unknown spherical medata when none is present

2017-11-02 Thread James Almer 
On 11/2/2017 5:12 PM, Sean McGovern wrote:
> Hi James,
> 
> On Nov 2, 2017 10:03, "James Almer"  wrote:
> 
> track->video.projection.type is 0 by default, and is the value set by the
> demuxer for files without the element.
> 
> Signed-off-by: James Almer 
> ---
>  libavformat/matroskadec.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index c6e1a190a8..5ed03bb642 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -1659,9 +1659,6 @@ static int mkv_parse_video_projection(AVStream *st,
> const MatroskaTrack *track)
>  }
>  break;
>  default:
> -av_log(NULL, AV_LOG_WARNING,
> -   "Unknown spherical metadata type %"PRIu64"\n",
> -   track->video.projection.type);
>  return 0;
>  }
> 
> --
> 2.14.2
> 
> ___
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel
> 
> 
> Er... I'm not sure this is a better than what I had (with which I agree
> on your review point). Isn't this log message potentially useful for
> corrupted streams?

That's a good reason to add a "do nothing" case for
MATROSKA_VIDEO_PROJECTION_TYPE_RECTANGULAR (aka, none), which is the
default value that the demuxer will fill for every single mkv file it
parses, and keep keep the warning for default:.

> 
> Also please note that the sample from BZ #1055 currently registers as
> projection type 15 which maps to the _NB. Pretty strange for a sample
> hailing from 2008. I have a feeling the real bug is elsewhere...

Not projection type, stereomode type 15. And in that case then the file
is corrupt, and it should perhaps be handled in ff_mkv_stereo3d_conv()
or similar.

> 
> -- Sean McGovern
> ___
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel
> 

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

Re: [libav-devel] [PATCH 3/3] matroskadec: allow flavor 0 files to contain RealAudio streams

2017-11-02 Thread Sean McGovern 
Hi,

On Nov 2, 2017 07:30, "Martin Storsjö"  wrote:

On Thu, 2 Nov 2017, Sean McGovern wrote:

Hi,
>
> On Nov 2, 2017 02:50, "Martin Storsjö"  wrote:
>
> On Wed, 1 Nov 2017, Sean McGovern wrote:
>
> Regression since 569d18aa9dc989c37bb4d4b968026fe5afa6fff9.
>
>>
>> Bug-Id: 1055
>> Cc: libav-sta...@libav.org
>> ---
>> libavformat/matroskadec.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>> index 9c523fb..bad750c 100644
>> --- a/libavformat/matroskadec.c
>> +++ b/libavformat/matroskadec.c
>> @@ -1919,7 +1919,7 @@ static int matroska_parse_tracks(AVFormatContext
>> *s)
>> track->audio.sub_packet_h= avio_rb16();
>> track->audio.frame_size  = avio_rb16();
>> track->audio.sub_packet_size = avio_rb16();
>> -if (flavor   <= 0 ||
>> +if (flavor   < 0 ||
>> track->audio.coded_framesize <= 0 ||
>> track->audio.sub_packet_h<= 0 ||
>> track->audio.frame_size  <= 0 ||
>> --
>> 2.7.4
>>
>>
> Possibly ok (the commit message of the offending commit doesn't indicate
> which sample it was about so I can't easily check whether that sample
> needed to check for strictly flavor <= 0 or not).
>
> // Martin
> ___
> libav-devel mailing list
> libav-devel@libav.org
> https://lists.libav.org/mailman/listinfo/libav-devel
>
>
> I was probably half-asleep when I wrote the commit message for this. It
> should really be something like:
>
> matroskadec: allow RealAudio/Cook/Sipro streams of flavor 0
>
> ...Or something similar (suggestions?)
>
> The sample used to fix this issue is from BZ #268 [1].
>

What I meant was; the original commit,
569d18aa9dc989c37bb4d4b968026fe5afa6fff9,
tried to fix some crash with some fuzzed input files. We should ideally
make sure those crashes stay fixed. Since I didn't write out which sample
it was about, it's not very easy to find out now which one it might have
been though, to verify we don't reopen that crash bug.


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


I hope this is still OK -- the other sanity checks in that if clause are
left untouched.

Thanks,
-- Sean McGovern
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] matroskadec: don't warn about unknown spherical medata when none is present

2017-11-02 Thread Sean McGovern 
Hi James,

On Nov 2, 2017 10:03, "James Almer"  wrote:

track->video.projection.type is 0 by default, and is the value set by the
demuxer for files without the element.

Signed-off-by: James Almer 
---
 libavformat/matroskadec.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index c6e1a190a8..5ed03bb642 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -1659,9 +1659,6 @@ static int mkv_parse_video_projection(AVStream *st,
const MatroskaTrack *track)
 }
 break;
 default:
-av_log(NULL, AV_LOG_WARNING,
-   "Unknown spherical metadata type %"PRIu64"\n",
-   track->video.projection.type);
 return 0;
 }

--
2.14.2

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


Er... I'm not sure this is a better than what I had (with which I agree
on your review point). Isn't this log message potentially useful for
corrupted streams?

Also please note that the sample from BZ #1055 currently registers as
projection type 15 which maps to the _NB. Pretty strange for a sample
hailing from 2008. I have a feeling the real bug is elsewhere...

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

Re: [libav-devel] [PATCH 2/3] avformat/matroskadec: Call matroska_read_close() on header parsing error

2017-11-02 Thread Sean McGovern 
Hi Vittorio,


On Nov 2, 2017 11:19 AM, "Vittorio Giovara" 
wrote:

On Wed, Nov 1, 2017 at 10:10 PM, Sean McGovern  wrote:

> From: Michael Niedermayer 
>
> Fixes memleak
>

nit full stop

Fixes Ticket5169
>

Please use the syntax "Bug-Id: ffmpeg/Ticket5169".


> Signed-off-by: Michael Niedermayer 
> ---
>  libavformat/matroskadec.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 3743d4d..9c523fb 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -592,6 +592,8 @@ static EbmlSyntax matroska_clusters_incremental[] = {
>
>  static const char *const matroska_doctypes[] = { "matroska", "webm" };
>
> +static int matroska_read_close(AVFormatContext *s);
> +
>

if possible there should be a separate patch that moves matroska_read_close
here, rather than introducing a forward statement.


>  static int matroska_resync(MatroskaDemuxContext *matroska, int64_t
> last_pos)
>  {
>  AVIOContext *pb = matroska->ctx->pb;
> @@ -2099,7 +2101,7 @@ static int matroska_read_header(AVFormatContext *s)
>  while (res != 1) {
>  res = matroska_resync(matroska, pos);
>  if (res < 0)
> -return res;
> +goto fail;
>  pos = avio_tell(matroska->ctx->pb);
>  res = ebml_parse(matroska, matroska_segment, matroska);
>  }
> @@ -2114,7 +2116,7 @@ static int matroska_read_header(AVFormatContext *s)
>
>  res = matroska_parse_tracks(s);
>  if (res < 0)
> -return res;
> +goto fail;
>
>  attachments = attachments_list->elem;
>  for (j = 0; j < attachments_list->nb_elem; j++) {
> @@ -2187,6 +2189,9 @@ static int matroska_read_header(AVFormatContext *s)
>  matroska_convert_tags(s);
>
>  return 0;
> +fail:
> +matroska_read_close(s);
> +return res;
>  }
>
> --
Vittorio
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel


OK, agree with your comments and will re-submit this later.

Thanks for the review,
Sean McGovern
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 2/3] avformat/matroskadec: Call matroska_read_close() on header parsing error

2017-11-02 Thread Vittorio Giovara 
On Wed, Nov 1, 2017 at 10:10 PM, Sean McGovern  wrote:

> From: Michael Niedermayer 
>
> Fixes memleak
>

nit full stop

Fixes Ticket5169
>

Please use the syntax "Bug-Id: ffmpeg/Ticket5169".


> Signed-off-by: Michael Niedermayer 
> ---
>  libavformat/matroskadec.c | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 3743d4d..9c523fb 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -592,6 +592,8 @@ static EbmlSyntax matroska_clusters_incremental[] = {
>
>  static const char *const matroska_doctypes[] = { "matroska", "webm" };
>
> +static int matroska_read_close(AVFormatContext *s);
> +
>

if possible there should be a separate patch that moves matroska_read_close
here, rather than introducing a forward statement.


>  static int matroska_resync(MatroskaDemuxContext *matroska, int64_t
> last_pos)
>  {
>  AVIOContext *pb = matroska->ctx->pb;
> @@ -2099,7 +2101,7 @@ static int matroska_read_header(AVFormatContext *s)
>  while (res != 1) {
>  res = matroska_resync(matroska, pos);
>  if (res < 0)
> -return res;
> +goto fail;
>  pos = avio_tell(matroska->ctx->pb);
>  res = ebml_parse(matroska, matroska_segment, matroska);
>  }
> @@ -2114,7 +2116,7 @@ static int matroska_read_header(AVFormatContext *s)
>
>  res = matroska_parse_tracks(s);
>  if (res < 0)
> -return res;
> +goto fail;
>
>  attachments = attachments_list->elem;
>  for (j = 0; j < attachments_list->nb_elem; j++) {
> @@ -2187,6 +2189,9 @@ static int matroska_read_header(AVFormatContext *s)
>  matroska_convert_tags(s);
>
>  return 0;
> +fail:
> +matroska_read_close(s);
> +return res;
>  }
>
> --
Vittorio
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] matroskadec: don't warn about unknown spherical medata when none is present

2017-11-02 Thread James Almer 
On 11/2/2017 12:01 PM, Luca Barbato wrote:
> On 02/11/2017 15:03, James Almer wrote:
>> track->video.projection.type is 0 by default, and is the value set by the
>> demuxer for files without the element.
>>
>> Signed-off-by: James Almer 
>> ---
>>   libavformat/matroskadec.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>> index c6e1a190a8..5ed03bb642 100644
>> --- a/libavformat/matroskadec.c
>> +++ b/libavformat/matroskadec.c
>> @@ -1659,9 +1659,6 @@ static int mkv_parse_video_projection(AVStream
>> *st, const MatroskaTrack *track)
>>   }
>>   break;
>>   default:
>> -    av_log(NULL, AV_LOG_WARNING,
>> -   "Unknown spherical metadata type %"PRIu64"\n",
>> -   track->video.projection.type);
>>   return 0;
>>   }
>>  
> 
> not sure if would be overkill add a
> 
> 0: return 0; // nothing to do
> 
> or such.

I don't think that's a good idea. There are projections we don't support
which need to be handled by default:.
Unless of course you prefer handling actual projection values we don't
support and 0 ("rectangular" as called by Mastroska, which is the same
as none) in a different way.
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] matroskadec: don't warn about unknown spherical medata when none is present

2017-11-02 Thread Luca Barbato 

On 02/11/2017 15:03, James Almer wrote:

track->video.projection.type is 0 by default, and is the value set by the
demuxer for files without the element.

Signed-off-by: James Almer 
---
  libavformat/matroskadec.c | 3 ---
  1 file changed, 3 deletions(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index c6e1a190a8..5ed03bb642 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -1659,9 +1659,6 @@ static int mkv_parse_video_projection(AVStream *st, const 
MatroskaTrack *track)
  }
  break;
  default:
-av_log(NULL, AV_LOG_WARNING,
-   "Unknown spherical metadata type %"PRIu64"\n",
-   track->video.projection.type);
  return 0;
  }
  



not sure if would be overkill add a

0: return 0; // nothing to do

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

[libav-devel] [PATCH] matroskadec: don't warn about unknown spherical medata when none is present

2017-11-02 Thread James Almer 
track->video.projection.type is 0 by default, and is the value set by the
demuxer for files without the element.

Signed-off-by: James Almer 
---
 libavformat/matroskadec.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index c6e1a190a8..5ed03bb642 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -1659,9 +1659,6 @@ static int mkv_parse_video_projection(AVStream *st, const 
MatroskaTrack *track)
 }
 break;
 default:
-av_log(NULL, AV_LOG_WARNING,
-   "Unknown spherical metadata type %"PRIu64"\n",
-   track->video.projection.type);
 return 0;
 }
 
-- 
2.14.2

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

Re: [libav-devel] [PATCH 1/3] matroskadec: skip video projection parsing for non-spherical video streams

2017-11-02 Thread James Almer 
On 11/1/2017 11:10 PM, Sean McGovern wrote:
> ---
>  libavformat/matroskadec.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index c6e1a19..3743d4d 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -1606,6 +1606,10 @@ static int mkv_parse_video_projection(AVStream *st, 
> const MatroskaTrack *track)
>  int ret;
>  GetByteContext gb;
>  
> +if (track->video.stereo_mode == MATROSKA_VIDEO_STEREOMODE_TYPE_NB) {
> + return 0;
> +}
> +
>  bytestream2_init(, track->video.projection.private.data,
>   track->video.projection.private.size);

This is not correct. As Hendrik said stereo mode has nothing to do with
Spherical. You can have elements from one but not the other.

The problem you're seeing ("Unknown spherical metadata type 0" warning)
is because track->video.projection.type == 0 is the default and set by
the demuxer for every file without spherical elements. The demuxer
should not emit a warning for that value and silently return instead.

I'll send a patch for this in a moment.
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 3/3] matroskadec: allow flavor 0 files to contain RealAudio streams

2017-11-02 Thread Martin Storsjö 

On Thu, 2 Nov 2017, Sean McGovern wrote:


Hi,

On Nov 2, 2017 02:50, "Martin Storsjö"  wrote:

On Wed, 1 Nov 2017, Sean McGovern wrote:

Regression since 569d18aa9dc989c37bb4d4b968026fe5afa6fff9.


Bug-Id: 1055
Cc: libav-sta...@libav.org
---
libavformat/matroskadec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 9c523fb..bad750c 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -1919,7 +1919,7 @@ static int matroska_parse_tracks(AVFormatContext *s)
track->audio.sub_packet_h= avio_rb16();
track->audio.frame_size  = avio_rb16();
track->audio.sub_packet_size = avio_rb16();
-if (flavor   <= 0 ||
+if (flavor   < 0 ||
track->audio.coded_framesize <= 0 ||
track->audio.sub_packet_h<= 0 ||
track->audio.frame_size  <= 0 ||
--
2.7.4



Possibly ok (the commit message of the offending commit doesn't indicate
which sample it was about so I can't easily check whether that sample
needed to check for strictly flavor <= 0 or not).

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


I was probably half-asleep when I wrote the commit message for this. It
should really be something like:

matroskadec: allow RealAudio/Cook/Sipro streams of flavor 0

...Or something similar (suggestions?)

The sample used to fix this issue is from BZ #268 [1].


What I meant was; the original commit, 
569d18aa9dc989c37bb4d4b968026fe5afa6fff9, tried to fix some crash with 
some fuzzed input files. We should ideally make sure those crashes stay 
fixed. Since I didn't write out which sample it was about, it's not very 
easy to find out now which one it might have been though, to verify we 
don't reopen that crash bug.


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

Re: [libav-devel] [PATCH 1/3] matroskadec: skip video projection parsing for non-spherical video streams

2017-11-02 Thread Sean McGovern 
Hi Hendrik,

On Nov 2, 2017 4:21 AM, "Hendrik Leppkes"  wrote:

On Thu, Nov 2, 2017 at 3:10 AM, Sean McGovern  wrote:
> ---
>  libavformat/matroskadec.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index c6e1a19..3743d4d 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -1606,6 +1606,10 @@ static int mkv_parse_video_projection(AVStream
*st, const MatroskaTrack *track)
>  int ret;
>  GetByteContext gb;
>
> +if (track->video.stereo_mode == MATROSKA_VIDEO_STEREOMODE_TYPE_NB) {
> +   return 0;
> +}
> +

Stereo-mode seems a bit unrelated to the projection, additionally
comparing to the _NB constant seems wrong, it should never be that
value.


The sample file in question predates both stereo mode and spherical video
streams and yet currently it triggers the log warning:

Unknown spherical metadata type 0

from the default: clause of the switch statement further down in this
function.

Thinking about this it probably shouldn't even be calling this function at
all in this situation. Will look at it again and propose a new patch.

Please consider this one dropped for now.


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


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

Re: [libav-devel] [PATCH 4/5] configure: Miscellaneous minor changes to config file handling

2017-11-02 Thread Diego Biurrun 
On Thu, Nov 02, 2017 at 12:00:39AM +0200, Martin Storsjö wrote:
> On Thu, 26 Oct 2017, Diego Biurrun wrote:
> 
> > - Move generating config.fate to a more sensible place.
> > - Move printing warnings to a more appropriate place.
> > - Improve "generated by" comment in libavutil/avconfig.h.
> > - Drop pointless informative output about generating config files.
> > - Write a standard comment header to config.asm as well.
> > ---
> > configure | 24 
> > 1 file changed, 12 insertions(+), 12 deletions(-)
> 
> Probably ok, assuming that the new location for printing warnings isn't so
> far up that it might get missed.

The warnings appear in the exact same place of the configure output. Only
the internal position of the code that prints the warnings has changed.

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

Re: [libav-devel] [PATCH 3/3] matroskadec: allow flavor 0 files to contain RealAudio streams

2017-11-02 Thread Sean McGovern 
Hi,

On Nov 2, 2017 02:50, "Martin Storsjö"  wrote:

On Wed, 1 Nov 2017, Sean McGovern wrote:

Regression since 569d18aa9dc989c37bb4d4b968026fe5afa6fff9.
>
> Bug-Id: 1055
> Cc: libav-sta...@libav.org
> ---
> libavformat/matroskadec.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index 9c523fb..bad750c 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -1919,7 +1919,7 @@ static int matroska_parse_tracks(AVFormatContext *s)
> track->audio.sub_packet_h= avio_rb16();
> track->audio.frame_size  = avio_rb16();
> track->audio.sub_packet_size = avio_rb16();
> -if (flavor   <= 0 ||
> +if (flavor   < 0 ||
> track->audio.coded_framesize <= 0 ||
> track->audio.sub_packet_h<= 0 ||
> track->audio.frame_size  <= 0 ||
> --
> 2.7.4
>

Possibly ok (the commit message of the offending commit doesn't indicate
which sample it was about so I can't easily check whether that sample
needed to check for strictly flavor <= 0 or not).

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


I was probably half-asleep when I wrote the commit message for this. It
should really be something like:

matroskadec: allow RealAudio/Cook/Sipro streams of flavor 0

...Or something similar (suggestions?)

The sample used to fix this issue is from BZ #268 [1].

-- Sean McG.

[1] https://bugzilla.libav.org/show_bug.cgi?id=268
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 1/3] matroskadec: skip video projection parsing for non-spherical video streams

2017-11-02 Thread Hendrik Leppkes 
On Thu, Nov 2, 2017 at 3:10 AM, Sean McGovern  wrote:
> ---
>  libavformat/matroskadec.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
> index c6e1a19..3743d4d 100644
> --- a/libavformat/matroskadec.c
> +++ b/libavformat/matroskadec.c
> @@ -1606,6 +1606,10 @@ static int mkv_parse_video_projection(AVStream *st, 
> const MatroskaTrack *track)
>  int ret;
>  GetByteContext gb;
>
> +if (track->video.stereo_mode == MATROSKA_VIDEO_STEREOMODE_TYPE_NB) {
> +   return 0;
> +}
> +

Stereo-mode seems a bit unrelated to the projection, additionally
comparing to the _NB constant seems wrong, it should never be that
value.
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH 3/3] matroskadec: allow flavor 0 files to contain RealAudio streams

2017-11-02 Thread Martin Storsjö 

On Wed, 1 Nov 2017, Sean McGovern wrote:


Regression since 569d18aa9dc989c37bb4d4b968026fe5afa6fff9.

Bug-Id: 1055
Cc: libav-sta...@libav.org
---
libavformat/matroskadec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 9c523fb..bad750c 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -1919,7 +1919,7 @@ static int matroska_parse_tracks(AVFormatContext *s)
track->audio.sub_packet_h= avio_rb16();
track->audio.frame_size  = avio_rb16();
track->audio.sub_packet_size = avio_rb16();
-if (flavor   <= 0 ||
+if (flavor   < 0 ||
track->audio.coded_framesize <= 0 ||
track->audio.sub_packet_h<= 0 ||
track->audio.frame_size  <= 0 ||
--
2.7.4


Possibly ok (the commit message of the offending commit doesn't indicate 
which sample it was about so I can't easily check whether that sample 
needed to check for strictly flavor <= 0 or not).


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