For our HLS implementation, We can retrieve the demuxed sample composed by AnnexB format from Exoplayer and we can easily prepend the SPS and PPS(which is formed by AnnexB starting code) to the sample. Android MediaCodec accept this format of sample even SPS/PPS changed dynamically. So we decided to bypass the data to the Android Decoder Module instead of using H264Converter.

Hi jya, Per comment 0 said, we might want to bypass the demuxed sample to Android decoder, but I'm not sure it is the best way to tell the PDM to avoid creating H264Converter. Maybe we could have a f2f discussion when you are in Taipei!

Comment on attachment 8864446 [details] Bug 1361984 - Fix the logic of AnnexB::ConvertSampleToAnnexB that checks the sample's extradata field even the sample is AVCC format converted by ConvertSampleToAVCC from AnnexB format. Hi jya, I upload this patch that is not ready for review but needs your feedback if it is OK for these modification. I will remove all the debugging logs when I'm ready to enter review process. The main modification is in AnnexB.cpp -> AnnexB::ConvertSampleToAnnexB Please take a look. Thanks.

Comment on attachment 8864446 [details] Bug 1361984 - Fix the logic of AnnexB::ConvertSampleToAnnexB that checks the sample's extradata field even the sample is AVCC format converted by ConvertSampleToAVCC from AnnexB format. https://reviewboard.mozilla.org/r/136128/#review140878 ::: dom/media/platforms/PDMFactory.cpp:295 (Diff revision 3) > RESULT_DETAIL("Decoder configuration error, expected audio or video.")); > return nullptr; > } > printf_stderr("config.GetAsVideoInfo()->mNeedConversion = %d", config.GetAsVideoInfo()->mNeedConversion); > if (MP4Decoder::IsH264(config.mMimeType) && > - !aParams.mUseNullDecoder && > + !aParams.mUseNullDecoder) { out of scope, I'm not sure how that has any relations to the current code. and where does thst printf comes from? Is that from an earlier patch? ::: dom/media/platforms/wrappers/H264Converter.h (Diff revision 3) > private: > // Will create the required MediaDataDecoder if need AVCC and we have a SPS NAL. > // Returns NS_ERROR_FAILURE if error is permanent and can't be recovered and > // will set mError accordingly. > - nsresult CreateDecoder(const VideoInfo& aConfig, > + nsresult CreateDecoder(const VideoInfo& aConfig, DecoderDoctorDiagnostics* aDiagnostics); > - DecoderDoctorDiagnostics* aDiagnostics); out of scope ::: dom/media/platforms/wrappers/H264Converter.cpp:21 (Diff revision 3) > #include "PDMFactory.h" > > +// TODO : Remove this when we finish the develop > +#include <string> > +#include <sstream> > +#include <iomanip> remove

Comment on attachment 8864446 [details] Bug 1361984 - Fix the logic of AnnexB::ConvertSampleToAnnexB that checks the sample's extradata field even the sample is AVCC format converted by ConvertSampleToAVCC from AnnexB format. https://reviewboard.mozilla.org/r/136128/#review140880 this is obviously workmin progress code, plenty of todo and printf. not sure its ready for review

Oops, I forget to manually remove the review request, sorry. I intended to do the f? for these patch. Sorry for confusing.

