Closed Bug 1145926 Opened 6 years ago Closed 6 years ago

Merge PlatformDecoderModule and AVCCDecoderModule

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: jya, Assigned: jya)

Details

Attachments

(2 files, 6 obsolete files)

Both the SharedDecodersManager and AVCC wrapper performs similar functionalities.

The SharedDecodersManager expect the managed decoder to handle annexB while the AVCC wrapper allows to use AVCC decoder and feed it annexB content.

Those two decoder managers should be merged and the functionality of converting the data to AVCC and re-creating decoders on the fly should be performed by the SharedDecodersManager
Attached patch Part1. Refactor AVCC wrapper (obsolete) — Splinter Review
Rework the h264 wrapper. Separate it in its own wrapper folder. Aim is to have AAC/ADTS converter in there. Add mechanism to monitor the format (even for annexB stream) and inform the decoder of format change.
Attachment #8582947 - Flags: review?(cpearce)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
Save parameters used in SDM and reuse them when re-creating a decoder
Attachment #8582951 - Flags: review?(cpearce)
Attached patch Part1. Refactor AVCC wrapper (obsolete) — Splinter Review
Another refactor. We generalise the need that in the future we will have to handle more than just AVCC conversion (will need audio too).
Attachment #8584277 - Flags: review?(cpearce)
Attachment #8582947 - Attachment is obsolete: true
Attachment #8582947 - Flags: review?(cpearce)
Attachment #8584277 - Attachment is obsolete: true
Attachment #8584277 - Flags: review?(cpearce)
Attached patch Part1. Refactor AVCC wrapper (obsolete) — Splinter Review
Fix missing explicit.
Attachment #8585221 - Flags: review?(cpearce)
Comment on attachment 8585221 [details] [diff] [review]
Part1. Refactor AVCC wrapper

Review of attachment 8585221 [details] [diff] [review]:
-----------------------------------------------------------------

r- because I think you shouldn't need to add CreateSafeVideoDecoder(), and I think you break the build on B2G with this. Other than that, looks pretty good.

::: dom/media/fmp4/MP4Reader.cpp
@@ +496,2 @@
>        mVideo.mDecoder =
>          mSharedDecoderManager->CreateVideoDecoder(mPlatform,

So only the SharedDecoderManager uses the unsafe CreateVideoDecoder()? I don't see why you need both safe and unsafe CreateVideoDecoder functions. Surely CreateVideoDecoder() could do the Right Thing regardless?

::: dom/media/fmp4/PlatformDecoderModule.cpp
@@ +177,5 @@
>  }
>  
> +already_AddRefed<MediaDataDecoder>
> +PlatformDecoderModule::CreateSafeVideoDecoder(const mp4_demuxer::VideoDecoderConfig& aConfig,
> +                       layers::LayersBackend aLayersBackend,

Hanging args should line up with the '(' character.

::: dom/media/fmp4/PlatformDecoderModule.h
@@ +89,5 @@
>  
> +  // Creates a Video decoder.
> +  // See CreateVideoDecoder for implementation details.
> +  virtual already_AddRefed<MediaDataDecoder>
> +  CreateSafeVideoDecoder(const mp4_demuxer::VideoDecoderConfig& aConfig,

Why would you want to create an unsafe decoder? Can you just make the existing implementations "safe" and not add more variants?

@@ +112,5 @@
> +  enum ConversionRequired {
> +    kNeedNone,
> +    kNeedAVCC,
> +    kNeedAnnexB,
> +    kNeedADTS,

kNeedADTS is unused. May as well not have it here.

@@ +115,5 @@
> +    kNeedAnnexB,
> +    kNeedADTS,
> +  };
> +
> +  // Indicates if the video decoder requires input conversion.

Does the return value indicate the format that the input track config must be converted to before it can be input into a MediaDataDecoder created by this PDM? Your comment should say that, and note that you include ADTS here, so your comment shouldn't make out that this only applies to video (unless it does only apply to video).

@@ +269,5 @@
>    virtual void AllocateMediaResources() {}
>    virtual void ReleaseMediaResources() {}
>    virtual bool IsHardwareAccelerated() const { return false; }
> +
> +  // Callback to inform the video or audio decoder that the format of the next

Probably not good to call it a "Callback" in the comment here, as the implementor is "called", rather than called back in response to something (right?). There's another interface here for callbacks, the MediaDataDecoderCallback, and we don't want to confuse them.

::: dom/media/fmp4/gonk/GonkVideoDecoderManager.cpp
@@ -452,5 @@
>                           0);
>  }
>  
> -bool
> -GonkVideoDecoderManager::PerformFormatSpecificProcess(mp4_demuxer::MP4Sample* aSample)

