1187118 - Add a GIF MediaDecoder to allow playback of animated GIFs in the <video> element

Status: NEW

(Core :: Audio/Video: Playback, defect, P5)

Description

[Seth Fowler [:seth] [:s2h]]

As part of the long term goal of playing animated GIFs through the media framework, we'd like to start by supporting them in the video element.

This work should proceed behind a pref.

Comment 1

[Jean-Yves Avenard [:jya]]

An introduction note:

we are trying to move aways from the MediaDecoderReader architecture where we have one blob doing the parsing of the raw data and decoding.

Instead you only need to implement a MediaDataDemuxer (bug 1154164) that will extract raw compressed samples (a MediaRawData object) and then implement a MediaDataDecoder which uncompress a single MediaRawData into a VideoData object (a picture really)

The overall architecture is described in bug 1148292.

The aim is to be later able to reuse the codec with different containers if need be. Probably not a worry with gif.

But mostly then you benefit from all the goodness of the MediaFormatReader which provides async / highly multi-threaded decoding.  

So in the end, we only have a single MediaDecoderReader and a pool of demuxer and codecs.

Comment 2

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Jean-Yves Avenard [:jya] from comment #1)
> We are trying to move aways from the MediaDecoderReader architecture where
> we have one blob doing the parsing of the raw data and decoding.

In an effort to not understate our intent here, we won't be accepting any new subclasses of MediaDecoderReader.

Comment 3

[Mihai Volmer [:mihaivolmer]]

Attachment: (Part 1) - Added support for frame by frame decoding in nsGIFDecoder2

Comment 4

[Mihai Volmer [:mihaivolmer]]

Attachment: (Part 2) - Added GIFDecoder and GIFDemuxer classes

Comment 5

[Mihai Volmer [:mihaivolmer]]

Attachment: (Part 4) - Integrated the GIFDecoder into DecoderTraits

Comment 6

[Mihai Volmer [:mihaivolmer]]

Attachment: (Part 5) - Added GIFDataDecoder implementation of MediaDataDecoder

Comment 7

[Mihai Volmer [:mihaivolmer]]

Attachment: (part 6) - Integrated the GIFDataDecoder as an Agnostic Mime Type in PlatformDecoderModule

Comment 8

[Mihai Volmer [:mihaivolmer]]

Attachment: (Part 7) - Keep track of how many VideoData objects are created and how many are destroyed at any given moment during playback

Comment 9

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Added support for frame by frame decoding in nsGIFDecoder2

Rebased version of part 1.

Comment 10

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Added GIFDecoder and GIFDemuxer classes

Rebased part 2.

Comment 11

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Integrated the GIFDecoder into DecoderTraits

Rebased part 3.

Comment 12

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 4) - Added GIFDataDecoder implementation of MediaDataDecoder

Rebased part 4.

Comment 13

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 5) - Integrated the GIFDataDecoder as an Agnostic Mime Type in PlatformDecoderModule

Rebased part 5.

Comment 14

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 6) - Keep track of how many VideoData objects are created and how many are destroyed at any given moment during playback

Rebased part 6.

Comment 15

[Mihai Volmer [:mihaivolmer]]

Attachment: (Part 1) - Added support for frame by frame decoding in nsGIFDecoder2

Comment 16

[Mihai Volmer [:mihaivolmer]]

Attachment: (Part 2) - Added GIFDecoder and GIFDemuxer classes

Comment 17

[Mihai Volmer [:mihaivolmer]]

Attachment: (Part 3) - Integrated the GIFDecoder into DecoderTraits

Comment 18

[Mihai Volmer [:mihaivolmer]]

Attachment: (Part 4) - Added GIFDataDecoder implementation of MediaDataDecoder

Comment 19

[Mihai Volmer [:mihaivolmer]]

Attachment: (Part 5) - Integrated the GIFDataDecoder as an Agnostic Mime Type in PlatformDecoderModule

Comment 20

[Mihai Volmer [:mihaivolmer]]

Attachment: (Part 6) - Keep track of how many VideoData objects are created and how many are destroyed at any given moment during playback

There are printfs in both the constructor and the destructor of the VideoData class. This way we can keep track of how many VideoData objects we have in memory at any given moment.

Comment 21

[Chris Peterson [:cpeterson]]

Comment on attachment 8664543 [details] [diff] [review]
(Part 6) - Keep track of how many VideoData objects are created and how many are destroyed at any given moment during playback

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

