Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Cherry-picked 2.0.1] Properly set #samples passed to encoder #3204

Closed
wants to merge 1 commit into from

Conversation

mthrok
Copy link
Collaborator

@mthrok mthrok commented Mar 24, 2023

Some audio encoders expect specific, exact number of samples described as in AVCodecContext.frame_size.

The AVFrame.nb_samples is set for the frames passed to AVFilterGraph, but frames coming out of the graph do not necessarily have the same numbr of frames.

This causes issues with encoding OPUS.

This commit fixes it by inserting asetnsamples to filter graph if a fixed number of samples is requested.

Followup:

  1. issue warning if encoding opus in FFmpeg 4.1
  2. check if AAC issue is caused by this
    ######################################################################
    # Note on slicing and AAC
    # ~~~~~~~~~~~~~~~~~~~~~~~
    #
    # .. warning::
    #
    # FFmpeg's native AAC encoder (which is used by default when
    # saving video with MP4 format) has a bug that affects the audibility.
    #
    # Please refer to the examples bellow.
    #
    def test_slice(audio_encoder, slice_size, ext="mp4"):
    path = get_path(f"slice_{slice_size}.{ext}")
    s = StreamWriter(dst=path)
    s.add_audio_stream(SAMPLE_RATE, NUM_CHANNELS, encoder=audio_encoder)
    with s.open():
    for start in range(0, NUM_FRAMES, slice_size):
    end = start + slice_size
    s.write_audio_chunk(0, WAVEFORM[start:end, ...])
    return path
    ######################################################################
    #
    # This causes some artifacts.
    # note:
    # Chrome does not support playing AAC audio directly while Safari does.
    # Using MP4 container and specifying AAC allows Chrome to play it.
    Video(test_slice(audio_encoder="aac", slice_size=8000, ext="mp4"), embed=True)
    ######################################################################
    #
    # It is more noticeable when using smaller slice.
    Video(test_slice(audio_encoder="aac", slice_size=512, ext="mp4"), embed=True)
    ######################################################################
    #
    # Lame MP3 encoder works fine for the same slice size.
    Audio(test_slice(audio_encoder="libmp3lame", slice_size=512, ext="mp3"))

@facebook-github-bot
Copy link
Contributor

@mthrok has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D44374668

mthrok added a commit to mthrok/audio that referenced this pull request Mar 24, 2023
Summary:
Some audio encoders expect specific, exact number of samples described as in `AVCodecContext.frame_size`.

The `AVFrame.nb_samples` is set for the frames passed to `AVFilterGraph`, but frames coming out of the graph do not necessarily have the same numbr of frames.

This causes issues with encoding OPUS.

This commit fixes it by inserting `asetnsamples` to filter graph if a fixed number of samples is requested.

Pull Request resolved: pytorch#3204

Differential Revision: D44374668

Pulled By: mthrok

fbshipit-source-id: 444f296c288bd7fee4172e89e2f465e0f2762d48
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D44374668

mthrok added a commit to mthrok/audio that referenced this pull request Mar 25, 2023
Summary:
Some audio encoders expect specific, exact number of samples described as in `AVCodecContext.frame_size`.

The `AVFrame.nb_samples` is set for the frames passed to `AVFilterGraph`, but frames coming out of the graph do not necessarily have the same numbr of frames.

This causes issues with encoding OPUS.

This commit fixes it by inserting `asetnsamples` to filter graph if a fixed number of samples is requested.

Pull Request resolved: pytorch#3204

Reviewed By: nateanl

Differential Revision: D44374668

Pulled By: mthrok

fbshipit-source-id: 3025ed586a010b87e7870220bfa0dadf2317ba87
@facebook-github-bot
Copy link
Contributor

@mthrok has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary:
Some audio encoders expect specific, exact number of samples described as in `AVCodecContext.frame_size`.

The `AVFrame.nb_samples` is set for the frames passed to `AVFilterGraph`,
but frames coming out of the graph do not necessarily have the same numbr of frames.

This causes issues with encoding OPUS (among others).

This commit fixes it by inserting `asetnsamples` to filter graph if a fixed number of samples is requested.

Note:
It turned out that FFmpeg 4.1 has issue with OPUS encoding. It does not properly discard some sample.
We should probably move the minimum required FFmpeg to 4.2, but I am not sure if we can enforce it via ABI.
Work around will be to issue an warning if encoding OPUS with 4.1. (follow-up)

Pull Request resolved: pytorch#3204

Reviewed By: nateanl

Differential Revision: D44374668

Pulled By: mthrok

fbshipit-source-id: 723131e4f9b1979928f3ea2eddda17b1180b1a27
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D44374668

@facebook-github-bot
Copy link
Contributor

@mthrok merged this pull request in d8a37a2.

@github-actions
Copy link

Hey @mthrok.
You merged this PR, but labels were not properly added. Please add a primary and secondary label (See https://github.com/pytorch/audio/blob/main/.github/process_commit.py)

@mthrok mthrok deleted the fix-opus branch March 25, 2023 18:56
@mthrok mthrok mentioned this pull request Apr 4, 2023
mthrok added a commit to mthrok/audio that referenced this pull request Apr 4, 2023
Summary:
Some audio encoders expect specific, exact number of samples described as in `AVCodecContext.frame_size`.

The `AVFrame.nb_samples` is set for the frames passed to `AVFilterGraph`,
but frames coming out of the graph do not necessarily have the same numbr of frames.

This causes issues with encoding OPUS (among others).

This commit fixes it by inserting `asetnsamples` to filter graph if a fixed number of samples is requested.

Note:
It turned out that FFmpeg 4.1 has issue with OPUS encoding. It does not properly discard some sample.
We should probably move the minimum required FFmpeg to 4.2, but I am not sure if we can enforce it via ABI.
Work around will be to issue an warning if encoding OPUS with 4.1. (follow-up)

Pull Request resolved: pytorch#3204

Reviewed By: nateanl

Differential Revision: D44374668

Pulled By: mthrok

fbshipit-source-id: 10ef5333dc0677dfb83c8e40b78edd8ded1b21dc
mthrok added a commit that referenced this pull request Apr 5, 2023
Summary:
Some audio encoders expect specific, exact number of samples described as in `AVCodecContext.frame_size`.

The `AVFrame.nb_samples` is set for the frames passed to `AVFilterGraph`,
but frames coming out of the graph do not necessarily have the same numbr of frames.

This causes issues with encoding OPUS (among others).

This commit fixes it by inserting `asetnsamples` to filter graph if a fixed number of samples is requested.

Note:
It turned out that FFmpeg 4.1 has issue with OPUS encoding. It does not properly discard some sample.
We should probably move the minimum required FFmpeg to 4.2, but I am not sure if we can enforce it via ABI.
Work around will be to issue an warning if encoding OPUS with 4.1. (follow-up)

Pull Request resolved: #3204

Reviewed By: nateanl

Differential Revision: D44374668

Pulled By: mthrok

fbshipit-source-id: 10ef5333dc0677dfb83c8e40b78edd8ded1b21dc
@mthrok mthrok changed the title Properly set #samples passed to encoder [Cherry-picked 2.0.1] Properly set #samples passed to encoder Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants