Skip to content

Commit

Permalink
Don't show video load error if a blob has unrecognized or non-video m…
Browse files Browse the repository at this point in the history
…edia type (#7528)

### What

Before:

![image](https://github.com/user-attachments/assets/acb4868a-b440-4a10-8019-8913246fa9a1)

After:

![image](https://github.com/user-attachments/assets/00385097-4420-4b47-bc15-759afac3876f)

... luckily I tried an obj file whose media type can't be recognized
from bytes (because obj is a wild text format), which made me run into
the corner case of video module not recognizing the format _and_ us not
recognizing the media type from the blob.

Counter example, trying to load a wmv:

![image](https://github.com/user-attachments/assets/846c20d9-ad46-42d6-94d9-ec6e175c2517)
(btw. I did so by renaming the file to mp4 and dragging it in, otherwise
we reject it earlier. Same could be achieved with direct log calls ofc)

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using examples from latest `main` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/7528?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[rerun.io/viewer](https://rerun.io/viewer/pr/7528?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!
* [x] If have noted any breaking changes to the log API in
`CHANGELOG.md` and the migration guide

- [PR Build Summary](https://build.rerun.io/pr/7528)
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

To run all checks from `main`, comment on the PR with `@rerun-bot
full-check`.
  • Loading branch information
Wumpf authored Sep 27, 2024
1 parent 846478f commit 6b8ad66
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 9 deletions.
33 changes: 25 additions & 8 deletions crates/store/re_video/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,24 @@ impl VideoData {
} else if mp4::is_mp4(data) {
"video/mp4".to_owned()
} else {
// Technically this means that we failed to determine the media type altogether,
// but we don't want to call it `FailedToDetermineMediaType` since the rest of Rerun has
// access to `re_types::components::MediaType` which has a much wider range of media type detection.
return Err(VideoLoadError::UnsupportedVideoType);
return Err(VideoLoadError::UnrecognizedVideoFormat {
provided_media_type: media_type.map(|m| m.to_owned()),
});
};

match media_type.as_str() {
"video/mp4" => mp4::load_mp4(data),
media_type => Err(VideoLoadError::UnsupportedMediaType(media_type.to_owned())),
media_type => {
if media_type.starts_with("video/") {
Err(VideoLoadError::UnsupportedMediaType {
provided_or_detected_media_type: media_type.to_owned(),
})
} else {
Err(VideoLoadError::MediaTypeIsNotAVideo {
provided_or_detected_media_type: media_type.to_owned(),
})
}
}
}
}

Expand Down Expand Up @@ -260,11 +269,19 @@ pub enum VideoLoadError {
#[error("Video file has invalid sample entries")]
InvalidSamples,

#[error("Video file has unsupported media type {0}")]
UnsupportedMediaType(String),
#[error("The media type of the blob is not a video: {provided_or_detected_media_type}")]
MediaTypeIsNotAVideo {
provided_or_detected_media_type: String,
},

#[error("Video file has unsupported media type {provided_or_detected_media_type}")]
UnsupportedMediaType {
provided_or_detected_media_type: String,
},

// Technically this is a "failed to detect" case, but the only formats we detect as of writing are the ones we support.
#[error("Video file has unsupported format")]
UnsupportedVideoType,
UnrecognizedVideoFormat { provided_media_type: Option<String> },

// `FourCC`'s debug impl doesn't quote the result
#[error("Video track uses unsupported codec \"{0}\"")] // NOLINT
Expand Down
14 changes: 14 additions & 0 deletions crates/viewer/re_data_ui/src/blob.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ pub fn blob_preview_and_save_ui(
let video_result = ctx.cache.entry(|c: &mut re_viewer_context::VideoCache| {
c.entry(blob_row_id, blob, media_type.as_ref().map(|mt| mt.as_str()))
});

show_video_blob_info(
ctx.render_ctx,
ui,
Expand Down Expand Up @@ -170,6 +171,7 @@ fn show_video_blob_info(
video_result: &Result<re_renderer::video::Video, VideoLoadError>,
video_timestamp: Option<VideoTimestamp>,
) {
#[allow(clippy::match_same_arms)]
match video_result {
Ok(video) => {
if ui_layout.is_single_line() {
Expand Down Expand Up @@ -264,6 +266,18 @@ fn show_video_blob_info(
}
});
}
Err(VideoLoadError::MediaTypeIsNotAVideo { .. }) => {
// Don't show an error if this wasn't a video in the first place.
// Unfortunately we can't easily detect here if the Blob was _supposed_ to be a video, for that we'd need tagged components!
// (User may have confidently logged a non-video format as Video, we should tell them that!)
}
Err(VideoLoadError::UnrecognizedVideoFormat {
provided_media_type: None,
}) => {
// If we couldn't detect the media type and the loader didn't know the format,
// we can't show an error for unrecognized formats since maybe this wasn't a video to begin with.
// See also `MediaTypeIsNotAVideo` case above.
}
Err(err) => {
if ui_layout.is_single_line() {
ui.error_label(&format!("Failed to load video: {err}"));
Expand Down
3 changes: 2 additions & 1 deletion crates/viewer/re_data_ui/src/instance_path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,8 @@ fn preview_if_blob_ui(
let blob = blob.component_mono::<components::Blob>()?.ok()?;
let media_type = component_map
.get(&components::MediaType::name())
.and_then(|unit| unit.component_mono::<components::MediaType>()?.ok());
.and_then(|unit| unit.component_mono::<components::MediaType>()?.ok())
.or_else(|| components::MediaType::guess_from_data(&blob));

let video_timestamp = component_map
.get(&components::VideoTimestamp::name())
Expand Down

0 comments on commit 6b8ad66

Please sign in to comment.