From 8bf3d4be3a2a0cdb72a6fa88f8f13becfe3eb1c0 Mon Sep 17 00:00:00 2001 From: Kaido Kert Date: Sun, 24 Mar 2024 11:27:25 -0700 Subject: [PATCH 1/4] Remove unused use_skia_next b/150410605 --- cobalt/site/docs/reference/starboard/gn-configuration.md | 1 - starboard/build/config/base_configuration.gni | 3 --- 2 files changed, 4 deletions(-) diff --git a/cobalt/site/docs/reference/starboard/gn-configuration.md b/cobalt/site/docs/reference/starboard/gn-configuration.md index b139d8036a23..4c78cd275042 100644 --- a/cobalt/site/docs/reference/starboard/gn-configuration.md +++ b/cobalt/site/docs/reference/starboard/gn-configuration.md @@ -44,7 +44,6 @@ Book: /youtube/cobalt/_book.yaml | **`starboard_level_final_executable_type`**

The default value is `"executable"`. | | **`starboard_level_gtest_target_type`**

The default value is `"executable"`. | | **`static_library_configs`**

Target-specific configurations for static_library targets.

The default value is `[]`. | -| **`use_skia_next`**

Flag to use a future version of Skia, currently not available.

The default value is `false`. | | **`use_thin_archive`**

Whether or not to link with thin archives.

The default value is `true`. | | **`v8_enable_pointer_compression_override`**

Set to true to enable pointer compression for v8.

The default value is `true`. | | **`v8_enable_webassembly`**

Enable WASM and install WebAssembly global.