::: dom/media/MediaData.cpp
@@ +122,5 @@
>  {
>    NS_ASSERTION(mDuration >= 0, "Frame must have non-negative duration.");
>    mKeyframe = aKeyframe;
>    mTimecode = aTimecode;
> +  printf("gifmediamemory %p created\n", this);

We don't want to include printfs in release builds. You might want to use MOZ_LOG.

Comment 22

[Jean-Yves Avenard [:jya]]

Attachment: 0001-Bug-1187118-Comments-and-fix-up.patch

quick fly over.

The image displays quickly because you set the duration of each frames that gives a refresh rate of 100Hz, causing the 77 images to be displayed in .7s only.

this didn't compile for me on mac with clang (error with the gotos and variables not being initialised / defined)

i was playing that GIF:
http://netdna.webdesignerdepot.com/uploads7/50-inspiring-animated-gifs/006.gif

it shows a black frame after a few images and the background becomes black.

not sure why

Comment 23

[Seth Fowler [:seth] [:s2h]]

(In reply to Chris Peterson [:cpeterson] from comment #21)
> We don't want to include printfs in release builds. You might want to use
> MOZ_LOG.

This code is not ready for review.

Comment 24

[Mihai Volmer [:mihaivolmer]]

Thank you for your feedback!

(In reply to Jean-Yves Avenard [:jya] from comment #22)
> The image displays quickly because you set the duration of each frames that
> gives a refresh rate of 100Hz, causing the 77 images to be displayed in .7s
> only.

I have tried with 0.039 and it still doesn't seem to affect the frame duration. Did this value work for you?

> this didn't compile for me on mac with clang (error with the gotos and
> variables not being initialised / defined)

This is strange because the "done" label was used before I even touched the code. I will redesign the "frame by frame" decoding code. However, I don't know what to say about the "done" label.

> i was playing that GIF:
> http://netdna.webdesignerdepot.com/uploads7/50-inspiring-animated-gifs/006.
> gif
> 
> it shows a black frame after a few images and the background becomes black.
> 
> not sure why

There are still some GiFs that crash. Also, loading the GIF directly seems to crash. Including them in <video> elements is working.
I used this for tests: 
http://people.mozilla.org/~mvolmer/testpage2.html

Comment 25

[Jean-Yves Avenard [:jya]]

(In reply to Mihai Volmer [:mihaivolmer] from comment #24)
> Thank you for your feedback!
> 
> (In reply to Jean-Yves Avenard [:jya] from comment #22)
> > The image displays quickly because you set the duration of each frames that
> > gives a refresh rate of 100Hz, causing the 77 images to be displayed in .7s
> > only.
> 
> I have tried with 0.039 and it still doesn't seem to affect the frame
> duration. Did this value work for you?

it did yes (for the gif I was using)

> 
> > this didn't compile for me on mac with clang (error with the gotos and
> > variables not being initialised / defined)
> 
> This is strange because the "done" label was used before I even touched the
> code. I will redesign the "frame by frame" decoding code. However, I don't
> know what to say about the "done" label.

it's the goto finished_frame; that gave error
likely because bool hitFinishedFrame = false; is defined after the goto, but before the label.

moving +  bool hitFinishedFrame = false; to be before the loop will likely do the trick too

(still ugly imho)

Comment 26

[Seth Fowler [:seth] [:s2h]]

(In reply to Jean-Yves Avenard [:jya] from comment #25)
> moving +  bool hitFinishedFrame = false; to be before the loop will likely
> do the trick too
> 
> (still ugly imho)

That code was just a hack to get things to work for the demo. I wouldn't r+ it. In fact, we're probably going to need to semi-rewrite the GIF decoder before we actually land anything in this bug. (See bug 1204392.)

Comment 27

[Seth Fowler [:seth] [:s2h]]

Mihai, I'm clearing these feedback flags because I think we need to make progress on getting basic functionality working before we're ready for detailed feedback.

At this time I think the biggest concern is fixing the frame duration problem.

Comment 28

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1187118 - [WIP] move FrameTimeout into a seperate header;

Review commit: https://reviewboard.mozilla.org/r/114884/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114884/

Comment 29

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1187118 part 1 - [WIP] Added GIFDecoder and GIFDemuxer classes;

Review commit: https://reviewboard.mozilla.org/r/114886/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114886/

Comment 30

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1187118 part 2 - [WIP] Integrated the GIFDecoder into DecoderTraits;

Review commit: https://reviewboard.mozilla.org/r/114888/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114888/

Comment 31

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1187118 part 3 - [WIP] Added GIFDataDecoder implementation of MediaDataDecoder;

Review commit: https://reviewboard.mozilla.org/r/114890/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114890/

Comment 32

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1187118 part 4 - [WIP] Integrated the GIFDataDecoder into AgnosticDecoderModule;

Review commit: https://reviewboard.mozilla.org/r/114892/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114892/

Comment 33

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1187118 - [WIP] debug log

Review commit: https://reviewboard.mozilla.org/r/114894/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114894/

Comment 34

[Tzuhao Kuo [:kaku]]

Attachment: Bug 1187118 - [WIP] disable an assertion for debug build;

Review commit: https://reviewboard.mozilla.org/r/114896/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114896/

Comment 35

[Liz Henry (:lizzard) (relman/hg->git project)]

Mihai, are you still working on this or should we unassign it?
Looks like the dependent bugs are now fixed.

Comment 36

[Caspy7]

It's been 2 months without response, perhaps Mihai is MIA? (or needs another ping?)