Comment on attachment 8864446 [details] Bug 1361984 - Fix the logic of AnnexB::ConvertSampleToAnnexB that checks the sample's extradata field even the sample is AVCC format converted by ConvertSampleToAVCC from AnnexB format. https://reviewboard.mozilla.org/r/136128/#review141106 ::: media/libstagefright/binding/AnnexB.cpp:46 (Diff revision 3) > MOZ_ASSERT(aSample->Data()); > + auto s = bin2hex(aSample->Data(), aSample->Size()); > + printf_stderr("[AnnexB][ConvertSampleToAnnexB] before CST4bytesAVCC : %s", > + s.c_str()); > > - if (!ConvertSampleTo4BytesAVCC(aSample)) { > + if (aSample->mExtraData && aSample->mExtraData->Length() != 0 && !ConvertSampleTo4BytesAVCC(aSample)) { if (IsAVCC() && ... ::: media/libstagefright/binding/AnnexB.cpp:92 (Diff revision 3) > + printf_stderr("[AnnexB][ConvertSampleToAnnexB]replace, return false"); > return false; > } > - > + printf_stderr("[AnnexB][ConvertSampleToAnnexB] after if (!samplewriter->Replace(tmp.begin(), tmp.length())) {"); > // Prepend the Annex B NAL with SPS and PPS tables to keyframes. > - if (aAddSPS && aSample->mKeyframe) { > + if (aAddSPS && aSample->mKeyframe && aSample->mExtraData) { && IsAVCC()) ::: media/libstagefright/binding/AnnexB.cpp:273 (Diff revision 3) > + printf_stderr("[AnnexB][ConvertSampleToAVCC]"); > if (IsAVCC(aSample)) { > + printf_stderr("[AnnexB][ConvertSampleToAVCC]IsAVCC"); > return ConvertSampleTo4BytesAVCC(aSample); > } > if (!IsAnnexB(aSample)) { Where is IsAnnexB?

Cleanup the code for review. Attach the treeherder result for reference. https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf471fcec37e7219d045d811c3e1e245773c993d

Comment on attachment 8864446 [details] Bug 1361984 - Fix the logic of AnnexB::ConvertSampleToAnnexB that checks the sample's extradata field even the sample is AVCC format converted by ConvertSampleToAVCC from AnnexB format. https://reviewboard.mozilla.org/r/136128/#review141494

Pushed by jacheng@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bbea5fa74727 Fix the logic of AnnexB::ConvertSampleToAnnexB that checks the sample's extradata field even the sample is AVCC format converted by ConvertSampleToAVCC from AnnexB format. r=jya

Hi jya, Does the dumexed sample from mp4demuxer always returned the sample with extra data or eztra data only with key frame? It seems my patch makes the avcc sample without extra data returned. Thanks

