Skip to content

Conversation

@mthrok
Copy link
Contributor

@mthrok mthrok commented Jan 20, 2023

Extraction from #2994

Add docstrings to C++ StreamReader/Writer.

@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.

@mthrok mthrok force-pushed the docstring-streamreader branch from 9682cce to e07091d Compare January 20, 2023 14:34
@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.

/// Find a suitable audio stream using heuristics from ffmpeg.
///
/// If successful, the index of the best stream (0>=) is returned.
/// otherwise a negative value is returned.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// otherwise a negative value is returned.
/// Otherwise a negative value is returned.

/// Fetch metadata of the source media.
OptionDict get_metadata() const;
// Fetch information about source streams
/// Fetch the number of source streames found in the input media.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Fetch the number of source streames found in the input media.
/// Fetch the number of source streams found in the input media.

int64_t num_src_streams() const;
/// Fetch information about the specified source stream.
///
/// The valid value are ``[0, num_src_streams())``.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// The valid value are ``[0, num_src_streams())``.
/// The valid values are ``[0, num_src_streams())``.

int64_t num_out_streams() const;
/// Fetch information about the specified output stream.
///
/// The valid value are ``[0, num_out_streams())``.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// The valid value are ``[0, num_out_streams())``.
/// The valid values are ``[0, num_out_streams())``.

/// @param frames_per_chunk Number of frames returned as one chunk.
/// @parblock
/// If the source stream is exhausted before enough frames are buffered,
/// then the chunk is returned as-is.The number of frames in one chunk.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the second sentence not finished?

Suggested change
/// then the chunk is returned as-is.The number of frames in one chunk.
/// then the chunk is returned as-is. The number of frames in one chunk.

/// Remove an output stream.
///
/// @param i The index of the output stream to be removed.
/// The valid value range is `[0, num_out_streams())`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This phrase looks better, shall we use it for all places?

Comment on lines 230 to 234
/// - ``0``: A packet was processed successfully and there are still packets
/// left in the stream, so client code can call it again.
/// - ``1``: A packet was processed successfully and it reached EOF.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// - ``0``: A packet was processed successfully and there are still packets
/// left in the stream, so client code can call it again.
/// - ``1``: A packet was processed successfully and it reached EOF.
/// - ``0``: A packet has been processed successfully and there are still packets
/// left in the stream, so client code can call it again.
/// - ``1``: A packet has been processed successfully and it has reached EOF.

/// When video is encoded on CUDA hardware, for example
/// `encoder="h264_nvenc"`, passing CUDA device indicator to `hw_accel`
/// (i.e. `hw_accel="cuda:0"`) will make StreamWriter expect video
/// chunk to be CUDA Tensor. Passing CPU Tensor will result in an error.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// chunk to be CUDA Tensor. Passing CPU Tensor will result in an error.
/// chunk to be a CUDA Tensor. Passing CPU Tensor will result in an error.

/// (i.e. `hw_accel="cuda:0"`) will make StreamWriter expect video
/// chunk to be CUDA Tensor. Passing CPU Tensor will result in an error.
///
/// If `None`, the video chunk Tensor has to be CPU Tensor.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// If `None`, the video chunk Tensor has to be CPU Tensor.
/// If `None`, the video chunk Tensor will be a CPU Tensor.

/// width and the number of channels)`` must match what was configured when
/// calling ``add_video_stream()``.
void write_video_chunk(int i, const torch::Tensor& chunk);
/// Flush the frames from encoders and write the frames to the destination.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should it be encoder?

Suggested change
/// Flush the frames from encoders and write the frames to the destination.
/// Flush the frames from encoder and write the frames to the destination.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, each stream has dedicated encoder. so there are multiple of them.

Summary:
Extraction from pytorch#2994

Add docstrings to C++ StreamReader/Writer.

Pull Request resolved: pytorch#2997

Reviewed By: nateanl

Differential Revision: D42628016

Pulled By: mthrok

fbshipit-source-id: c2fe25802c1a3b5c07e253db8fc104e652e0add3
@mthrok mthrok force-pushed the docstring-streamreader branch from e07091d to f97df83 Compare January 20, 2023 16:34
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@mthrok merged this pull request in de62822.

@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 docstring-streamreader branch January 20, 2023 20:40
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.

4 participants