[linux] Implement libiamf-based IAMF decoder #4221
Conversation
Also clean up logs and reformat decoder.
osagie98
requested review from
sideb0ard,
jasonzhangxx,
xiaomings,
zhongqiliang and
borongc
| decoder_ = IAMF_decoder_open(); | ||
| if (!decoder_) { | ||
| SB_LOG(ERROR) << "Error creating libiamf decoder"; | ||
| ReportError("Error creating libiamf decoder"); |
There was a problem hiding this comment.
error_cb_ hasn't been set yet.
There was a problem hiding this comment.
Consider checking is_valid() inside PlayerComponentsFactory as we no longer report an error here.
| if (bytes_read == str_size) { | ||
| if (buf[bytes_read - 1] != '\0') { | ||
| return -1; | ||
| } | ||
| } else { | ||
| if (buf[bytes_read] != '\0') { | ||
| return -1; | ||
| } | ||
| } |
There was a problem hiding this comment.
Is it possible for you to elaborate when these failure cases may happen?
There was a problem hiding this comment.
Simplified the implementation. This should only fail if a string is read that doesn't have a null terminator, which could happen if we read up to the end of the buffer, or the full 128 bytes, whichever is first.
| bool HasSupportedIamfProfile(const IamfMimeUtil* mime_util) { | ||
| return mime_util->primary_profile() == kIamfProfileSimple || | ||
| mime_util->additional_profile() == kIamfProfileBase; | ||
| } |
There was a problem hiding this comment.
This is different than the previous implementation, where primary profile can be base and additional profile can be simple. Just wanted to double check if this is expected.
| #if SB_API_VERSION >= 15 && ENABLE_IAMF_DECODE | ||
| } else if (audio_stream_info.codec == kSbMediaAudioCodecIamf) { | ||
| SB_LOG(INFO) << "Playing audio using IamfAudioDecoder."; | ||
| return std::unique_ptr<AudioDecoder>( |
There was a problem hiding this comment.
Do we need to check if it's valid?
| return false; | ||
| } | ||
|
|
||
| if (samples_decoded > info.num_samples) { |
There was a problem hiding this comment.
Can samples_decoded be info.num_samples - 1?
There was a problem hiding this comment.
From what I've seen, IAMF_decoder_decode() should decode every sample indicated by the Codec config OBU if we pass it a complete access unit, which we do here.
Context:
https://github.com/AOMediaCodec/libiamf/blob/780ec7a112d4cea6350504e65d164526e01938a2/code/include/IAMF_decoder.h#L97
| samples_per_second_ = info.sample_rate; | ||
| } | ||
|
|
||
| // TODO: Enable partial audio once float32 pcm output is available. |
There was a problem hiding this comment.
Given the TODO here, what would happen for the following AdjustForDiscardedDurations() call when discarded_duration_from_front or discarded_duration_from_back isn't 0?
There was a problem hiding this comment.
I think I had it commented out earlier and at some point removed the comment. I removed that section entirely, partial audio won't be supported at all at this time.
| stream_ended_ = false; | ||
| decoded_audios_ = std::queue<scoped_refptr<DecodedAudio>>(); // clear | ||
|
|
||
| CancelPendingJobs(); |
There was a problem hiding this comment.
nit: there aren't any pending jobs.
| if (bytes_read == max_bytes_to_read) { | ||
| // Ensure that the string is null terminated. | ||
| if (buf[bytes_read] != '\0') { | ||
| return false; |
| return ++bytes_read; | ||
| } | ||
|
|
||
| int pos_ = 0; |
There was a problem hiding this comment.
nit: move pos_ below the two variables as they will be initialized in the ctor and never modified after.
|
|
||
| BufferReader reader(input_buffer->data(), input_buffer->size()); | ||
|
|
||
| while (!info->is_valid() && reader.pos() < reader.size()) { |
There was a problem hiding this comment.
Ideally we should have a fast forward mode that skips all config OBUs so we can return the encoded data. The detailed config OBU info is only used to configure the decoder once when it's created.
Feel free to add it as a TODO and finish this in future.
Implements a libiamf-based audio decoder.
Decoder specific code is guarded behind a GN flag enable_iamf_decode, which controls the ENABLE_IAMF_DECODE macro. enable_iamf_decode is off by default, so the new code doesn't build.
b/341792042