diff mbox

[FFmpeg-devel] lavf/latmenc: Error out for invalid codecs

Message ID CAB0OVGqB-eruhN9RZV6=WRDhzF5sGHXdH9C9jcfihZmjp-fdZw@mail.gmail.com
State Superseded
Headers show

Commit Message

Carl Eugen Hoyos Oct. 30, 2017, 10:51 p.m. UTC
Hi!

Attached patch makes sure the loas muxer does not try to write
anything but aac and latm.

Please comment, Carl Eugen

Comments

Michael Niedermayer Oct. 31, 2017, 4:38 p.m. UTC | #1
On Mon, Oct 30, 2017 at 11:51:30PM +0100, Carl Eugen Hoyos wrote:
> Hi!
> 
> Attached patch makes sure the loas muxer does not try to write
> anything but aac and latm.
> 
> Please comment, Carl Eugen

>  latmenc.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 2b64f3d5ecb189e77b85dbab7a6cbfe9657701f2  0001-lavf-latmenc-Error-out-for-invalid-codecs.patch
> From 9f8f39b402f77b53613a395129f96feee5e873ba Mon Sep 17 00:00:00 2001
> From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
> Date: Mon, 30 Oct 2017 23:49:29 +0100
> Subject: [PATCH] lavf/latmenc: Error out for invalid codecs.

isnt AV_CODEC_ID_MP4ALS supported too ? (i see ALS related code in
latmenc.c)

[...]
Paul B Mahol Nov. 1, 2017, 9:44 a.m. UTC | #2
On 10/31/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
> On Mon, Oct 30, 2017 at 11:51:30PM +0100, Carl Eugen Hoyos wrote:
>> Hi!
>>
>> Attached patch makes sure the loas muxer does not try to write
>> anything but aac and latm.
>>
>> Please comment, Carl Eugen
>
>>  latmenc.c |    4 ++++
>>  1 file changed, 4 insertions(+)
>> 2b64f3d5ecb189e77b85dbab7a6cbfe9657701f2
>> 0001-lavf-latmenc-Error-out-for-invalid-codecs.patch
>> From 9f8f39b402f77b53613a395129f96feee5e873ba Mon Sep 17 00:00:00 2001
>> From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
>> Date: Mon, 30 Oct 2017 23:49:29 +0100
>> Subject: [PATCH] lavf/latmenc: Error out for invalid codecs.
>
> isnt AV_CODEC_ID_MP4ALS supported too ? (i see ALS related code in
> latmenc.c)

How, when demuxer doesn't support it?
Hendrik Leppkes Nov. 1, 2017, 10:04 a.m. UTC | #3
On Wed, Nov 1, 2017 at 10:44 AM, Paul B Mahol <onemda@gmail.com> wrote:
> On 10/31/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
>> On Mon, Oct 30, 2017 at 11:51:30PM +0100, Carl Eugen Hoyos wrote:
>>> Hi!
>>>
>>> Attached patch makes sure the loas muxer does not try to write
>>> anything but aac and latm.
>>>
>>> Please comment, Carl Eugen
>>
>>>  latmenc.c |    4 ++++
>>>  1 file changed, 4 insertions(+)
>>> 2b64f3d5ecb189e77b85dbab7a6cbfe9657701f2
>>> 0001-lavf-latmenc-Error-out-for-invalid-codecs.patch
>>> From 9f8f39b402f77b53613a395129f96feee5e873ba Mon Sep 17 00:00:00 2001
>>> From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
>>> Date: Mon, 30 Oct 2017 23:49:29 +0100
>>> Subject: [PATCH] lavf/latmenc: Error out for invalid codecs.
>>
>> isnt AV_CODEC_ID_MP4ALS supported too ? (i see ALS related code in
>> latmenc.c)
>
> How, when demuxer doesn't support it?

Isn't ALS basically an extension of AAC? It would make sense that it
can also go into LATM/LAOS.

