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

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] 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] 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