mbox series

[FFmpeg-devel,0/2] avcodec/dolby_e: Add a parser

Message ID 20210114162610.1385-1-nicolas.gaullier@cji.paris
Headers show
Series avcodec/dolby_e: Add a parser
Related show

Message

Nicolas Gaullier Jan. 14, 2021, 4:26 p.m. UTC
I have limited duplicated code by making the decoder calling the parser.
An option would be to leave all common code to dolby_e.c and move decoding to dolby_edec.c,
but that would require either to duplicate 3 "very-internal" functions (skip_input/parse_key/convert_input) 3x times instead of twice currently,
or to share them with ff_ prefix although they are very-low level and difficult to document etc.

If you have an idea for a better design, please tell me.

Nicolas Gaullier (2):
  avcodec/dolby_e: Add a parser
  avcodec/dolby_e: Split decoder/parser files

 Changelog                            |   1 +
 libavcodec/Makefile                  |   1 +
 libavcodec/dolby_e.c                 | 209 ++++-----
 libavcodec/dolby_e.h                 | 608 +--------------------------
 libavcodec/dolby_e_parser.c          | 227 ++++++++++
 libavcodec/dolby_e_parser.h          |  41 ++
 libavcodec/dolby_e_parser_internal.h |  46 ++
 libavcodec/dolby_edec.h              | 607 ++++++++++++++++++++++++++
 libavcodec/parsers.c                 |   1 +
 libavcodec/version.h                 |   2 +-
 10 files changed, 1019 insertions(+), 724 deletions(-)
 create mode 100644 libavcodec/dolby_e_parser.c
 create mode 100644 libavcodec/dolby_e_parser.h
 create mode 100644 libavcodec/dolby_e_parser_internal.h
 create mode 100644 libavcodec/dolby_edec.h

Comments

Nicolas Gaullier Jan. 21, 2021, 12:29 p.m. UTC | #1
>De : Nicolas Gaullier <nicolas.gaullier@cji.paris> 
>Envoyé : jeudi 14 janvier 2021 17:26
>À : ffmpeg-devel@ffmpeg.org
>Cc : Nicolas Gaullier <nicolas.gaullier@cji.paris>
>Objet : [PATCH 0/2] avcodec/dolby_e: Add a parser
>
>I have limited duplicated code by making the decoder calling the parser.
>An option would be to leave all common code to dolby_e.c and move decoding to dolby_edec.c, but that would require either to duplicate 3 "very-internal" functions (skip_input/parse_key/convert_input) 3x times instead of twice >currently, or to share them with ff_ prefix although they are very-low level and difficult to document etc.
>
>If you have an idea for a better design, please tell me.
>
>Nicolas Gaullier (2):
>  avcodec/dolby_e: Add a parser
>  avcodec/dolby_e: Split decoder/parser files
>
> Changelog                            |   1 +
> libavcodec/Makefile                  |   1 +
> libavcodec/dolby_e.c                 | 209 ++++-----
> libavcodec/dolby_e.h                 | 608 +--------------------------
> libavcodec/dolby_e_parser.c          | 227 ++++++++++
> libavcodec/dolby_e_parser.h          |  41 ++
> libavcodec/dolby_e_parser_internal.h |  46 ++
> libavcodec/dolby_edec.h              | 607 ++++++++++++++++++++++++++
> libavcodec/parsers.c                 |   1 +
> libavcodec/version.h                 |   2 +-
> 10 files changed, 1019 insertions(+), 724 deletions(-)  create mode 100644 libavcodec/dolby_e_parser.c  create mode 100644 libavcodec/dolby_e_parser.h  create mode 100644 libavcodec/dolby_e_parser_internal.h
> create mode 100644 libavcodec/dolby_edec.h
>
>--
>2.27.0.windows.1

I have not received any feedback since my post last week.
Could someone find time to start the review ?
Thank you very much
Nicolas
Paul B Mahol Jan. 21, 2021, 12:35 p.m. UTC | #2
Changes looks trivial enough.

On Thu, Jan 21, 2021 at 1:30 PM Nicolas Gaullier <nicolas.gaullier@cji.paris>
wrote:

> >De : Nicolas Gaullier <nicolas.gaullier@cji.paris>
> >Envoyé : jeudi 14 janvier 2021 17:26
> >À : ffmpeg-devel@ffmpeg.org
> >Cc : Nicolas Gaullier <nicolas.gaullier@cji.paris>
> >Objet : [PATCH 0/2] avcodec/dolby_e: Add a parser
> >
> >I have limited duplicated code by making the decoder calling the parser.
> >An option would be to leave all common code to dolby_e.c and move
> decoding to dolby_edec.c, but that would require either to duplicate 3
> "very-internal" functions (skip_input/parse_key/convert_input) 3x times
> instead of twice >currently, or to share them with ff_ prefix although they
> are very-low level and difficult to document etc.
> >
> >If you have an idea for a better design, please tell me.
> >
> >Nicolas Gaullier (2):
> >  avcodec/dolby_e: Add a parser
> >  avcodec/dolby_e: Split decoder/parser files
> >
> > Changelog                            |   1 +
> > libavcodec/Makefile                  |   1 +
> > libavcodec/dolby_e.c                 | 209 ++++-----
> > libavcodec/dolby_e.h                 | 608 +--------------------------
> > libavcodec/dolby_e_parser.c          | 227 ++++++++++
> > libavcodec/dolby_e_parser.h          |  41 ++
> > libavcodec/dolby_e_parser_internal.h |  46 ++
> > libavcodec/dolby_edec.h              | 607 ++++++++++++++++++++++++++
> > libavcodec/parsers.c                 |   1 +
> > libavcodec/version.h                 |   2 +-
> > 10 files changed, 1019 insertions(+), 724 deletions(-)  create mode
> 100644 libavcodec/dolby_e_parser.c  create mode 100644
> libavcodec/dolby_e_parser.h  create mode 100644
> libavcodec/dolby_e_parser_internal.h
> > create mode 100644 libavcodec/dolby_edec.h
> >
> >--
> >2.27.0.windows.1
>
> I have not received any feedback since my post last week.
> Could someone find time to start the review ?
> Thank you very much
> Nicolas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Nicolas Gaullier Jan. 25, 2021, 11:03 a.m. UTC | #3
>De : ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> De la part de Paul B Mahol
>Envoyé : jeudi 21 janvier 2021 13:36
>À : FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
>Objet : Re: [FFmpeg-devel] [PATCH 0/2] avcodec/dolby_e: Add a parser
>
>Changes looks trivial enough.

If no one object, the two patches may be applied now ? I can rebase to make it straightforward.
Thank you,
Nicolas
Paul B Mahol Jan. 25, 2021, 11:08 a.m. UTC | #4
On Mon, Jan 25, 2021 at 12:04 PM Nicolas Gaullier
<nicolas.gaullier@cji.paris> wrote:

> >De : ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> De la part de Paul B
> Mahol
> >Envoyé : jeudi 21 janvier 2021 13:36
> >À : FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> >Objet : Re: [FFmpeg-devel] [PATCH 0/2] avcodec/dolby_e: Add a parser
> >
> >Changes looks trivial enough.
>
> If no one object, the two patches may be applied now ? I can rebase to
> make it straightforward.
>

Please do.


> Thank you,
> Nicolas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Lynne Jan. 25, 2021, 3:35 p.m. UTC | #5
Jan 25, 2021, 12:03 by nicolas.gaullier@cji.paris:

> >De : ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> De la part de Paul B Mahol
> >Envoyé : jeudi 21 janvier 2021 13:36
> >À : FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
> >Objet : Re: [FFmpeg-devel] [PATCH 0/2] avcodec/dolby_e: Add a parser
>
>>
>>
> >Changes looks trivial enough.
>
> If no one object, the two patches may be applied now ? I can rebase to make it straightforward.
> Thank you,
> Nicolas
>

Any objections to renaming dolby_edec.h to dolby_e_tab?
It's what it is.
James Almer Jan. 25, 2021, 3:39 p.m. UTC | #6
On 1/25/2021 12:35 PM, Lynne wrote:
> Jan 25, 2021, 12:03 by nicolas.gaullier@cji.paris:
> 
>>> De : ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> De la part de Paul B Mahol
>>> Envoyé : jeudi 21 janvier 2021 13:36
>>> À : FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
>>> Objet : Re: [FFmpeg-devel] [PATCH 0/2] avcodec/dolby_e: Add a parser
>>
>>>
>>>
>>> Changes looks trivial enough.
>>
>> If no one object, the two patches may be applied now ? I can rebase to make it straightforward.
>> Thank you,
>> Nicolas
>>
> 
> Any objections to renaming dolby_edec.h to dolby_e_tab?
> It's what it is.

Look at my patch if you can. I moved them to the decoder header.

They could even be moved to dolby_e.c since that's the only user of such 
tables.
Lynne Jan. 25, 2021, 3:46 p.m. UTC | #7
Jan 25, 2021, 16:39 by jamrial@gmail.com:

> On 1/25/2021 12:35 PM, Lynne wrote:
>
>> Jan 25, 2021, 12:03 by nicolas.gaullier@cji.paris:
>>
>>>> De : ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> De la part de Paul B Mahol
>>>> Envoyé : jeudi 21 janvier 2021 13:36
>>>> À : FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
>>>> Objet : Re: [FFmpeg-devel] [PATCH 0/2] avcodec/dolby_e: Add a parser
>>>>
>>>>
>>>>
>>>> Changes looks trivial enough.
>>>>
>>>
>>> If no one object, the two patches may be applied now ? I can rebase to make it straightforward.
>>> Thank you,
>>> Nicolas
>>>
>>
>> Any objections to renaming dolby_edec.h to dolby_e_tab?
>> It's what it is.
>>
>
> Look at my patch if you can. I moved them to the decoder header.
>
> They could even be moved to dolby_e.c since that's the only user of such tables.
>

Yeah, I just noticed your patch, was looking through the oldest emails
first (tons of emails today on the ML).
It's fine. LGTM.