- Hendrik
Paul B Mahol Nov. 1, 2017, 10:09 a.m. UTC | #4
On 11/1/17, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
> On Wed, Nov 1, 2017 at 10:44 AM, Paul B Mahol <onemda@gmail.com> wrote:
>> On 10/31/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
>>> On Mon, Oct 30, 2017 at 11:51:30PM +0100, Carl Eugen Hoyos wrote:
>>>> Hi!
>>>>
>>>> Attached patch makes sure the loas muxer does not try to write
>>>> anything but aac and latm.
>>>>
>>>> Please comment, Carl Eugen
>>>
>>>>  latmenc.c |    4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>> 2b64f3d5ecb189e77b85dbab7a6cbfe9657701f2
>>>> 0001-lavf-latmenc-Error-out-for-invalid-codecs.patch
>>>> From 9f8f39b402f77b53613a395129f96feee5e873ba Mon Sep 17 00:00:00 2001
>>>> From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
>>>> Date: Mon, 30 Oct 2017 23:49:29 +0100
>>>> Subject: [PATCH] lavf/latmenc: Error out for invalid codecs.
>>>
>>> isnt AV_CODEC_ID_MP4ALS supported too ? (i see ALS related code in
>>> latmenc.c)
>>
>> How, when demuxer doesn't support it?
>
> Isn't ALS basically an extension of AAC? It would make sense that it
> can also go into LATM/LAOS.

But do we actually supports it? There is no ALS encoder, and only way to trigger
that is via -c copy.
Hendrik Leppkes Nov. 1, 2017, 12:22 p.m. UTC | #5
On Wed, Nov 1, 2017 at 11:09 AM, Paul B Mahol <onemda@gmail.com> wrote:
> On 11/1/17, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>> On Wed, Nov 1, 2017 at 10:44 AM, Paul B Mahol <onemda@gmail.com> wrote:
>>> On 10/31/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
>>>> On Mon, Oct 30, 2017 at 11:51:30PM +0100, Carl Eugen Hoyos wrote:
>>>>> Hi!
>>>>>
>>>>> Attached patch makes sure the loas muxer does not try to write
>>>>> anything but aac and latm.
>>>>>
>>>>> Please comment, Carl Eugen
>>>>
>>>>>  latmenc.c |    4 ++++
>>>>>  1 file changed, 4 insertions(+)
>>>>> 2b64f3d5ecb189e77b85dbab7a6cbfe9657701f2
>>>>> 0001-lavf-latmenc-Error-out-for-invalid-codecs.patch
>>>>> From 9f8f39b402f77b53613a395129f96feee5e873ba Mon Sep 17 00:00:00 2001
>>>>> From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
>>>>> Date: Mon, 30 Oct 2017 23:49:29 +0100
>>>>> Subject: [PATCH] lavf/latmenc: Error out for invalid codecs.
>>>>
>>>> isnt AV_CODEC_ID_MP4ALS supported too ? (i see ALS related code in
>>>> latmenc.c)
>>>
>>> How, when demuxer doesn't support it?
>>
>> Isn't ALS basically an extension of AAC? It would make sense that it
>> can also go into LATM/LAOS.
>
> But do we actually supports it? There is no ALS encoder, and only way to trigger
> that is via -c copy.

And thats a problem why?

- Hendrik
Paul B Mahol Nov. 1, 2017, 12:32 p.m. UTC | #6
On 11/1/17, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
> On Wed, Nov 1, 2017 at 11:09 AM, Paul B Mahol <onemda@gmail.com> wrote:
>> On 11/1/17, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>>> On Wed, Nov 1, 2017 at 10:44 AM, Paul B Mahol <onemda@gmail.com> wrote:
>>>> On 10/31/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
>>>>> On Mon, Oct 30, 2017 at 11:51:30PM +0100, Carl Eugen Hoyos wrote:
>>>>>> Hi!
>>>>>>
>>>>>> Attached patch makes sure the loas muxer does not try to write
>>>>>> anything but aac and latm.
>>>>>>
>>>>>> Please comment, Carl Eugen
>>>>>
>>>>>>  latmenc.c |    4 ++++
>>>>>>  1 file changed, 4 insertions(+)
>>>>>> 2b64f3d5ecb189e77b85dbab7a6cbfe9657701f2
>>>>>> 0001-lavf-latmenc-Error-out-for-invalid-codecs.patch
>>>>>> From 9f8f39b402f77b53613a395129f96feee5e873ba Mon Sep 17 00:00:00 2001
>>>>>> From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
>>>>>> Date: Mon, 30 Oct 2017 23:49:29 +0100
>>>>>> Subject: [PATCH] lavf/latmenc: Error out for invalid codecs.
>>>>>
>>>>> isnt AV_CODEC_ID_MP4ALS supported too ? (i see ALS related code in
>>>>> latmenc.c)
>>>>
>>>> How, when demuxer doesn't support it?
>>>
>>> Isn't ALS basically an extension of AAC? It would make sense that it
>>> can also go into LATM/LAOS.
>>
>> But do we actually supports it? There is no ALS encoder, and only way to
>> trigger
>> that is via -c copy.
>
> And thats a problem why?