MXR thinks this function is called by GonkMediaDataDecoder. So you'll need to fix that if you're going to build on FirefoxOS.

http://mxr.mozilla.org/mozilla-central/source/dom/media/fmp4/gonk/GonkMediaDataDecoder.cpp#66

::: dom/media/fmp4/wrappers/H264Converter.cpp
@@ +13,5 @@
> +
> +namespace mozilla
> +{
> +
> +  // H264 AnnexB or AVCC handler

Indentation of this comment off.
Attachment #8585221 - Flags: review?(cpearce) → review-
Attachment #8582951 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #5)
> Comment on attachment 8585221 [details] [diff] [review]
> Part1. Refactor AVCC wrapper
> 
> Review of attachment 8585221 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- because I think you shouldn't need to add CreateSafeVideoDecoder(), and I
> think you break the build on B2G with this. Other than that, looks pretty
> good.

It does build and run on B2G just fine (https://treeherder.mozilla.org/#/jobs?repo=try&revision=dee6ff1630f4) (there were infrastructure issues, but can't do anything about those two)

> 
> ::: dom/media/fmp4/MP4Reader.cpp
> @@ +496,2 @@
> >        mVideo.mDecoder =
> >          mSharedDecoderManager->CreateVideoDecoder(mPlatform,
> 
> So only the SharedDecoderManager uses the unsafe CreateVideoDecoder()? I
> don't see why you need both safe and unsafe CreateVideoDecoder functions.
> Surely CreateVideoDecoder() could do the Right Thing regardless?
> 

This is SharedDecoderManager::CreateVideoDecoder, it has nothing to do with the PlatformDecoderModule::CreateVideoDecoder.

The SharedDecoderManager itself call CreateSafeVideoDecoder

> ::: dom/media/fmp4/PlatformDecoderModule.cpp
> @@ +177,5 @@
> >  }
> >  
> > +already_AddRefed<MediaDataDecoder>
> > +PlatformDecoderModule::CreateSafeVideoDecoder(const mp4_demuxer::VideoDecoderConfig& aConfig,
> > +                       layers::LayersBackend aLayersBackend,
> 
> Hanging args should line up with the '(' character.

that's the exact same formatting as it had before the rename.

> Why would you want to create an unsafe decoder? Can you just make the
> existing implementations "safe" and not add more variants?

If you have a better suggestion for the naming, I'm all for it.

I felt it was easier to create a CreateSafe*Decoder than modifying all PDM implementations and name that say Create*DecoderInternal. This was the only rationale behind the naming.

The decoder returned by the PlatformDecoderModule now have the extra requirements that it must be able to handle different format on input (change of resolution etc). To facilitate this requirement, it is now the responsibility of the PlatformDecoderModule to handle change of content by feeding data to decoders able to handle it, or destroy / recreate the decoders who can't (like AVCC h264 decoders) 

> 
> @@ +112,5 @@
> > +  enum ConversionRequired {
> > +    kNeedNone,
> > +    kNeedAVCC,
> > +    kNeedAnnexB,
> > +    kNeedADTS,
> 
> kNeedADTS is unused. May as well not have it here.

it's not there ... *yet*
could of course add it later. When the conversion to ADTS is moved back to the PDM like the conversion to AVCC or AnnexB was.

That was my Part3.

> Does the return value indicate the format that the input track config must
> be converted to before it can be input into a MediaDataDecoder created by
> this PDM? Your comment should say that, and note that you include ADTS here,

yes, the conversion must happen before MediaDataDecoder::Input() is called.

> so your comment shouldn't make out that this only applies to video (unless
> it does only apply to video).

currently it only happens to video, however we will handle ADTS the same way. Some AAC decoders need ADTS (ffmpeg, gonk), some can take raw aac (windows and mac)

> 
> @@ +269,5 @@
> >    virtual void AllocateMediaResources() {}
> >    virtual void ReleaseMediaResources() {}
> >    virtual bool IsHardwareAccelerated() const { return false; }
> > +
> > +  // Callback to inform the video or audio decoder that the format of the next
> 
> Probably not good to call it a "Callback" in the comment here, as the
> implementor is "called", rather than called back in response to something
> (right?). There's another interface here for callbacks, the
> MediaDataDecoderCallback, and we don't want to confuse them.

ok.

> 
> ::: dom/media/fmp4/gonk/GonkVideoDecoderManager.cpp
> @@ -452,5 @@
> >                           0);
> >  }
> >  
> > -bool
> > -GonkVideoDecoderManager::PerformFormatSpecificProcess(mp4_demuxer::MP4Sample* aSample)
> 
> MXR thinks this function is called by GonkMediaDataDecoder. So you'll need
> to fix that if you're going to build on FirefoxOS.

no.. because the default GonkMediaDataDecoder::PerformFormatSpecificProcess implementation does nothing ( virtual bool PerformFormatSpecificProcess(mp4_demuxer::MP4Sample* aSample) { return true; } ).
By removing it from GonkVideoDecoderManager, we simply stop doing anything special for H264 (as this is operation is done by the H264Decoder).

Once the ADTS conversion is also done in a similar fashion as the AnnexB conversion, there will be no need for PerformFormatSpecificProcess.

> http://mxr.mozilla.org/mozilla-central/source/dom/media/fmp4/gonk/
> GonkMediaDataDecoder.cpp#66
> 
> ::: dom/media/fmp4/wrappers/H264Converter.cpp
> @@ +13,5 @@
> > +
> > +namespace mozilla
> > +{
> > +
> > +  // H264 AnnexB or AVCC handler
> 
> Indentation of this comment off.
Flags: needinfo?(cpearce)
Flags: needinfo?(cpearce)
re-implement as discussed. In the process, combined the Supports*MimeType into on, seeing that we have one CreateDecoder now.
Attachment #8586524 - Flags: review?(cpearce)
Attachment #8585221 - Attachment is obsolete: true
Attachment #8582951 - Attachment is obsolete: true
Attachment #8586524 - Flags: review?(cpearce) → review+
Summary: Merge SharedDecoderManager and AVCCDecoderModule → Merge PlatformDecoderModule and AVCCDecoderModule
Remove remnant of libstagefright. This codepath is now unused
Attachment #8587147 - Flags: review?(ajones)
Comment on attachment 8587147 [details] [diff] [review]
Part1. Remove of now unused code path

wrong bug #
Attachment #8587147 - Attachment is obsolete: true
Attachment #8587147 - Flags: review?(ajones)
Remove remnant of libstagefright. This codepath is now unused
Attachment #8587155 - Flags: review?(ajones)
Comment on attachment 8587155 [details] [diff] [review]
Part1. Remove of now unused code path

still wrong bug # !
Attachment #8587155 - Attachment is obsolete: true
Attachment #8587155 - Flags: review?(ajones)
https://hg.mozilla.org/mozilla-central/rev/8d4f2e3618b7
https://hg.mozilla.org/mozilla-central/rev/f7996461e200
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
In the first patch here, the DecoderNeedsConversion() method-decl (in BlankDecoderModule.cpp) was missing the 'override' keyword, which breaks clang 3.6 warnings-as-error builds. (build warning in bug 1117034)

I pushed a followup to add that keyword, with blanket r+ that ehsan granted me for fixes of this sort over in bug 1126447 comment 2:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/4ab18dfd2884

(I also added 'virtual', for consistency with other overridden virtual methods in the same class & with other DecoderNeedsConversion impls. That annotation has no effect, since the inherited method is already virtual; I just added it for consistency/clarity.)
Thanks for that
You need to log in before you can comment on or make changes to this bug.