-
Notifications
You must be signed in to change notification settings - Fork 48
VideoStreamMetadata.sample_aspect_ratio: new metadata field (#733) #737
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
Changes from 1 commit
c51a341
6c85f63
9f9db90
aa27992
8410456
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,6 +80,12 @@ class VideoStreamMetadata(StreamMetadata): | |
average_fps_from_header: Optional[float] | ||
"""Averate fps of the stream, obtained from the header (float or None). | ||
We recommend using the ``average_fps`` attribute instead.""" | ||
sample_aspect_ratio: Optional[tuple[int, int]] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because this is a public name, I think we should name it And as mentioned in previous discussion, let's make this a Fraction just to be on the safe side. |
||
"""Sample Aspect Ratio (SAR), also known as Pixel Aspect Ratio | ||
(PAR), is the ratio between the width of a pixel and the height of | ||
each pixel. This is a tuple of two ints: the first element is the | ||
numerator, and the second element is the denominator. Not to be | ||
confused with Storage Aspect Ratio (also SAR).""" | ||
|
||
@property | ||
def duration_seconds(self) -> Optional[float]: | ||
|
@@ -211,6 +217,16 @@ def best_audio_stream(self) -> AudioStreamMetadata: | |
return metadata | ||
|
||
|
||
def _get_optional_sar_tuple(stream_dict): | ||
try: | ||
return ( | ||
stream_dict["sampleAspectRatioNum"], | ||
stream_dict["sampleAspectRatioDen"], | ||
) | ||
except KeyError: | ||
return None | ||
|
||
|
||
# TODO-AUDIO: This is user-facing. Should this just be `get_metadata`, without | ||
# the "container" name in it? Same below. | ||
def get_container_metadata(decoder: torch.Tensor) -> ContainerMetadata: | ||
|
@@ -247,6 +263,9 @@ def get_container_metadata(decoder: torch.Tensor) -> ContainerMetadata: | |
num_frames_from_header=stream_dict.get("numFramesFromHeader"), | ||
num_frames_from_content=stream_dict.get("numFramesFromContent"), | ||
average_fps_from_header=stream_dict.get("averageFpsFromHeader"), | ||
# sample_aspect_ratio is a tuple. Return None, | ||
# and not (None, None), if missing. | ||
sample_aspect_ratio=_get_optional_sar_tuple(stream_dict), | ||
**common_meta, | ||
) | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -601,6 +601,12 @@ std::string get_stream_json_metadata( | |
if (streamMetadata.height.has_value()) { | ||
map["height"] = std::to_string(*streamMetadata.height); | ||
} | ||
if (streamMetadata.sampleAspectRatio.has_value()) { | ||
map["sampleAspectRatioNum"] = | ||
std::to_string((*streamMetadata.sampleAspectRatio).first); | ||
map["sampleAspectRatioDen"] = | ||
std::to_string((*streamMetadata.sampleAspectRatio).second); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think what you're doing here is OK. I don't see a much cleaner way around the json intermediate layer. @scotts any concern? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. We're turning what is fundamentally two values into one object. This is a reasonable way to do that. |
||
if (streamMetadata.averageFpsFromHeader.has_value()) { | ||
map["averageFpsFromHeader"] = | ||
std::to_string(*streamMetadata.averageFpsFromHeader); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're using FFmpeg types here already, so I think we should define this as AVRational instead of a tuple