Untested path.
Carl Eugen Hoyos Nov. 1, 2017, 1:30 p.m. UTC | #7
2017-11-01 13:32 GMT+01:00 Paul B Mahol <onemda@gmail.com>:
> On 11/1/17, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>> On Wed, Nov 1, 2017 at 11:09 AM, Paul B Mahol <onemda@gmail.com> wrote:
>>> On 11/1/17, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>>>> On Wed, Nov 1, 2017 at 10:44 AM, Paul B Mahol <onemda@gmail.com> wrote:
>>>>> On 10/31/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
>>>>>> On Mon, Oct 30, 2017 at 11:51:30PM +0100, Carl Eugen Hoyos wrote:
>>>>>>> Hi!
>>>>>>>
>>>>>>> Attached patch makes sure the loas muxer does not try to write
>>>>>>> anything but aac and latm.
>>>>>>>
>>>>>>> Please comment, Carl Eugen
>>>>>>
>>>>>>>  latmenc.c |    4 ++++
>>>>>>>  1 file changed, 4 insertions(+)
>>>>>>> 2b64f3d5ecb189e77b85dbab7a6cbfe9657701f2
>>>>>>> 0001-lavf-latmenc-Error-out-for-invalid-codecs.patch
>>>>>>> From 9f8f39b402f77b53613a395129f96feee5e873ba Mon Sep 17 00:00:00 2001
>>>>>>> From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
>>>>>>> Date: Mon, 30 Oct 2017 23:49:29 +0100
>>>>>>> Subject: [PATCH] lavf/latmenc: Error out for invalid codecs.
>>>>>>
>>>>>> isnt AV_CODEC_ID_MP4ALS supported too ? (i see ALS related code in
>>>>>> latmenc.c)
>>>>>
>>>>> How, when demuxer doesn't support it?
>>>>
>>>> Isn't ALS basically an extension of AAC? It would make sense that it
>>>> can also go into LATM/LAOS.
>>>
>>> But do we actually supports it? There is no ALS encoder, and only way to
>>> trigger
>>> that is via -c copy.
>>
>> And thats a problem why?
>
> Untested path.

But that seems unrelated to this patch, no?

Carl Eugen
Paul B Mahol Nov. 1, 2017, 1:39 p.m. UTC | #8
On 11/1/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> 2017-11-01 13:32 GMT+01:00 Paul B Mahol <onemda@gmail.com>:
>> On 11/1/17, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>>> On Wed, Nov 1, 2017 at 11:09 AM, Paul B Mahol <onemda@gmail.com> wrote:
>>>> On 11/1/17, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>>>>> On Wed, Nov 1, 2017 at 10:44 AM, Paul B Mahol <onemda@gmail.com> wrote:
>>>>>> On 10/31/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
>>>>>>> On Mon, Oct 30, 2017 at 11:51:30PM +0100, Carl Eugen Hoyos wrote:
>>>>>>>> Hi!
>>>>>>>>
>>>>>>>> Attached patch makes sure the loas muxer does not try to write
>>>>>>>> anything but aac and latm.
>>>>>>>>
>>>>>>>> Please comment, Carl Eugen
>>>>>>>
>>>>>>>>  latmenc.c |    4 ++++
>>>>>>>>  1 file changed, 4 insertions(+)
>>>>>>>> 2b64f3d5ecb189e77b85dbab7a6cbfe9657701f2
>>>>>>>> 0001-lavf-latmenc-Error-out-for-invalid-codecs.patch
>>>>>>>> From 9f8f39b402f77b53613a395129f96feee5e873ba Mon Sep 17 00:00:00
>>>>>>>> 2001
>>>>>>>> From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
>>>>>>>> Date: Mon, 30 Oct 2017 23:49:29 +0100
>>>>>>>> Subject: [PATCH] lavf/latmenc: Error out for invalid codecs.
>>>>>>>
>>>>>>> isnt AV_CODEC_ID_MP4ALS supported too ? (i see ALS related code in
>>>>>>> latmenc.c)
>>>>>>
>>>>>> How, when demuxer doesn't support it?
>>>>>
>>>>> Isn't ALS basically an extension of AAC? It would make sense that it
>>>>> can also go into LATM/LAOS.
>>>>
>>>> But do we actually supports it? There is no ALS encoder, and only way to
>>>> trigger
>>>> that is via -c copy.
>>>
>>> And thats a problem why?
>>
>> Untested path.
>
> But that seems unrelated to this patch, no?