The default value is `false`. | diff --git a/starboard/build/config/base_configuration.gni b/starboard/build/config/base_configuration.gni index 713606314c28..aed5a1af9117 100644 --- a/starboard/build/config/base_configuration.gni +++ b/starboard/build/config/base_configuration.gni @@ -162,9 +162,6 @@ declare_args() { # Enables an NPLB audit of C++17 support. sb_enable_cpp17_audit = true - # Flag to use a future version of Skia, currently not available. - use_skia_next = false - # Enable when using clang 16. is_clang_16 = false From 2eb0dbc8071fa5862d40623a2439394e1e970c74 Mon Sep 17 00:00:00 2001 From: Kaido Kert Date: Sun, 24 Mar 2024 12:28:57 -0700 Subject: [PATCH 2/4] Remove BILINEAR_FILTERING config Starboard config for this was already deprecated, this removes remnant code referencing the value. b/150410605 --- cobalt/renderer/rasterizer/pixel_test.cc | 12 ------------ starboard/linux/shared/configuration_public.h | 7 ------- starboard/xb1/shared/configuration_public.h | 5 ----- 3 files changed, 24 deletions(-) diff --git a/cobalt/renderer/rasterizer/pixel_test.cc b/cobalt/renderer/rasterizer/pixel_test.cc index 51cc1644ab89..225dd3439c25 100644 --- a/cobalt/renderer/rasterizer/pixel_test.cc +++ b/cobalt/renderer/rasterizer/pixel_test.cc @@ -57,16 +57,8 @@ #include "third_party/glm/glm/gtc/matrix_transform.hpp" #include "third_party/glm/glm/gtx/transform.hpp" -#define BILINEAR_FILTERING_SUPPORTED 1 #define NV12_TEXTURE_SUPPORTED 1 -#if defined(STARBOARD) -#if !SB_HAS(BILINEAR_FILTERING_SUPPORT) -#undef BILINEAR_FILTERING_SUPPORTED -#define BILINEAR_FILTERING_SUPPORTED 0 -#endif -#endif - #if defined(STARBOARD) #if !SB_HAS(NV12_TEXTURE_SUPPORT) #undef NV12_TEXTURE_SUPPORTED @@ -2075,8 +2067,6 @@ TEST_F(PixelTest, ImageEdgeNoWrapWithPixelCentersOffset) { PointF(100.0f, 100.51f), kNumCascades)); } -#if BILINEAR_FILTERING_SUPPORTED - TEST_F(PixelTest, ImagesAreLinearlyInterpolated) { // We want to make sure that image pixels are accessed through a bilinear // interpolation magnification filter. @@ -2097,8 +2087,6 @@ TEST_F(PixelTest, ZoomedInImagesDoNotWrapInterpolated) { ScaleMatrix(2) * TranslateMatrix(-0.5f, -0.5f))); } -#endif // BILINEAR_FILTERING_SUPPORTED - TEST_F(PixelTest, YUV3PlaneImagesAreLinearlyInterpolated) { // Tests that three plane YUV images are bilinearly interpolated. scoped_refptr image = MakeI420Image(GetResourceProvider(), Size(8, 8)); diff --git a/starboard/linux/shared/configuration_public.h b/starboard/linux/shared/configuration_public.h index cd5054f487af..7e0514fe3dee 100644 --- a/starboard/linux/shared/configuration_public.h +++ b/starboard/linux/shared/configuration_public.h @@ -104,13 +104,6 @@ // The location to include hash_set on this platform. #define SB_HASH_SET_INCLUDE -// --- Graphics Configuration ------------------------------------------------ - -// Indicates whether or not the given platform supports bilinear filtering. -// This can be checked to enable/disable renderer tests that verify that this is -// working properly. -#define SB_HAS_BILINEAR_FILTERING_SUPPORT 1 - // --- I/O Configuration ----------------------------------------------------- // Whether the current platform has speech synthesis. diff --git a/starboard/xb1/shared/configuration_public.h b/starboard/xb1/shared/configuration_public.h index a1f5c6feda3b..f520feea734f 100644 --- a/starboard/xb1/shared/configuration_public.h +++ b/starboard/xb1/shared/configuration_public.h @@ -111,11 +111,6 @@ // --- Graphics Configuration ------------------------------------------------ -// Indicates whether or not the given platform supports bilinear filtering. -// This can be checked to enable/disable renderer tests that verify that this is -// working properly. -#define SB_HAS_BILINEAR_FILTERING_SUPPORT 1 - // Indicates whether or not the given platform supports rendering of NV12 // textures. These textures typically originate from video decoders. #define SB_HAS_NV12_TEXTURE_SUPPORT 0 From 999de539bc71495078d0f51616f1531f9410b9e0 Mon Sep 17 00:00:00 2001 From: Antonio Rivera <141369695+antoniori-eng@users.noreply.github.com> Date: Mon, 25 Mar 2024 09:00:02 -0700 Subject: [PATCH 3/4] Fix FFmpeg errors when using the newer FFmpeg decoding API. (#2683) The newer API requires decoding to be done in two stages: 1. send the packet to FFmpeg, and 2. receive 0 or more decoded frames from FFmpeg. Test: Served a webpage locally via flask. The webpage contained a single audio or video element. I then ran, e.g.: cobalt --url=http://127.0.0.1:5000/mp3 --enable_demuxer_extension and played the content (e.g. `document.querySelector('audio').play()` from the JS inspector) Previously, avcodec_receive_frame would repeatedly fail with AVERROR(EAGAIN), because no packets had been pushed. Bug: b/330908454 Change-Id: I245d74f880ed9297dca35e8c548f482373077121 --- .../ffmpeg/ffmpeg_audio_decoder_impl.cc | 124 +++++++++++------- .../shared/ffmpeg/ffmpeg_audio_decoder_impl.h | 5 + starboard/shared/ffmpeg/ffmpeg_dispatch.h | 8 +- .../ffmpeg_dynamic_load_dispatch_impl.cc | 1 + .../ffmpeg/ffmpeg_linked_dispatch_impl.cc | 5 + .../ffmpeg/ffmpeg_video_decoder_impl.cc | 80 ++++++++--- .../shared/ffmpeg/ffmpeg_video_decoder_impl.h | 6 + 7 files changed, 160 insertions(+), 69 deletions(-) diff --git a/starboard/shared/ffmpeg/ffmpeg_audio_decoder_impl.cc b/starboard/shared/ffmpeg/ffmpeg_audio_decoder_impl.cc index ba54a7817a14..a5cff77b9634 100644 --- a/starboard/shared/ffmpeg/ffmpeg_audio_decoder_impl.cc +++ b/starboard/shared/ffmpeg/ffmpeg_audio_decoder_impl.cc @@ -17,6 +17,8 @@ #include "starboard/shared/ffmpeg/ffmpeg_audio_decoder_impl.h" +#include + #include "starboard/audio_sink.h" #include "starboard/common/log.h" #include "starboard/common/string.h" @@ -160,18 +162,13 @@ void AudioDecoderImpl::Decode(const InputBuffers& input_buffers, #if LIBAVUTIL_VERSION_INT < LIBAVUTIL_VERSION_52_8 ffmpeg_->avcodec_get_frame_defaults(av_frame_); #endif // LIBAVUTIL_VERSION_INT < LIBAVUTIL_VERSION_52_8 - int frame_decoded = 0; int result = 0; if (ffmpeg_->avcodec_version() < kAVCodecHasUniformDecodeAPI) { + int frame_decoded = 0; result = ffmpeg_->avcodec_decode_audio4(codec_context_, av_frame_, &frame_decoded, &packet); - } else { - result = ffmpeg_->avcodec_receive_frame(codec_context_, av_frame_); - } - - if (result != input_buffer->size()) { - if (ffmpeg_->avcodec_version() < kAVCodecHasUniformDecodeAPI) { + if (result != input_buffer->size()) { // TODO: Consider fill it with silence. SB_DLOG(WARNING) << "avcodec_decode_audio4() failed with result: " << result @@ -180,60 +177,93 @@ void AudioDecoderImpl::Decode(const InputBuffers& input_buffers, error_cb_(kSbPlayerErrorDecode, FormatString("avcodec_decode_audio4() failed with result %d.", result)); - } else { - SB_DLOG(WARNING) << "avcodec_receive_frame() failed with result: " - << result - << " with input buffer size: " << input_buffer->size(); - error_cb_(kSbPlayerErrorDecode, - FormatString("avcodec_receive_frame() failed with result %d.", - result)); + return; + } + + if (frame_decoded != 1) { + // TODO: Adjust timestamp accordingly when decoding result is shifted. + SB_DCHECK(frame_decoded == 0); + SB_DLOG(WARNING) << "avcodec_decode_audio4()/avcodec_receive_frame() " + "returns with 0 frames decoded"; + return; } + + ProcessDecodedFrame(*input_buffer, *av_frame_); return; } - if (frame_decoded != 1) { - // TODO: Adjust timestamp accordingly when decoding result is shifted. - SB_DCHECK(frame_decoded == 0); - SB_DLOG(WARNING) << "avcodec_decode_audio4()/avcodec_receive_frame() " - "returns with 0 frames decoded"; + // Newer decode API. + const int send_packet_result = + ffmpeg_->avcodec_send_packet(codec_context_, &packet); + if (send_packet_result != 0) { + const std::string error_message = FormatString( + "avcodec_send_packet() failed with result %d.", send_packet_result); + SB_DLOG(WARNING) << error_message; + error_cb_(kSbPlayerErrorDecode, error_message); return; } + // Keep receiving frames until the decoder has processed the entire packet. + for (;;) { + result = ffmpeg_->avcodec_receive_frame(codec_context_, av_frame_); + if (result != 0) { + // We either hit an error or are done processing packet. + break; + } + ProcessDecodedFrame(*input_buffer, *av_frame_); + } + + // A return value of AVERROR(EAGAIN) signifies that the decoder needs + // another packet, so we are done processing the existing packet at that + // point. + if (result != AVERROR(EAGAIN)) { + SB_DLOG(WARNING) << "avcodec_receive_frame() failed with result: " + << result; + error_cb_( + kSbPlayerErrorDecode, + FormatString("avcodec_receive_frame() failed with result %d.", result)); + } +} + +void AudioDecoderImpl::ProcessDecodedFrame( + const InputBuffer& input_buffer, + const AVFrame& av_frame) { int decoded_audio_size = ffmpeg_->av_samples_get_buffer_size( - NULL, codec_context_->channels, av_frame_->nb_samples, + NULL, codec_context_->channels, av_frame.nb_samples, codec_context_->sample_fmt, 1); audio_stream_info_.samples_per_second = codec_context_->sample_rate; - if (decoded_audio_size > 0) { - scoped_refptr decoded_audio = new DecodedAudio( - codec_context_->channels, GetSampleType(), GetStorageType(), - input_buffer->timestamp(), - codec_context_->channels * av_frame_->nb_samples * - starboard::media::GetBytesPerSample(GetSampleType())); - if (GetStorageType() == kSbMediaAudioFrameStorageTypeInterleaved) { - memcpy(decoded_audio->data(), *av_frame_->extended_data, - decoded_audio->size_in_bytes()); - } else { - SB_DCHECK(GetStorageType() == kSbMediaAudioFrameStorageTypePlanar); - const int per_channel_size_in_bytes = - decoded_audio->size_in_bytes() / decoded_audio->channels(); - for (int i = 0; i < decoded_audio->channels(); ++i) { - memcpy(decoded_audio->data() + per_channel_size_in_bytes * i, - av_frame_->extended_data[i], per_channel_size_in_bytes); - } - decoded_audio = decoded_audio->SwitchFormatTo( - GetSampleType(), kSbMediaAudioFrameStorageTypeInterleaved); - } - decoded_audio->AdjustForDiscardedDurations( - audio_stream_info_.samples_per_second, - input_buffer->audio_sample_info().discarded_duration_from_front, - input_buffer->audio_sample_info().discarded_duration_from_back); - decoded_audios_.push(decoded_audio); - Schedule(output_cb_); - } else { + if (decoded_audio_size <= 0) { // TODO: Consider fill it with silence. SB_LOG(ERROR) << "Decoded audio frame is empty."; + return; + } + + scoped_refptr decoded_audio = new DecodedAudio( + codec_context_->channels, GetSampleType(), GetStorageType(), + input_buffer.timestamp(), + codec_context_->channels * av_frame.nb_samples * + starboard::media::GetBytesPerSample(GetSampleType())); + if (GetStorageType() == kSbMediaAudioFrameStorageTypeInterleaved) { + memcpy(decoded_audio->data(), *av_frame.extended_data, + decoded_audio->size_in_bytes()); + } else { + SB_DCHECK(GetStorageType() == kSbMediaAudioFrameStorageTypePlanar); + const int per_channel_size_in_bytes = + decoded_audio->size_in_bytes() / decoded_audio->channels(); + for (int i = 0; i < decoded_audio->channels(); ++i) { + memcpy(decoded_audio->data() + per_channel_size_in_bytes * i, + av_frame.extended_data[i], per_channel_size_in_bytes); + } + decoded_audio = decoded_audio->SwitchFormatTo( + GetSampleType(), kSbMediaAudioFrameStorageTypeInterleaved); } + decoded_audio->AdjustForDiscardedDurations( + audio_stream_info_.samples_per_second, + input_buffer.audio_sample_info().discarded_duration_from_front, + input_buffer.audio_sample_info().discarded_duration_from_back); + decoded_audios_.push(decoded_audio); + Schedule(output_cb_); } void AudioDecoderImpl::WriteEndOfStream() { diff --git a/starboard/shared/ffmpeg/ffmpeg_audio_decoder_impl.h b/starboard/shared/ffmpeg/ffmpeg_audio_decoder_impl.h index bb430cce4be4..c8f96afb4841 100644 --- a/starboard/shared/ffmpeg/ffmpeg_audio_decoder_impl.h +++ b/starboard/shared/ffmpeg/ffmpeg_audio_decoder_impl.h @@ -64,6 +64,11 @@ class AudioDecoderImpl : public AudioDecoder, void InitializeCodec(); void TeardownCodec(); + // Processes decoded (PCM) audio data received from FFmpeg. The audio data is + // ultimately enqueued in decoded_audios_. + void ProcessDecodedFrame(const InputBuffer& input_buffer, + const AVFrame& av_frame); + static const int kMaxDecodedAudiosSize = 64; FFMPEGDispatch* ffmpeg_; diff --git a/starboard/shared/ffmpeg/ffmpeg_dispatch.h b/starboard/shared/ffmpeg/ffmpeg_dispatch.h index 852c47167fd4..082444a6f0e5 100644 --- a/starboard/shared/ffmpeg/ffmpeg_dispatch.h +++ b/starboard/shared/ffmpeg/ffmpeg_dispatch.h @@ -48,7 +48,12 @@ constexpr int kAVUtilSupportsBufferCreate = 3409920; // https://github.com/FFmpeg/FFmpeg/blob/70d25268c21cbee5f08304da95be1f647c630c15/doc/APIchanges#L195 // avcodec_decode_audio4 and avcodec_decode_video2 replaced by // avcodec_receive_frame() -constexpr int kAVCodecHasUniformDecodeAPI = 3940198; +// +// The APIs were removed in this change: +// https://github.com/FFmpeg/FFmpeg/commit/7c1f347b184b6738abdc22fdcda40baa9f932522#diff-76418b674d0db8d5027d2e1e325dbe9b92b65b09d9f20cdd305ad14b0e46562d +// (note the values in libavcodec/version.h) +// AV_VERSION_INT(58, 137, 100) +constexpr int kAVCodecHasUniformDecodeAPI = 3836260; // https://github.com/FFmpeg/FFmpeg/blob/70d25268c21cbee5f08304da95be1f647c630c15/doc/APIchanges#L86 // no longer required @@ -114,6 +119,7 @@ class FFMPEGDispatch { AVFrame* picture, int* got_picture_ptr, const AVPacket* avpkt); + int (*avcodec_send_packet)(AVCodecContext* avctx, const AVPacket* avpkt); int (*avcodec_receive_frame)(AVCodecContext* avctx, AVFrame* frame); void (*avcodec_flush_buffers)(AVCodecContext* avctx); AVFrame* (*avcodec_alloc_frame)(void); diff --git a/starboard/shared/ffmpeg/ffmpeg_dynamic_load_dispatch_impl.cc b/starboard/shared/ffmpeg/ffmpeg_dynamic_load_dispatch_impl.cc index ffea213c1846..3648b8a34eb4 100644 --- a/starboard/shared/ffmpeg/ffmpeg_dynamic_load_dispatch_impl.cc +++ b/starboard/shared/ffmpeg/ffmpeg_dynamic_load_dispatch_impl.cc @@ -311,6 +311,7 @@ void FFMPEGDispatchImpl::LoadSymbols() { INITSYMBOL(avcodec_, avcodec_decode_audio4); INITSYMBOL(avcodec_, avcodec_decode_video2); } else { + INITSYMBOL(avcodec_, avcodec_send_packet); INITSYMBOL(avcodec_, avcodec_receive_frame); } diff --git a/starboard/shared/ffmpeg/ffmpeg_linked_dispatch_impl.cc b/starboard/shared/ffmpeg/ffmpeg_linked_dispatch_impl.cc index b2fb431a66b8..c491f8e8e189 100644 --- a/starboard/shared/ffmpeg/ffmpeg_linked_dispatch_impl.cc +++ b/starboard/shared/ffmpeg/ffmpeg_linked_dispatch_impl.cc @@ -77,8 +77,13 @@ void LoadSymbols(FFMPEGDispatch* ffmpeg) { INITSYMBOL(avcodec_close); INITSYMBOL(avcodec_open2); INITSYMBOL(av_init_packet); +#if LIBAVCODEC_VERSION_INT < AV_VERSION_INT(58, 137, 100) INITSYMBOL(avcodec_decode_audio4); INITSYMBOL(avcodec_decode_video2); +#else + INITSYMBOL(avcodec_send_packet); + INITSYMBOL(avcodec_receive_frame); +#endif // LIBAVCODEC_VERSION_INT < AV_VERSION_INT(58, 137, 100) INITSYMBOL(avcodec_flush_buffers); #if LIBAVUTIL_VERSION_INT < LIBAVUTIL_VERSION_52_8 INITSYMBOL(avcodec_alloc_frame); diff --git a/starboard/shared/ffmpeg/ffmpeg_video_decoder_impl.cc b/starboard/shared/ffmpeg/ffmpeg_video_decoder_impl.cc index 3503e7a07a63..5e6c6d6e3c2d 100644 --- a/starboard/shared/ffmpeg/ffmpeg_video_decoder_impl.cc +++ b/starboard/shared/ffmpeg/ffmpeg_video_decoder_impl.cc @@ -19,6 +19,8 @@ #include +#include + #include "starboard/common/string.h" #include "starboard/linux/shared/decode_target_internal.h" #include "starboard/thread.h" @@ -270,32 +272,68 @@ bool VideoDecoderImpl::DecodePacket(AVPacket* packet) { } else { ffmpeg_->avcodec_get_frame_defaults(av_frame_); } - int frame_decoded = 0; int decode_result = 0; if (ffmpeg_->avcodec_version() < kAVCodecHasUniformDecodeAPI) { + // Old decode API. + int frame_decoded = 0; decode_result = ffmpeg_->avcodec_decode_video2(codec_context_, av_frame_, &frame_decoded, packet); - } else { - decode_result = ffmpeg_->avcodec_receive_frame(codec_context_, av_frame_); + if (decode_result < 0) { + SB_DLOG(ERROR) << "avcodec_decode_video2() failed with result " + << decode_result; + error_cb_(kSbPlayerErrorDecode, + FormatString("avcodec_decode_video2() failed with result %d.", + decode_result)); + error_occurred_ = true; + return false; + } + + if (frame_decoded == 0) { + return false; + } + + return ProcessDecodedFrame(*av_frame_); } - if (decode_result < 0) { - SB_DLOG(ERROR) - << "avcodec_decode_video2()/avcodec_receive_frame() failed with result " - << decode_result; - error_cb_(kSbPlayerErrorDecode, - FormatString("avcodec_decode_video2()/avcodec_receive_frame() " - "failed with result %d.", - decode_result)); - error_occurred_ = true; + // Newer decode API. + const int send_packet_result = + ffmpeg_->avcodec_send_packet(codec_context_, packet); + if (send_packet_result != 0) { + const std::string error_message = FormatString( + "avcodec_send_packet() failed with result %d.", send_packet_result); + SB_DLOG(WARNING) << error_message; + error_cb_(kSbPlayerErrorDecode, error_message); return false; } - if (frame_decoded == 0) { + + // Keep receiving frames until the decoder has processed the entire packet. + for (;;) { + decode_result = ffmpeg_->avcodec_receive_frame(codec_context_, av_frame_); + if (decode_result != 0) { + // We either hit an error or are done processing packet. + break; + } + + if (!ProcessDecodedFrame(*av_frame_)) { + return false; + } + } + + // A return value of AVERROR(EAGAIN) signifies that the decoder needs + // another packet, so we are done processing the existing packet at that + // point. + if (decode_result != AVERROR(EAGAIN)) { + SB_DLOG(WARNING) << "avcodec_receive_frame() failed with result: " + << decode_result; return false; } - if (av_frame_->opaque == NULL) { + return true; +} + +bool VideoDecoderImpl::ProcessDecodedFrame(const AVFrame& av_frame) { + if (av_frame.opaque == NULL) { SB_DLOG(ERROR) << "Video frame was produced yet has invalid frame data."; error_cb_(kSbPlayerErrorDecode, "Video frame was produced yet has invalid frame data."); @@ -303,21 +341,21 @@ bool VideoDecoderImpl::DecodePacket(AVPacket* packet) { return false; } - int codec_aligned_width = av_frame_->width; - int codec_aligned_height = av_frame_->height; + int codec_aligned_width = av_frame.width; + int codec_aligned_height = av_frame.height; int codec_linesize_align[AV_NUM_DATA_POINTERS]; ffmpeg_->avcodec_align_dimensions2(codec_context_, &codec_aligned_width, &codec_aligned_height, codec_linesize_align); - int y_pitch = AlignUp(av_frame_->width, codec_linesize_align[0] * 2); - int uv_pitch = av_frame_->linesize[1]; + int y_pitch = AlignUp(av_frame.width, codec_linesize_align[0] * 2); + int uv_pitch = av_frame.linesize[1]; const int kBitDepth = 8; scoped_refptr frame = CpuVideoFrame::CreateYV12Frame( - kBitDepth, av_frame_->width, av_frame_->height, y_pitch, uv_pitch, - av_frame_->reordered_opaque, av_frame_->data[0], av_frame_->data[1], - av_frame_->data[2]); + kBitDepth, av_frame.width, av_frame.height, y_pitch, uv_pitch, + av_frame.reordered_opaque, av_frame.data[0], av_frame.data[1], + av_frame.data[2]); bool result = true; if (output_mode_ == kSbPlayerOutputModeDecodeToTexture) { diff --git a/starboard/shared/ffmpeg/ffmpeg_video_decoder_impl.h b/starboard/shared/ffmpeg/ffmpeg_video_decoder_impl.h index 673cf7ee5b35..73c37ffad1f0 100644 --- a/starboard/shared/ffmpeg/ffmpeg_video_decoder_impl.h +++ b/starboard/shared/ffmpeg/ffmpeg_video_decoder_impl.h @@ -107,6 +107,12 @@ class VideoDecoderImpl : public VideoDecoder { void UpdateDecodeTarget_Locked(const scoped_refptr& frame); + // Processes a decoded video frame received from FFmpeg. The frame is + // ultimately passed to decoder_status_cb_. + // + // Returns false if the frame contains invalid data. + bool ProcessDecodedFrame(const AVFrame& av_frame); + FFMPEGDispatch* ffmpeg_; // |video_codec_| will be initialized inside ctor and won't be changed during From bf9373676f97022af808a07c45e30b67f883a304 Mon Sep 17 00:00:00 2001 From: Holden Warriner Date: Mon, 25 Mar 2024 13:44:12 -0400 Subject: [PATCH 4/4] Add Cobalt Telemetry for the Loader App (#2667) A LoaderAppMetrics Starboard extension is added so that measurements taken by the Loader App can be reliably accessed by Cobalt once loaded. This PR just adds one metric for now, for b/329458881, but this pattern and extension can be used for b/329445690 and other Loader App metrics. b/329458881 Change-Id: I940d07b058d197afa2ad8b1160eba469f1055f8a --- cobalt/browser/application.cc | 19 ++++++ starboard/extension/extension_test.cc | 22 +++++++ starboard/extension/loader_app_metrics.h | 64 +++++++++++++++++++ starboard/linux/shared/BUILD.gn | 2 + .../linux/shared/system_get_extensions.cc | 5 ++ .../shared/starboard/loader_app_metrics.cc | 50 +++++++++++++++ .../shared/starboard/loader_app_metrics.h | 28 ++++++++ third_party/crashpad/wrapper/wrapper.cc | 38 +++++++++-- .../histograms/metadata/cobalt/enums.xml | 11 ++++ .../histograms/metadata/cobalt/histograms.xml | 11 ++++ 10 files changed, 243 insertions(+), 7 deletions(-) create mode 100644 starboard/extension/loader_app_metrics.h create mode 100644 starboard/shared/starboard/loader_app_metrics.cc create mode 100644 starboard/shared/starboard/loader_app_metrics.h diff --git a/cobalt/browser/application.cc b/cobalt/browser/application.cc index b60de64f1d2d..1dc3fc9a5175 100644 --- a/cobalt/browser/application.cc +++ b/cobalt/browser/application.cc @@ -23,6 +23,7 @@ #include "base/command_line.h" #include "base/lazy_instance.h" #include "base/logging.h" +#include "base/metrics/histogram_functions.h" #include "base/metrics/statistics_recorder.h" #include "base/metrics/user_metrics.h" #include "base/optional.h" @@ -82,6 +83,7 @@ #include "starboard/event.h" #include "starboard/extension/crash_handler.h" #include "starboard/extension/installation_manager.h" +#include "starboard/extension/loader_app_metrics.h" #include "starboard/system.h" #include "url/gurl.h" @@ -617,6 +619,22 @@ void AddCrashLogApplicationState(base::ApplicationState state) { << "required version, so not sending application state."; } +void RecordLoaderAppMetrics() { + auto metrics_extension = + static_cast( + SbSystemGetExtension(kStarboardExtensionLoaderAppMetricsName)); + if (metrics_extension && + strcmp(metrics_extension->name, + kStarboardExtensionLoaderAppMetricsName) == 0 && + metrics_extension->version >= 1) { + base::UmaHistogramEnumeration( + "Cobalt.LoaderApp.CrashpadInstallationStatus", + metrics_extension->GetCrashpadInstallationStatus()); + LOG(INFO) << "Recorded sample for " + << "Cobalt.LoaderApp.CrashpadInstallationStatus"; + } +} + } // namespace // Static user logs @@ -1023,6 +1041,7 @@ Application::Application(const base::Closure& quit_closure, bool should_preload, #endif // ENABLE_DEBUG_COMMAND_LINE_SWITCHES AddCrashLogApplicationState(base::kApplicationStateStarted); + RecordLoaderAppMetrics(); } Application::~Application() { diff --git a/starboard/extension/extension_test.cc b/starboard/extension/extension_test.cc index 4308cb8608a1..caa0cceb08c6 100644 --- a/starboard/extension/extension_test.cc +++ b/starboard/extension/extension_test.cc @@ -24,6 +24,7 @@ #include "starboard/extension/ifa.h" #include "starboard/extension/installation_manager.h" #include "starboard/extension/javascript_cache.h" +#include "starboard/extension/loader_app_metrics.h" #include "starboard/extension/media_session.h" #include "starboard/extension/memory_mapped_file.h" #include "starboard/extension/platform_info.h" @@ -503,5 +504,26 @@ TEST(ExtensionTest, PlayerSetMaxVideoInputSize) { << "Extension struct should be a singleton"; } +TEST(ExtensionTest, LoaderAppMetrics) { + typedef StarboardExtensionLoaderAppMetricsApi ExtensionApi; + const char* kExtensionName = kStarboardExtensionLoaderAppMetricsName; + + const ExtensionApi* extension_api = + static_cast(SbSystemGetExtension(kExtensionName)); + if (!extension_api) { + return; + } + + EXPECT_STREQ(extension_api->name, kExtensionName); + EXPECT_EQ(extension_api->version, 1u); + EXPECT_NE(extension_api->SetCrashpadInstallationStatus, nullptr); + EXPECT_NE(extension_api->GetCrashpadInstallationStatus, nullptr); + + const ExtensionApi* second_extension_api = + static_cast(SbSystemGetExtension(kExtensionName)); + EXPECT_EQ(second_extension_api, extension_api) + << "Extension struct should be a singleton"; +} + } // namespace extension } // namespace starboard diff --git a/starboard/extension/loader_app_metrics.h b/starboard/extension/loader_app_metrics.h new file mode 100644 index 000000000000..abdb120a2de7 --- /dev/null +++ b/starboard/extension/loader_app_metrics.h @@ -0,0 +1,64 @@ +// Copyright 2024 The Cobalt Authors. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef STARBOARD_EXTENSION_LOADER_APP_METRICS_H_ +#define STARBOARD_EXTENSION_LOADER_APP_METRICS_H_ + +#include + +#ifdef __cplusplus +extern "C" { +#endif + +#define kStarboardExtensionLoaderAppMetricsName \ + "dev.cobalt.extension.LoaderAppMetrics" + +// These values are persisted to logs. Entries should not be renumbered and +// numeric values should never be reused. Must be kept in sync with the +// corresponding definition in +// tools/metrics/histograms/metadata/cobalt/enums.xml. +typedef enum CrashpadInstallationStatus { + // The enumerators below this point were added in version 1 or later. + kUnknown = 0, + kSucceeded = 1, + kFailedCrashpadHandlerBinaryNotFound = 2, + kFailedDatabaseInitializationFailed = 3, + kFailedSignalHandlerInstallationFailed = 4, + kMaxValue = kFailedSignalHandlerInstallationFailed +} CrashpadInstallationStatus; + +typedef struct StarboardExtensionLoaderAppMetricsApi { + // Name should be the string |kStarboardExtensionLoaderAppMetricsName|. + // This helps to validate that the extension API is correct. + const char* name; + + // This specifies the version of the API that is implemented. + uint32_t version; + + // The fields below this point were added in version 1 or later. + + // The accessors and mutators below are assumed to be called from the same + // thread: Cobalt's application thread. + + void (*SetCrashpadInstallationStatus)(CrashpadInstallationStatus status); + + CrashpadInstallationStatus (*GetCrashpadInstallationStatus)(); + +} StarboardExtensionLoaderAppMetricsApi; + +#ifdef __cplusplus +} // extern "C" +#endif + +#endif // STARBOARD_EXTENSION_LOADER_APP_METRICS_H_ diff --git a/starboard/linux/shared/BUILD.gn b/starboard/linux/shared/BUILD.gn index 93ad2884477f..81b14120e942 100644 --- a/starboard/linux/shared/BUILD.gn +++ b/starboard/linux/shared/BUILD.gn @@ -293,6 +293,8 @@ static_library("starboard_platform_sources") { "//starboard/shared/starboard/file_storage/storage_get_record_size.cc", "//starboard/shared/starboard/file_storage/storage_open_record.cc", "//starboard/shared/starboard/file_storage/storage_read_record.cc", + "//starboard/shared/starboard/loader_app_metrics.cc", + "//starboard/shared/starboard/loader_app_metrics.h", "//starboard/shared/starboard/log_mutex.cc", "//starboard/shared/starboard/log_mutex.h", "//starboard/shared/starboard/log_raw_dump_stack.cc", diff --git a/starboard/linux/shared/system_get_extensions.cc b/starboard/linux/shared/system_get_extensions.cc index efc9169f829d..0e1bf0662dba 100644 --- a/starboard/linux/shared/system_get_extensions.cc +++ b/starboard/linux/shared/system_get_extensions.cc @@ -21,6 +21,7 @@ #include "starboard/extension/enhanced_audio.h" #include "starboard/extension/free_space.h" #include "starboard/extension/ifa.h" +#include "starboard/extension/loader_app_metrics.h" #include "starboard/extension/memory_mapped_file.h" #include "starboard/extension/platform_service.h" #include "starboard/extension/time_zone.h" @@ -33,6 +34,7 @@ #include "starboard/shared/posix/memory_mapped_file.h" #include "starboard/shared/starboard/application.h" #include "starboard/shared/starboard/crash_handler.h" +#include "starboard/shared/starboard/loader_app_metrics.h" #if SB_IS(EVERGREEN_COMPATIBLE) #include "starboard/elf_loader/evergreen_config.h" #endif @@ -86,5 +88,8 @@ const void* SbSystemGetExtension(const char* name) { return starboard::shared::GetIfaApi(); } #endif // SB_API_VERSION < 14 + if (strcmp(name, kStarboardExtensionLoaderAppMetricsName) == 0) { + return starboard::shared::starboard::GetLoaderAppMetricsApi(); + } return NULL; } diff --git a/starboard/shared/starboard/loader_app_metrics.cc b/starboard/shared/starboard/loader_app_metrics.cc new file mode 100644 index 000000000000..6b7b8d7d6477 --- /dev/null +++ b/starboard/shared/starboard/loader_app_metrics.cc @@ -0,0 +1,50 @@ +// Copyright 2024 The Cobalt Authors. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "starboard/shared/starboard/loader_app_metrics.h" + +#include "starboard/extension/loader_app_metrics.h" + +namespace starboard { +namespace shared { +namespace starboard { + +namespace { + +// Thread safety isn't needed for this global variable since the extension's +// interface specifies that all accesses and mutations must be from the same +// thread. +static CrashpadInstallationStatus g_crashpad_installation_status; + +void SetCrashpadInstallationStatus(CrashpadInstallationStatus status) { + g_crashpad_installation_status = status; +} + +CrashpadInstallationStatus GetCrashpadInstallationStatus() { + return g_crashpad_installation_status; +} + +const StarboardExtensionLoaderAppMetricsApi kLoaderAppMetricsApi = { + kStarboardExtensionLoaderAppMetricsName, 1, &SetCrashpadInstallationStatus, + &GetCrashpadInstallationStatus}; + +} // namespace + +const void* GetLoaderAppMetricsApi() { + return &kLoaderAppMetricsApi; +} + +} // namespace starboard +} // namespace shared +} // namespace starboard diff --git a/starboard/shared/starboard/loader_app_metrics.h b/starboard/shared/starboard/loader_app_metrics.h new file mode 100644 index 000000000000..ede4cef01b75 --- /dev/null +++ b/starboard/shared/starboard/loader_app_metrics.h @@ -0,0 +1,28 @@ +// Copyright 2024 The Cobalt Authors. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef STARBOARD_SHARED_STARBOARD_LOADER_APP_METRICS_H_ +#define STARBOARD_SHARED_STARBOARD_LOADER_APP_METRICS_H_ + +namespace starboard { +namespace shared { +namespace starboard { + +const void* GetLoaderAppMetricsApi(); + +} // namespace starboard +} // namespace shared +} // namespace starboard + +#endif // STARBOARD_SHARED_STARBOARD_LOADER_APP_METRICS_H_ diff --git a/third_party/crashpad/wrapper/wrapper.cc b/third_party/crashpad/wrapper/wrapper.cc index e8da56a77533..94ed7f5f8643 100644 --- a/third_party/crashpad/wrapper/wrapper.cc +++ b/third_party/crashpad/wrapper/wrapper.cc @@ -26,6 +26,7 @@ #include "starboard/common/system_property.h" #include "starboard/configuration_constants.h" #include "starboard/directory.h" +#include "starboard/extension/loader_app_metrics.h" #include "starboard/file.h" #include "starboard/system.h" #include "third_party/crashpad/snapshot/sanitized/sanitization_information.h" @@ -187,6 +188,18 @@ std::map GetPlatformInfo() { return platform_info; } +void RecordStatus(CrashpadInstallationStatus status) { + auto metrics_extension = + static_cast( + SbSystemGetExtension(kStarboardExtensionLoaderAppMetricsName)); + if (metrics_extension && + strcmp(metrics_extension->name, + kStarboardExtensionLoaderAppMetricsName) == 0 && + metrics_extension->version >= 1) { + metrics_extension->SetCrashpadInstallationStatus(status); + } +} + } // namespace void InstallCrashpadHandler(const std::string& ca_certificates_path) { @@ -196,6 +209,8 @@ void InstallCrashpadHandler(const std::string& ca_certificates_path) { if (!SbFileExists(handler_path.value().c_str())) { LOG(ERROR) << "crashpad_handler not at expected location of " << handler_path.value(); + RecordStatus( + CrashpadInstallationStatus::kFailedCrashpadHandlerBinaryNotFound); return; } @@ -213,6 +228,8 @@ void InstallCrashpadHandler(const std::string& ca_certificates_path) { if (!InitializeCrashpadDatabase(database_directory_path)) { LOG(ERROR) << "Failed to initialize Crashpad database"; + RecordStatus( + CrashpadInstallationStatus::kFailedDatabaseInitializationFailed); // As we investigate b/329458881 we may find that it's safe to continue with // installation of the Crashpad handler here with the hope that the handler, @@ -224,16 +241,23 @@ void InstallCrashpadHandler(const std::string& ca_certificates_path) { client->SetUnhandledSignals({}); - client->StartHandlerAtCrash(handler_path, - database_directory_path, - default_metrics_dir, - kUploadUrl, - ca_certificates_path, - default_annotations, - default_arguments); + if (!client->StartHandlerAtCrash(handler_path, + database_directory_path, + default_metrics_dir, + kUploadUrl, + ca_certificates_path, + default_annotations, + default_arguments)) { + LOG(ERROR) << "Failed to install the signal handler"; + RecordStatus( + CrashpadInstallationStatus::kFailedSignalHandlerInstallationFailed); + return; + } ::crashpad::SanitizationInformation sanitization_info = {0, 0, 0, 1}; client->SendSanitizationInformationToHandler(sanitization_info); + + RecordStatus(CrashpadInstallationStatus::kSucceeded); } bool AddEvergreenInfoToCrashpad(EvergreenInfo evergreen_info) { diff --git a/tools/metrics/histograms/metadata/cobalt/enums.xml b/tools/metrics/histograms/metadata/cobalt/enums.xml index 474c9e75e775..f54fc6fdc46a 100644 --- a/tools/metrics/histograms/metadata/cobalt/enums.xml +++ b/tools/metrics/histograms/metadata/cobalt/enums.xml @@ -69,6 +69,17 @@ https://chromium.googlesource.com/chromium/src.git/+/HEAD/tools/metrics/histogra + + + Possible status of Crashpad installation by the Loader App + + + + + + + + diff --git a/tools/metrics/histograms/metadata/cobalt/histograms.xml b/tools/metrics/histograms/metadata/cobalt/histograms.xml index 33b968ce75b5..13f4d9d71a57 100644 --- a/tools/metrics/histograms/metadata/cobalt/histograms.xml +++ b/tools/metrics/histograms/metadata/cobalt/histograms.xml @@ -198,6 +198,17 @@ Always run the pretty print utility on this file after editing: + + + + hwarriner@google.com + cobalt-team@google.com + + Status of Crashpad installation by the Loader App. + + +