mbox series

[FFmpeg-devel,v5,0/9] avformat: wav-s337m support + new probe_stream option

Message ID 20201019081929.1926-1-nicolas.gaullier@cji.paris
Headers show
Series avformat: wav-s337m support + new probe_stream option
Related show

Message

Nicolas Gaullier Oct. 19, 2020, 8:19 a.m. UTC
Updates:
* patch 1 : commit msg amended (anton)
* patch 3 : 'if' line split (tomas)

Everthing else: unchanged since v4

For remembering: the test sample 512.wav can be downloaded here:
https://0x0.st/zdW-.wav

Nicolas Gaullier (9):
  avcodec/dolby_e: set constant frame_size
  avformat/s337m: Split read_packet/get_packet
  avformat/s337m: Consider container bit resolution
  avformat/s337m: New ff_s337m_probe()
  avformat/wavdec: s337m support
  avformat/wavdec.c: Reindent after last commit
  avformat/wavdec: fix s337m/spdif probing beyond data_end
  avformat/wavdec: Test s337m
  avformat: Add probe_stream option

 doc/formats.texi            |  3 ++
 libavcodec/dolby_e.c        |  1 +
 libavformat/avformat.h      |  9 ++++-
 libavformat/options_table.h |  1 +
 libavformat/s337m.c         | 73 ++++++++++++++++++++++++++++++++-----
 libavformat/s337m.h         | 54 +++++++++++++++++++++++++++
 libavformat/utils.c         |  2 +
 libavformat/version.h       |  2 +-
 libavformat/wavdec.c        | 53 ++++++++++++++++++---------
 tests/Makefile              |  1 +
 tests/fate/audio.mak        |  3 ++
 tests/ref/fate/s337m-wav    | 11 ++++++
 12 files changed, 185 insertions(+), 28 deletions(-)
 create mode 100644 libavformat/s337m.h
 create mode 100644 tests/ref/fate/s337m-wav

Comments

Nicolas Gaullier Nov. 9, 2020, 9:34 a.m. UTC | #1
>Updates:
>* patch 1 : commit msg amended (anton)
>* patch 3 : 'if' line split (tomas)
>
>Everthing else: unchanged since v4
>
>For remembering: the test sample 512.wav can be downloaded here:
>https://0x0.st/zdW-.wav
>
>Nicolas Gaullier (9):
>  avcodec/dolby_e: set constant frame_size
>  avformat/s337m: Split read_packet/get_packet
>  avformat/s337m: Consider container bit resolution
>  avformat/s337m: New ff_s337m_probe()
>  avformat/wavdec: s337m support
>  avformat/wavdec.c: Reindent after last commit
>  avformat/wavdec: fix s337m/spdif probing beyond data_end
>  avformat/wavdec: Test s337m
>  avformat: Add probe_stream option
>
> doc/formats.texi            |  3 ++
> libavcodec/dolby_e.c        |  1 +
> libavformat/avformat.h      |  9 ++++-
> libavformat/options_table.h |  1 +
> libavformat/s337m.c         | 73 ++++++++++++++++++++++++++++++++-----
> libavformat/s337m.h         | 54 +++++++++++++++++++++++++++
> libavformat/utils.c         |  2 +
> libavformat/version.h       |  2 +-
> libavformat/wavdec.c        | 53 ++++++++++++++++++---------
> tests/Makefile              |  1 +
> tests/fate/audio.mak        |  3 ++
> tests/ref/fate/s337m-wav    | 11 ++++++
> 12 files changed, 185 insertions(+), 28 deletions(-)  create mode 100644 libavformat/s337m.h  create mode 100644 tests/ref/fate/s337m-wav

I think I have not received any feedback for this v5 posted 3 weeks ago. I can rebase everything if it can help for reviewing (or applying if you think it is close to be accepted).
NB: I keep an eye on irc (ngaullier).

Thx
Nicolas
Nicolas George Nov. 9, 2020, 9:40 a.m. UTC | #2
Nicolas Gaullier (12020-11-09):
> > doc/formats.texi            |  3 ++
> > libavcodec/dolby_e.c        |  1 +
> > libavformat/avformat.h      |  9 ++++-
> > libavformat/options_table.h |  1 +
> > libavformat/s337m.c         | 73 ++++++++++++++++++++++++++++++++-----
> > libavformat/s337m.h         | 54 +++++++++++++++++++++++++++
> > libavformat/utils.c         |  2 +
> > libavformat/version.h       |  2 +-
> > libavformat/wavdec.c        | 53 ++++++++++++++++++---------
> > tests/Makefile              |  1 +
> > tests/fate/audio.mak        |  3 ++
> > tests/ref/fate/s337m-wav    | 11 ++++++
> > 12 files changed, 185 insertions(+), 28 deletions(-)  create mode 100644 libavformat/s337m.h  create mode 100644 tests/ref/fate/s337m-wav
> 
> I think I have not received any feedback for this v5 posted 3 weeks
> ago. I can rebase everything if it can help for reviewing (or applying
> if you think it is close to be accepted).

A patch that mixes so many different changes has little chances to be
accepted as is. You need to separate changes by function. It would also
increase the chances of review.

Regards,
Nicolas Gaullier Nov. 9, 2020, 10:21 a.m. UTC | #3
>Nicolas Gaullier (12020-11-09):
>> > doc/formats.texi            |  3 ++
>> > libavcodec/dolby_e.c        |  1 +
>> > libavformat/avformat.h      |  9 ++++-
>> > libavformat/options_table.h |  1 +
>> > libavformat/s337m.c         | 73 ++++++++++++++++++++++++++++++++-----
>> > libavformat/s337m.h         | 54 +++++++++++++++++++++++++++
>> > libavformat/utils.c         |  2 +
>> > libavformat/version.h       |  2 +-
>> > libavformat/wavdec.c        | 53 ++++++++++++++++++---------
>> > tests/Makefile              |  1 +
>> > tests/fate/audio.mak        |  3 ++
>> > tests/ref/fate/s337m-wav    | 11 ++++++
>> > 12 files changed, 185 insertions(+), 28 deletions(-)  create mode
>> > 100644 libavformat/s337m.h  create mode 100644 
>> > tests/ref/fate/s337m-wav
>> 
>> I think I have not received any feedback for this v5 posted 3 weeks 
>> ago. I can rebase everything if it can help for reviewing (or 
>> applying if you think it is close to be accepted).
>
>A patch that mixes so many different changes has little chances to be accepted as is. You need to separate changes by function. It would also increase the chances of review.
>
>Regards,

Thank you for your response.
I fully understand that but the fact is, everything is bind :
- the first patch has not much interest alone, but yes, this one can be separated and it has already been discussed with Anton and approved on irc (10/06)
- patch 2 -> 8 are about one single thing : s337m support in wav; I could potentially remove patch 7 which affects spdif, but it has already been reviewed and seems no problem at all
- patch 9 : this is the tricky one as it is the most recent update in this patch set and has received few feedbacks

The fact is, as I explained earlier, patch 9 cannot be separated, because users must have the choice to disable dolby_e probing, this is a hard requirement.
I thought patchs 1->8 were ready - this thread began a long long time ago, I received many reviews ! - and patch 9 could be reviewed "alone".
That would be the best "git history", and I was not afraid by time but...
At the end, I understand your point, for sure.
I will wait another week and if the process is still frozen, I will rework everything : I will propose the patch 9 "Add AVOption" alone (ie. not including dolby_e), and wait for its approval before going on with the dolby_e serie.

Nicolas