Its related because you are adding codec which is not going to work.
Carl Eugen Hoyos Nov. 1, 2017, 2:23 p.m. UTC | #9
2017-11-01 14:39 GMT+01:00 Paul B Mahol <onemda@gmail.com>:
> On 11/1/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>> 2017-11-01 13:32 GMT+01:00 Paul B Mahol <onemda@gmail.com>:
>>> On 11/1/17, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>>>> On Wed, Nov 1, 2017 at 11:09 AM, Paul B Mahol <onemda@gmail.com> wrote:
>>>>> On 11/1/17, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>>>>>> On Wed, Nov 1, 2017 at 10:44 AM, Paul B Mahol <onemda@gmail.com> wrote:
>>>>>>> On 10/31/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
>>>>>>>> On Mon, Oct 30, 2017 at 11:51:30PM +0100, Carl Eugen Hoyos wrote:
>>>>>>>>> Hi!
>>>>>>>>>
>>>>>>>>> Attached patch makes sure the loas muxer does not try to write
>>>>>>>>> anything but aac and latm.
>>>>>>>>>
>>>>>>>>> Please comment, Carl Eugen
>>>>>>>>
>>>>>>>>>  latmenc.c |    4 ++++
>>>>>>>>>  1 file changed, 4 insertions(+)
>>>>>>>>> 2b64f3d5ecb189e77b85dbab7a6cbfe9657701f2
>>>>>>>>> 0001-lavf-latmenc-Error-out-for-invalid-codecs.patch
>>>>>>>>> From 9f8f39b402f77b53613a395129f96feee5e873ba Mon Sep 17 00:00:00
>>>>>>>>> 2001
>>>>>>>>> From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
>>>>>>>>> Date: Mon, 30 Oct 2017 23:49:29 +0100
>>>>>>>>> Subject: [PATCH] lavf/latmenc: Error out for invalid codecs.
>>>>>>>>
>>>>>>>> isnt AV_CODEC_ID_MP4ALS supported too ? (i see ALS related code in
>>>>>>>> latmenc.c)
>>>>>>>
>>>>>>> How, when demuxer doesn't support it?
>>>>>>
>>>>>> Isn't ALS basically an extension of AAC? It would make sense that it
>>>>>> can also go into LATM/LAOS.
>>>>>
>>>>> But do we actually supports it? There is no ALS encoder, and only way to
>>>>> trigger
>>>>> that is via -c copy.
>>>>
>>>> And thats a problem why?
>>>
>>> Untested path.
>>
>> But that seems unrelated to this patch, no?
>
> Its related because you are adding codec which is not going to work.