Of course it's broken... You have: https://dxr.mozilla.org/mozilla-central/source/media/libstagefright/binding/AnnexB.cpp#66 if (aAddSPS && aSample->mKeyframe && IsAVCC(aSample)) { this wasn't in what I reviewed: you had: if (aAddSPS && aSample->mKeyframe && aSample->mExtraData) { This test right after the data has been converted from AVCC to AnnexB. So IsAVCC(aSample) will now always be false. and the SPS/PPS will never be added to the data... The result can't be decoded.

Backed out on request from jya: https://hg.mozilla.org/mozilla-central/rev/e66dedabe582ba7b394aee4f89ed70fe389b3c46

Hi jya, The main root cause for this issue is that !IsAVCC(aSample) is not equivalent to IsAnnexB(aSample) for the sample which starts with 0x000001 or 0x00000001. So I've modified the logic and tested it on Windows and Android platform. Please review it again and sorry for causing this regression. Thanks.

Comment on attachment 8864446 [details] Bug 1361984 - Fix the logic of AnnexB::ConvertSampleToAnnexB that checks the sample's extradata field even the sample is AVCC format converted by ConvertSampleToAVCC from AnnexB format. https://reviewboard.mozilla.org/r/136128/#review142456 ::: media/libstagefright/binding/AnnexB.cpp:25 (Diff revisions 4 - 5) > bool > AnnexB::ConvertSampleToAnnexB(mozilla::MediaRawData* aSample, bool aAddSPS) > { > MOZ_ASSERT(aSample); > > - if (IsAnnexB(aSample)) { > + // AVCC sample may start with 0x00000001 or 0x000001 which causees the that comment doesn't make sense to me. That just can't be... AVCC nal starts with their size. A NAL size of 1 isn't possible with h264.

Comment on attachment 8864446 [details] Bug 1361984 - Fix the logic of AnnexB::ConvertSampleToAnnexB that checks the sample's extradata field even the sample is AVCC format converted by ConvertSampleToAVCC from AnnexB format. https://reviewboard.mozilla.org/r/136128/#review142458 ::: media/libstagefright/binding/AnnexB.cpp:68 (Diff revision 5) > if (!samplewriter->Replace(tmp.begin(), tmp.length())) { > return false; > } > > // Prepend the Annex B NAL with SPS and PPS tables to keyframes. > - if (aAddSPS && aSample->mKeyframe) { > + if (aAddSPS && aSample->mKeyframe && aSample->mExtraData) { this assumes that aSample->mExtraData is valid. Something IsAVCC would have previously checked on. If it's not valid, that ConvertExtraDataToAnnexB may return nullptr, in which case we will now get a null dereference.

Comment on attachment 8864446 [details] Bug 1361984 - Fix the logic of AnnexB::ConvertSampleToAnnexB that checks the sample's extradata field even the sample is AVCC format converted by ConvertSampleToAVCC from AnnexB format. https://reviewboard.mozilla.org/r/136128/#review142456 > that comment doesn't make sense to me. > > That just can't be... > > AVCC nal starts with their size. A NAL size of 1 isn't possible with h264. I think that doensnt mean the nalsize is 1, it just the pattern that the sample starts with 0x000001xxxxxxxxxxxxxxx. Does it make sense?

Comment on attachment 8864446 [details] Bug 1361984 - Fix the logic of AnnexB::ConvertSampleToAnnexB that checks the sample's extradata field even the sample is AVCC format converted by ConvertSampleToAVCC from AnnexB format. https://reviewboard.mozilla.org/r/136128/#review142458 > this assumes that aSample->mExtraData is valid. Something IsAVCC would have previously checked on. > > If it's not valid, that ConvertExtraDataToAnnexB may return nullptr, in which case we will now get a null dereference. So do I need to use IsAvcc instead as I wrote before? Thanks

Hi jya, Please see my reply. Thank you.

Comment on attachment 8864446 [details] Bug 1361984 - Fix the logic of AnnexB::ConvertSampleToAnnexB that checks the sample's extradata field even the sample is AVCC format converted by ConvertSampleToAVCC from AnnexB format. https://reviewboard.mozilla.org/r/136128/#review142456 > I think that doensnt mean the nalsize is 1, it just the pattern that the sample starts with 0x000001xxxxxxxxxxxxxxx. Does it make sense? I got your point, if I reword "start with 0x00000001 or 0x000001 which causes the " to "start with 0x000001 which causes the" Is it ok?

Comment on attachment 8864446 [details] Bug 1361984 - Fix the logic of AnnexB::ConvertSampleToAnnexB that checks the sample's extradata field even the sample is AVCC format converted by ConvertSampleToAVCC from AnnexB format. https://reviewboard.mozilla.org/r/136128/#review142456 > I got your point, if I reword "start with 0x00000001 or 0x000001 which causes the > " to "start with 0x000001 which causes the" > Is it ok? No, it is not possible for an AVCC NAL to start with 000001. So the comment is wrong. What you are doing isn't working around an AVCC NAL starting doesn't start with 0000001, but checking that the extradata isn't null. So what we want is return early *if* it's AnnexB *and) we don't have extradata, as this indicates aSample comes from an earlier conversion AnnexB->AVCC

Comment on attachment 8864446 [details] Bug 1361984 - Fix the logic of AnnexB::ConvertSampleToAnnexB that checks the sample's extradata field even the sample is AVCC format converted by ConvertSampleToAVCC from AnnexB format. https://reviewboard.mozilla.org/r/136128/#review142458 > So do I need to use IsAvcc instead as I wrote before? Thanks you can't use IsAVCC, this isn't AVCC, aSample has been converted to AnnexB right above. *that* is the bug that caused the YouTube problem. Not that the AVCC started with 0000001.

Hello Jya, Thanks for your feedback. The log is generated from the patch attachment 8867703 [details] [diff] [review] when playing with https://assets14.ign.com/videos/zencoder/2017/03/27/960/43d38201fe12e5c79989bbf6d4e89a41-1129000-1490639127.mp4 I just noticed the AVCC sample may start with 0x 00 00 01 a8 like the log shown "@@@@@@@@@The sample is avcc but starting with 0, 0, 1, 168 ,65". Therefore, the IsAnnexB returns true then return, so I said it is the root cause for this bug. And the IsAVCC check returns true since the mExtraData is still valid(except our HLS case which may got mExtraData with nullptr). Please take a look. I think this is the easiest way for me to handle the issue and fulfill our HLS case. Thank you so much

I think the entire assumption that we can detect annexB from Avcc by simply looking at the binary data is flawed. We can't reliably do so.. every attempt to do that is just a workaround working on a small subset of examples... So we should close this bug as won't fix or invalid and instead amend VideoData so that the type is indicated in the MediaRawData. Also, rather than working in place for the conversion, keep the original sample and pass that to the decoder (with the extra data field updated if necessary)

(In reply to Jean-Yves Avenard [:jya] from comment #31) > I think the entire assumption that we can detect annexB from Avcc by simply > looking at the binary data is flawed. We can't reliably do so.. every > attempt to do that is just a workaround working on a small subset of > examples... > Sure. > So we should close this bug as won't fix or invalid and instead amend > VideoData so that the type is indicated in the MediaRawData. > ^^^^^^^^^^^^ Do you mean MediaData? Add a field like bool mBypassToDecoder? > Also, rather than working in place for the conversion, keep the original > sample and pass that to the decoder (with the extra data field updated if > necessary) We should extract for SPS/PPS to detect the changes. Could we get rid of the conversion? Or just add a flag in MediaData like mIsAnnexB(too specified for H264 but I cannot figure out a better one.) and use this flag to deal with the logic in AnnexB::ConvertSampleToAnnexB like I did instead of checking the binary. Honestly, I don't really understand what I should do next, or we could refine the logic of IsAnnexB rather than just checking the starting code to determine if it is AnnexB. Thank you so much.

It appears to me that you can't reliably (or confidently) detect if a MediaRawData is made of AVCC format or AnnexB. Sure, we can kind of guess based on some current characteristics (does it have an extradata field or start with 0x00 00 01 or 0x00 00 00 01 and a combination of those), but I'm afraid we could always miss something, and quite honestly it shouldn't be up to the converter function to determine what type of data is input. We should change the definition of AnnexB::ConvertSampleToAnnexB to AnnexB::ConvertAVCCSampleToAnnexB, so that there are no ambiguity. The only thing that knows for sure is the demuxer who produced the MediaRawData. So I think we should add a new field to MediaRawData, an enum type like ContentHint, something like: enum ContentHint { kAnnexB, kOther } (In reply to James Cheng[:JamesCheng] from comment #32) > We should extract for SPS/PPS to detect the changes. > Could we get rid of the conversion? yes, we still need to extract the SPS/PPS if the decoder requires it. But do we need to? If the demuxer knows for sure the content metadata (width, height, refresh rate etc...) then we could do without. You've written the HLS Demuxer, does it know for certain the metadata? do we need to extract it from the SPS? If you do need to extract the SPS and decode it, the current code currently only works with AVCC. But it doesn't have to be. I see several possible options here: - Continue to convert the AnnexB data into AVCC just for the purpose of extracting the SPS. or: - Write a new function that will extract the SPS/PPS from AnnexB content. The later wouldn't be that hard. Most of the primitive to do so are there. I can help you write those. > Or just add a flag in MediaData like mIsAnnexB(too specified for H264 but I > cannot figure out a better one.) and use this flag to deal with the logic in > AnnexB::ConvertSampleToAnnexB like I did instead of checking the binary. > > Honestly, I don't really understand what I should do next, or we could > refine the logic of IsAnnexB rather than just checking the starting code to > determine if it is AnnexB. I hope things are more clear now... I'm available from Thursday if you still need to discuss about it further.

(In reply to James Cheng[:JamesCheng] from comment #30) > I just noticed the AVCC sample may start with 0x 00 00 01 a8 > like the log shown "@@@@@@@@@The sample is avcc but starting with 0, 0, 1, > 168 ,65". > > Therefore, the IsAnnexB returns true then return, so I said it is the root > cause for this bug. Doh! of course! my deepest apologies for not having considered this case. I had totally forgotten that the annex b marker could be stored on 3 bytes only.

With bug 1365227, this bug should no longer be necessary and I think the H264Converter should work straight away with the HLS generated data.

bug 1365227 deals with the intention of this bug, mark as "won't fix"