Sorry, I don't understand:
Some developers claim that als-in-loas muxing will work, you doubt it.
(I don't know, it's not easy to test and I don't find the spec atm.)
Now you are saying that if I prevent anything but aac, latm and als
from being muxed into loas, I am adding non-working code?

Please elaborate, Carl Eugen
James Almer Nov. 1, 2017, 2:26 p.m. UTC | #10
On 11/1/2017 11:23 AM, Carl Eugen Hoyos wrote:
> 2017-11-01 14:39 GMT+01:00 Paul B Mahol <onemda@gmail.com>:
>> On 11/1/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>> 2017-11-01 13:32 GMT+01:00 Paul B Mahol <onemda@gmail.com>:
>>>> On 11/1/17, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>>>>> On Wed, Nov 1, 2017 at 11:09 AM, Paul B Mahol <onemda@gmail.com> wrote:
>>>>>> On 11/1/17, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>>>>>>> On Wed, Nov 1, 2017 at 10:44 AM, Paul B Mahol <onemda@gmail.com> wrote:
>>>>>>>> On 10/31/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
>>>>>>>>> On Mon, Oct 30, 2017 at 11:51:30PM +0100, Carl Eugen Hoyos wrote:
>>>>>>>>>> Hi!
>>>>>>>>>>
>>>>>>>>>> Attached patch makes sure the loas muxer does not try to write
>>>>>>>>>> anything but aac and latm.
>>>>>>>>>>
>>>>>>>>>> Please comment, Carl Eugen
>>>>>>>>>
>>>>>>>>>>  latmenc.c |    4 ++++
>>>>>>>>>>  1 file changed, 4 insertions(+)
>>>>>>>>>> 2b64f3d5ecb189e77b85dbab7a6cbfe9657701f2
>>>>>>>>>> 0001-lavf-latmenc-Error-out-for-invalid-codecs.patch
>>>>>>>>>> From 9f8f39b402f77b53613a395129f96feee5e873ba Mon Sep 17 00:00:00
>>>>>>>>>> 2001
>>>>>>>>>> From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
>>>>>>>>>> Date: Mon, 30 Oct 2017 23:49:29 +0100
>>>>>>>>>> Subject: [PATCH] lavf/latmenc: Error out for invalid codecs.
>>>>>>>>>
>>>>>>>>> isnt AV_CODEC_ID_MP4ALS supported too ? (i see ALS related code in
>>>>>>>>> latmenc.c)
>>>>>>>>
>>>>>>>> How, when demuxer doesn't support it?
>>>>>>>
>>>>>>> Isn't ALS basically an extension of AAC? It would make sense that it
>>>>>>> can also go into LATM/LAOS.
>>>>>>
>>>>>> But do we actually supports it? There is no ALS encoder, and only way to
>>>>>> trigger
>>>>>> that is via -c copy.
>>>>>
>>>>> And thats a problem why?
>>>>
>>>> Untested path.
>>>
>>> But that seems unrelated to this patch, no?
>>
>> Its related because you are adding codec which is not going to work.
> 
> Sorry, I don't understand:
> Some developers claim that als-in-loas muxing will work, you doubt it.
> (I don't know, it's not easy to test and I don't find the spec atm.)
> Now you are saying that if I prevent anything but aac, latm and als
> from being muxed into loas, I am adding non-working code?
> 
> Please elaborate, Carl Eugen

I guess he's saying we'd be muxing files we wouldn't be able to demux,
which is a bad practice as we can't really test them.
Sort of like how we create animated Webp files we can't demux/decode.
Carl Eugen Hoyos Nov. 1, 2017, 2:33 p.m. UTC | #11
2017-11-01 15:26 GMT+01:00 James Almer <jamrial@gmail.com>:
> On 11/1/2017 11:23 AM, Carl Eugen Hoyos wrote:
>> 2017-11-01 14:39 GMT+01:00 Paul B Mahol <onemda@gmail.com>:
>>> On 11/1/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>>> 2017-11-01 13:32 GMT+01:00 Paul B Mahol <onemda@gmail.com>:
>>>>> On 11/1/17, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>>>>>> On Wed, Nov 1, 2017 at 11:09 AM, Paul B Mahol <onemda@gmail.com> wrote:
>>>>>>> On 11/1/17, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>>>>>>>> On Wed, Nov 1, 2017 at 10:44 AM, Paul B Mahol <onemda@gmail.com> wrote:
>>>>>>>>> On 10/31/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
>>>>>>>>>> On Mon, Oct 30, 2017 at 11:51:30PM +0100, Carl Eugen Hoyos wrote:
>>>>>>>>>>> Hi!
>>>>>>>>>>>
>>>>>>>>>>> Attached patch makes sure the loas muxer does not try to write
>>>>>>>>>>> anything but aac and latm.
>>>>>>>>>>>
>>>>>>>>>>> Please comment, Carl Eugen
>>>>>>>>>>
>>>>>>>>>>>  latmenc.c |    4 ++++
>>>>>>>>>>>  1 file changed, 4 insertions(+)
>>>>>>>>>>> 2b64f3d5ecb189e77b85dbab7a6cbfe9657701f2
>>>>>>>>>>> 0001-lavf-latmenc-Error-out-for-invalid-codecs.patch
>>>>>>>>>>> From 9f8f39b402f77b53613a395129f96feee5e873ba Mon Sep 17 00:00:00
>>>>>>>>>>> 2001
>>>>>>>>>>> From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
>>>>>>>>>>> Date: Mon, 30 Oct 2017 23:49:29 +0100
>>>>>>>>>>> Subject: [PATCH] lavf/latmenc: Error out for invalid codecs.
>>>>>>>>>>
>>>>>>>>>> isnt AV_CODEC_ID_MP4ALS supported too ? (i see ALS related code in
>>>>>>>>>> latmenc.c)
>>>>>>>>>
>>>>>>>>> How, when demuxer doesn't support it?
>>>>>>>>
>>>>>>>> Isn't ALS basically an extension of AAC? It would make sense that it
>>>>>>>> can also go into LATM/LAOS.
>>>>>>>
>>>>>>> But do we actually supports it? There is no ALS encoder, and only way to
>>>>>>> trigger
>>>>>>> that is via -c copy.
>>>>>>
>>>>>> And thats a problem why?
>>>>>
>>>>> Untested path.
>>>>
>>>> But that seems unrelated to this patch, no?
>>>
>>> Its related because you are adding codec which is not going to work.
>>
>> Sorry, I don't understand:
>> Some developers claim that als-in-loas muxing will work, you doubt it.
>> (I don't know, it's not easy to test and I don't find the spec atm.)
>> Now you are saying that if I prevent anything but aac, latm and als
>> from being muxed into loas, I am adding non-working code?
>>
>> Please elaborate, Carl Eugen
>
> I guess he's saying we'd be muxing files we wouldn't be able to demux,
> which is a bad practice as we can't really test them.

But how is this related to this patch?
(Apart from the fact that three developers disagree.)

> Sort of like how we create animated Webp files we can't demux/decode.

You are suggesting we should not create webp files?

Iirc, the practice used to be to first implement encoders / muxers,
then decoders / demuxers...

Carl Eugen
James Almer Nov. 1, 2017, 2:41 p.m. UTC | #12
On 11/1/2017 11:33 AM, Carl Eugen Hoyos wrote:
> 2017-11-01 15:26 GMT+01:00 James Almer <jamrial@gmail.com>:
>> On 11/1/2017 11:23 AM, Carl Eugen Hoyos wrote:
>>> 2017-11-01 14:39 GMT+01:00 Paul B Mahol <onemda@gmail.com>:
>>>> On 11/1/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>>>> 2017-11-01 13:32 GMT+01:00 Paul B Mahol <onemda@gmail.com>:
>>>>>> On 11/1/17, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>>>>>>> On Wed, Nov 1, 2017 at 11:09 AM, Paul B Mahol <onemda@gmail.com> wrote:
>>>>>>>> On 11/1/17, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>>>>>>>>> On Wed, Nov 1, 2017 at 10:44 AM, Paul B Mahol <onemda@gmail.com> wrote:
>>>>>>>>>> On 10/31/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
>>>>>>>>>>> On Mon, Oct 30, 2017 at 11:51:30PM +0100, Carl Eugen Hoyos wrote:
>>>>>>>>>>>> Hi!
>>>>>>>>>>>>
>>>>>>>>>>>> Attached patch makes sure the loas muxer does not try to write
>>>>>>>>>>>> anything but aac and latm.
>>>>>>>>>>>>
>>>>>>>>>>>> Please comment, Carl Eugen
>>>>>>>>>>>
>>>>>>>>>>>>  latmenc.c |    4 ++++
>>>>>>>>>>>>  1 file changed, 4 insertions(+)
>>>>>>>>>>>> 2b64f3d5ecb189e77b85dbab7a6cbfe9657701f2
>>>>>>>>>>>> 0001-lavf-latmenc-Error-out-for-invalid-codecs.patch
>>>>>>>>>>>> From 9f8f39b402f77b53613a395129f96feee5e873ba Mon Sep 17 00:00:00
>>>>>>>>>>>> 2001
>>>>>>>>>>>> From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
>>>>>>>>>>>> Date: Mon, 30 Oct 2017 23:49:29 +0100
>>>>>>>>>>>> Subject: [PATCH] lavf/latmenc: Error out for invalid codecs.
>>>>>>>>>>>
>>>>>>>>>>> isnt AV_CODEC_ID_MP4ALS supported too ? (i see ALS related code in
>>>>>>>>>>> latmenc.c)
>>>>>>>>>>
>>>>>>>>>> How, when demuxer doesn't support it?
>>>>>>>>>
>>>>>>>>> Isn't ALS basically an extension of AAC? It would make sense that it
>>>>>>>>> can also go into LATM/LAOS.
>>>>>>>>
>>>>>>>> But do we actually supports it? There is no ALS encoder, and only way to
>>>>>>>> trigger
>>>>>>>> that is via -c copy.
>>>>>>>
>>>>>>> And thats a problem why?
>>>>>>
>>>>>> Untested path.
>>>>>
>>>>> But that seems unrelated to this patch, no?
>>>>
>>>> Its related because you are adding codec which is not going to work.
>>>
>>> Sorry, I don't understand:
>>> Some developers claim that als-in-loas muxing will work, you doubt it.
>>> (I don't know, it's not easy to test and I don't find the spec atm.)
>>> Now you are saying that if I prevent anything but aac, latm and als
>>> from being muxed into loas, I am adding non-working code?
>>>
>>> Please elaborate, Carl Eugen
>>
>> I guess he's saying we'd be muxing files we wouldn't be able to demux,
>> which is a bad practice as we can't really test them.
> 
> But how is this related to this patch?
> (Apart from the fact that three developers disagree.)

I don't know, he might be able to tell you. I'm just saying what i
assume he meant with "not working".
It's not like this patch makes the LATM muxer start muxing things it
couldn't before.

> 
>> Sort of like how we create animated Webp files we can't demux/decode.
> 
> You are suggesting we should not create webp files?

I'm obviously suggesting we should add support for demuxing/decoding
animated webp.

> Iirc, the practice used to be to first implement encoders / muxers,
> then decoders / demuxers...

I don't know if that's always the case, but IMO it's a very bad practice
having ffmpeg produce files it can't decode to confirm they are in fact
working.
Paul B Mahol Nov. 1, 2017, 2:42 p.m. UTC | #13
On 11/1/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> 2017-11-01 15:26 GMT+01:00 James Almer <jamrial@gmail.com>:
>> On 11/1/2017 11:23 AM, Carl Eugen Hoyos wrote:
>>> 2017-11-01 14:39 GMT+01:00 Paul B Mahol <onemda@gmail.com>:
>>>> On 11/1/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>>>> 2017-11-01 13:32 GMT+01:00 Paul B Mahol <onemda@gmail.com>:
>>>>>> On 11/1/17, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>>>>>>> On Wed, Nov 1, 2017 at 11:09 AM, Paul B Mahol <onemda@gmail.com>
>>>>>>> wrote:
>>>>>>>> On 11/1/17, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>>>>>>>>> On Wed, Nov 1, 2017 at 10:44 AM, Paul B Mahol <onemda@gmail.com>
>>>>>>>>> wrote:
>>>>>>>>>> On 10/31/17, Michael Niedermayer <michael@niedermayer.cc> wrote:
>>>>>>>>>>> On Mon, Oct 30, 2017 at 11:51:30PM +0100, Carl Eugen Hoyos wrote:
>>>>>>>>>>>> Hi!
>>>>>>>>>>>>
>>>>>>>>>>>> Attached patch makes sure the loas muxer does not try to write
>>>>>>>>>>>> anything but aac and latm.
>>>>>>>>>>>>
>>>>>>>>>>>> Please comment, Carl Eugen
>>>>>>>>>>>
>>>>>>>>>>>>  latmenc.c |    4 ++++
>>>>>>>>>>>>  1 file changed, 4 insertions(+)
>>>>>>>>>>>> 2b64f3d5ecb189e77b85dbab7a6cbfe9657701f2
>>>>>>>>>>>> 0001-lavf-latmenc-Error-out-for-invalid-codecs.patch
>>>>>>>>>>>> From 9f8f39b402f77b53613a395129f96feee5e873ba Mon Sep 17
>>>>>>>>>>>> 00:00:00
>>>>>>>>>>>> 2001
>>>>>>>>>>>> From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
>>>>>>>>>>>> Date: Mon, 30 Oct 2017 23:49:29 +0100
>>>>>>>>>>>> Subject: [PATCH] lavf/latmenc: Error out for invalid codecs.
>>>>>>>>>>>
>>>>>>>>>>> isnt AV_CODEC_ID_MP4ALS supported too ? (i see ALS related code
>>>>>>>>>>> in
>>>>>>>>>>> latmenc.c)
>>>>>>>>>>
>>>>>>>>>> How, when demuxer doesn't support it?
>>>>>>>>>
>>>>>>>>> Isn't ALS basically an extension of AAC? It would make sense that
>>>>>>>>> it
>>>>>>>>> can also go into LATM/LAOS.
>>>>>>>>
>>>>>>>> But do we actually supports it? There is no ALS encoder, and only
>>>>>>>> way to
>>>>>>>> trigger
>>>>>>>> that is via -c copy.
>>>>>>>
>>>>>>> And thats a problem why?
>>>>>>
>>>>>> Untested path.
>>>>>
>>>>> But that seems unrelated to this patch, no?
>>>>
>>>> Its related because you are adding codec which is not going to work.
>>>
>>> Sorry, I don't understand:
>>> Some developers claim that als-in-loas muxing will work, you doubt it.
>>> (I don't know, it's not easy to test and I don't find the spec atm.)
>>> Now you are saying that if I prevent anything but aac, latm and als
>>> from being muxed into loas, I am adding non-working code?
>>>
>>> Please elaborate, Carl Eugen
>>
>> I guess he's saying we'd be muxing files we wouldn't be able to demux,
>> which is a bad practice as we can't really test them.
>
> But how is this related to this patch?
> (Apart from the fact that three developers disagree.)
>
>> Sort of like how we create animated Webp files we can't demux/decode.
>
> You are suggesting we should not create webp files?

Not creating broken webp files, see difference.

>
> Iirc, the practice used to be to first implement encoders / muxers,
> then decoders / demuxers...

No, it is other way around.
Carl Eugen Hoyos Nov. 1, 2017, 2:52 p.m. UTC | #14
2017-11-01 15:42 GMT+01:00 Paul B Mahol <onemda@gmail.com>:
> On 11/1/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> Not creating broken webp files, see difference.

How do you know that als-in-loas in invalid?

>> Iirc, the practice used to be to first implement encoders / muxers,
>> then decoders / demuxers...
>
> No, it is other way around.

I was talking about how the practice used to be, I don't think
you can deny that...

So what about this patch?

Carl Eugen
Paul B Mahol Nov. 1, 2017, 3:59 p.m. UTC | #15
On 11/1/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> 2017-11-01 15:42 GMT+01:00 Paul B Mahol <onemda@gmail.com>:
>> On 11/1/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
>> Not creating broken webp files, see difference.
>
> How do you know that als-in-loas in invalid?

Because you do not claim it is valid.

>
>>> Iirc, the practice used to be to first implement encoders / muxers,
>>> then decoders / demuxers...
>>
>> No, it is other way around.
>
> I was talking about how the practice used to be, I don't think
> you can deny that...
>
> So what about this patch?

It is flawed.
Carl Eugen Hoyos Nov. 1, 2017, 4:01 p.m. UTC | #16
2017-11-01 16:59 GMT+01:00 Paul B Mahol <onemda@gmail.com>:
> On 11/1/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>> 2017-11-01 15:42 GMT+01:00 Paul B Mahol <onemda@gmail.com>:
>>> On 11/1/17, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>
>>> Not creating broken webp files, see difference.
>>
>> How do you know that als-in-loas in invalid?
>
> Because you do not claim it is valid.

As said, I don't find the spec atm.
Isn't Kieran's opinion sufficient in this case?

Carl Eugen
Nicolas George Nov. 1, 2017, 6:30 p.m. UTC | #17
Le primidi 11 brumaire, an CCXXVI, James Almer a écrit :
> I guess he's saying

If you have to guess, then he is not saying it, he is hinting at it with
half sentences containing only the draft of an argument. I wish he would
start making full sentences with full arguments.

Regards,
diff mbox

Patch

From 9f8f39b402f77b53613a395129f96feee5e873ba Mon Sep 17 00:00:00 2001
From: Carl Eugen Hoyos <ceffmpeg@gmail.com>
Date: Mon, 30 Oct 2017 23:49:29 +0100
Subject: [PATCH] lavf/latmenc: Error out for invalid codecs.

---
 libavformat/latmenc.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/libavformat/latmenc.c b/libavformat/latmenc.c
index c919976..29a74e3 100644
--- a/libavformat/latmenc.c
+++ b/libavformat/latmenc.c
@@ -89,6 +89,10 @@  static int latm_write_header(AVFormatContext *s)
 
     if (par->codec_id == AV_CODEC_ID_AAC_LATM)
         return 0;
+    if (par->codec_id != AV_CODEC_ID_AAC) {
+        av_log(ctx, AV_LOG_ERROR, "Only AAC and LATM are supported\n");
+        return AVERROR_INVALIDDATA;
+    }
 
     if (par->extradata_size > 0 &&
         latm_decode_extradata(ctx, par->extradata, par->extradata_size) < 0)
-- 
1.7.10.4